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

Subcommands broken if bin name is different from name of script it resolves to #830

Closed
wants to merge 2 commits into from

Conversation

jd20
Copy link

@jd20 jd20 commented Jul 23, 2018

When using subcommands, you may have a list of scripts like:

cli.js
cli-search.js
cli-parse.js

You might then supply a bin field like:

"bin": { "mytool": "cli.js" }

However, this does not work with subcommands, as Commander.js will search for mytool-search.js, mytool-parse.js, etc... For some projects, renaming all the scripts to be the same as what's in the bin field may not be a viable option.

This fix adds additional logic to check if the symlink in bin resolved to a different name, and if so will use that name only if the regular logic fails to find the subcommand. Thus, this change should not affect cases that were already working before, it would only affect cases that were broken (the new logic is only invoked when Commander.js would bail out after failing to find the subcommand script).

I've also added a test case, which verifies that the new behavior is working properly.

@shadowspawn shadowspawn self-requested a review May 13, 2019 10:11
@shadowspawn shadowspawn removed the request for review from roman-vanesyan June 22, 2019 02:02
@shadowspawn
Copy link
Collaborator

I am leaving this one out of v3 in favour of #854, but would like to come back in future and take a deep look at the name resolution for executables with this Pull Request as one of the likely improvements. No action likely in meantime.

@shadowspawn shadowspawn removed their request for review June 22, 2019 05:04
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The related code has changed significantly, so this PR will need reworking. I am ok with the concept. :-)

I wonder whether it would be better to recursively call executeSubCommand with the new name, to get all the parsing logic?

@shadowspawn
Copy link
Collaborator

@jd20
Are you interested in working more on this?

@shadowspawn
Copy link
Collaborator

There is at least a manual work-around now with the executableFile option for commands. Closing this PR as inactive with conflicts.

Thank you for your contributions.

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

Successfully merging this pull request may close these issues.

None yet

3 participants