Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement read-cmd-shim via a command-based directive instead of regexp parsing #19

Open
cspotcode opened this issue Aug 21, 2019 · 1 comment

Comments

@cspotcode
Copy link

cspotcode commented Aug 21, 2019

Over on npm/cmd-shim, it was mentioned that changing the shim to implement #17 will require a change to read-cmd-shim to parse the new implementation. I guess read-cmd-shim uses a regexp to parse the target path from the shim.

I think it'd be better going forward to embed the target path in a way that can be parsed out more easily. For example, at the top of every shim is a comment like this:

@REM __CMD_SHIM_TARGET__=../foo/bin/bar.js

...or...

# __CMD_SHIM_TARGET__=../foo/bin/bar.js

read-cmd-shim can check for this directive. If found, that's the target path. If not, it'll fall-back to legacy regexp matching. This is a more future-proof approach to parsing the target out of a shim because the shims are free to change in the future. As long as they include that directive, they can be read unambiguously.

...of course I'm not sure who uses read-cmd-shim or what they use it for. So maybe this is not worth the effort. I still want to share the idea.

I also think, if read-cmd-shim is a useful feature, it makes more sense to include it in cmd-shim than to implement as a separate module. It's not much code and putting them in 2 places is just lots of duplicated boilerplate and configuration.

@ExE-Boss
Copy link
Member

When linking to external websites, you have to specify the scheme, eg.:

[read-cmd-shim](https://npm.im/read-cmd-shim)

instead of

[read-cmd-shim](npm.im/read-cmd-shim)

as the latter produces a relative link to https://github.com/pnpm/cmd-shim/issues/npm.im/read-cmd-shim, which doesn't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants