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

Refactor help option and improve option parsing from parent when default command is invoked implicitly #1934

Closed
wants to merge 35 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 4, 2023

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 to outputHelpIfRequested() 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 a displayHelp 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:

program.command('sub <arg>', { isDefault: true });
program.parse(['sub', 'arg', '--help'], { from: 'user' }); // help for sub is displayed
program.command('sub <arg>', { isDefault: true });
program.parse(['arg', '--help'], { from: 'user' }); // help for program is displayed

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

  • (from #1929) Breaking: use .createOption() in .helpOption() to support custom option flag extraction
  • Breaking: make option handling when the default command is invoked implicitly consistent with how options are handled for subcommands invoked explicitly via the name or an alias (parent only processes arguments before encountering a subcommand when using enablePositionalOptions, or either a subcommand or a command-argument when using passThroughOptions)

Borrowed from a3f0e28 that was supposed to land in the now-closed tj#1921.
Analogous to 3c94dfd in tj#1933 for version option.

Storing the Option instance of the help option instead of just its
flags (_helpShortFlag, _helpLongFlag) is meaningful for cases when other
properties of the instance need to be accessed (not currently the case,
but could be in the future).
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)
@shadowspawn
Copy link
Collaborator

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.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 4, 2023

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.
@aweebit aweebit force-pushed the feature/help-option-refactor branch from fcfbec1 to eae6c5f Compare August 4, 2023 16:52
@aweebit
Copy link
Contributor Author

aweebit commented Aug 4, 2023

(Force push is due to a commit amendment changing a commit message that had mixed up options and commands.)

@aweebit
Copy link
Contributor Author

aweebit commented Aug 5, 2023

The revert is due to the fact that stopping processing at unknown options when passThroughOptions is on is expected behavior, as explained in #1936 (comment).

For consistency with how other options work, the help option will be conceded to the subcommand if encountered after an unknown option and both passThroughOptions and allowUnknownOption are on. Consider this expected behavior.

@aweebit aweebit marked this pull request as ready for review August 5, 2023 11:17
@aweebit aweebit marked this pull request as draft August 5, 2023 11:17
@aweebit aweebit changed the title Refactor help option and improve option parsing from parent when default command is invoked indirectly Refactor help option and improve option parsing from parent when default command is invoked implicitly Aug 5, 2023
@aweebit
Copy link
Contributor Author

aweebit commented Aug 5, 2023

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.
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 7, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 7, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 7, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 8, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 8, 2023
- Store onlyKnownOptionsSoFar value instead of computing
- Removed passThroughOptions comment considered unclear (see tj#1942)

(cherry picked from commit d7e2399)
@shadowspawn
Copy link
Collaborator

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.

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.

2 participants