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

fix showing help for sub commands with custom executableFile #1018

Merged
merged 1 commit into from Aug 16, 2019
Merged

fix showing help for sub commands with custom executableFile #1018

merged 1 commit into from Aug 16, 2019

Conversation

nazieb
Copy link
Contributor

@nazieb nazieb commented Aug 14, 2019

Problem

As of 3.0.0, the git-style executable sub commands can have executableFile option to customize the name of file containing the command.

But running help for these kind of sub commands will return error as the built-in help sub command will try to look executable files with default constructed name.

Example in old version:

program
  .version('0.0.1')
  .command('specifyInstall', 'specify install subcommand', { executableFile: 'pm-install' })
$ ./pm help specifyInstall
error: pm-specifyInstall(1) does not exist, try --help

Solution

Turns out, this is because when we run help, it will be treated as any user defined sub commands with special treatment of switching the arguments in the executeSubCommand function to make it as if we run our custom sub command using --help flag (see here).

This is incorrect behaviour since the executeSubCommand will further assume the executable file name with default constructed name, as the resolving mechanism of the executableFile option is done at the parse function.

This PR fixes the problem by moving the "argument switching" mechanism to the parse function before the resolving mechanism is done, so the executeSubCommand will get correct executable file name.

@shadowspawn shadowspawn self-requested a review August 14, 2019 20:33
@shadowspawn
Copy link
Collaborator

Thank-you for identifying the issue, the PR, and good description.

I have not looked at where the code moved to yet, but a concern I have from description is whether this will affect .action based commands. I'll work it out quickly when I look, but wanted to mention it.

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.

(Does not change action parsing.)

@nazieb
Copy link
Contributor Author

nazieb commented Aug 15, 2019

@shadowspawn thanks for your review 🙇‍♂️

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

Thank you!

@shadowspawn shadowspawn merged commit 3e89cc9 into tj:develop Aug 16, 2019
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Aug 16, 2019
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Aug 30, 2019
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