-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Refactor help option and improve option parsing from parent when default command is invoked implicitly #1934
Conversation
(cherry picked from commit 87db4ba)
Eliminates outputHelpIfRequested() and incorporates help option parsing into .parseOptions() instead for consistency with other options. Additionally, improvements to the .parseOptions() code have been made, first and foremost to the comments.
Eliminates the need for the check in other places. (cherry picked from commit a12f5be)
Quick comment. I have wondered at times whether the help option and command should be custom, but decided each time that they do have some deliberately special behaviours. There is special checks and treatment of the help option scattered through the code. Some for historical reasons, but some carefully crafted to achieve the desired outcome. |
Converted to draft since obscured help flags are currently not handled correctly. |
Achieved by giving up the earlier introduced 'option:' events for help and a better .parseOptions() design. The default command now concedes the help option to the parent in exactly the same cases in which all other options would be conceded.
Add a displayHelp property to the returned object indicating whether a help flag had been found before encountering a subcommand, or before giving up option processing in favor of subcommands due to passThroughOptions being enabled. Before encountering a subcommand, the help option is now consumed in exactly the same cases any other option would be consumed.
So that the help option is correctly consumed.
fcfbec1
to
eae6c5f
Compare
(Force push is due to a commit amendment changing a commit message that had mixed up options and commands.) |
This reverts commit eae6c5f.
The revert is due to the fact that stopping processing at unknown options when For consistency with how other options work, the help option will be conceded to the subcommand if encountered after an unknown option and both |
Includes a partial fix for tj#1936 (docs need an update, too)
Please check out the updated PR description. I hope I could make my motivation and some implementation details more clear. |
(cherry picked from commit a8b656d)
Reverts to original conditionals structure so that implicitly invoked default commands behave as if the command name came before the first arg that is not a known option, and not as if it came before the first arg that is not an option (known or unknown) like previously. Fixes pass through and handling of options after the unknown for implicitly invoked default commands.
Eliminates redundant calls and checks.
Partially borrowed from tj#1934
(cherry picked from commit 0931218)
This reverts commit e8bea4a.
f814096
to
e01bb9a
Compare
This is two different things, neither of which I am convinced of the need for at this time. Reducing my review workload by closing this without further discussion. |
An extension of #1929 refactoring the help option.
Has been merged with the already approved tiny fixes PR #1930 because the parent #1929 has been merged with it.
Problem
4788f70 introduced
outputHelpIfNecessary()
as part of a solution to #2. It was later renamed tooutputHelpIfRequested()
in #1131 and is used in the library code to this day. However, its use is inconsistent with how all other options are parsed, namely in the.parseOptions()
method.Solution
Eliminate
outputHelpIfRequested()
and incorporate help option parsing into.parseOptions()
instead.The object returned by
.parseOptions()
now includes adisplayHelp
property indicating whether a help flag had been found before encountering a subcommand, or before encountering either a subcommand or a command-argument when the default command is being invoked implicitly.The last part definitely needs clarification. The problem is that currently, there is an inconsistency between the way the default command invoked implicitly and explicitly invoked commands are handled: while the parent command ignores help flags found in the input part pertaining to the subcommand (args placed after the subcommand name) when invoked explicitly, the flags are always "robbed" by the parent command when the default command is implicitly invoked.
Here is an example to demonstrate the discrepancy:
This is very unlikely to be what the users of the library expect.
However, we do want the parent command to handle the help option in certain cases, for example when a help flag is placed in the first arg passed to it.
The PR fixes the problem by only letting the implicitly invoked default command's parent consume help flags encountered before the first arg that is not an option (could be a subcommand or a command-argument).
A visual explanation could be that the default subcommand's name is implicitly inserted before the first arg that is not a known option (although that is not what really happens internally), so that
--global-option --unknown-option arg --help
turns into--global-option sub --unknown-option arg --help
.Additionally, improvements to the
.parseOptions()
code have been made, first and foremost to the comments.ChangeLog
Added
displayHelp
property in the object returned by.parseOption()
indicating whether a help flag had been found before encountering a subcommand (or before encountering either a subcommand or a command-argument when the default command is being invoked implicitly)Changed
.createOption()
in.helpOption()
to support custom option flag extractionenablePositionalOptions
, or either a subcommand or a command-argument when usingpassThroughOptions
)