-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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(help): Partial fix for 'help help' #2887
Conversation
parser.app.settings = parser.app.settings | self.app.g_settings; | ||
parser.app.g_settings = self.app.g_settings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the first line seems to be manual propagation of a global setting but its unclear
- Why do we need the second line?
- Why is this only needed for
help help
compared to anything else?
So I removed it in the name of trying to make help help
behave like everything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add
DisableHelpFlag
and it should remove the--help
flag.
Didn't work.
My intention is to make this incremental improvement and then either immediately dig further into this or create an issue (feels weird creating an issue for a behavior that doesn't exist yet :) ) rather than block progress on getting everything in at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't need to block this if it doesn't work. I thought it would though.
Can I get another test for nested scenario? |
Who knew people need to ask `help` for how to use `help`? While auditing `MultpleValues`, I saw commented out code. Looks its commit (f230cfe) was part of a large refactor and updating that part fell through the cracks. Just simply updating it didn't quite get it to work. The advantage of this approach is it gets us closer to how clap works on its own. In clap 2.33.3, `cmd help help` looks like ``` myapp-help Prints this message or the help of the given subcommand(s) USAGE: test-clap help [subcommand]... ARGS: <subcommand>... The subcommand whose help message to display ``` But clap3 master looks like: ``` myapp USAGE: myapp [SUBCOMMAND] OPTIONS: -h, --help Print custom help about text SUBCOMMANDS: help Print this message or the help of the given subcommand(s) subcmd ``` This change improves it to: ``` myapp-help Print this message or the help of the given subcommand(s) USAGE: myapp help [SUBCOMMAND]... ARGS: <SUBCOMMAND>... The subcommand whose help message to display OPTIONS: -h, --help Print custom help about text ``` We still have global arguments showing up (like `-h`) that will error but its an improvement! In general, I'd like to find a way to leverage clap's stanard behavior for implementing this so we don't have to worry about any of these corner cases in the future. Note: compared to clap2, I changed `<subcommand>` to `<SUBCOMMAND>` because I believe the standard convention is for value names to be all caps (e.g. `clap_derive` has been updated to default to that).
Extra test added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Build succeeded: |
Who knew people need to ask
help
for how to usehelp
?While auditing
MultpleValues
, I saw commented out code. Looksits commit (f230cfe) was part of a large refactor and updating that
part fell through the cracks. Just simply updating it didn't quite get
it to work. The advantage of this approach is it gets us closer to how
clap works on its own.
In clap 2.33.3,
cmd help help
looks likeBut clap3 master looks like:
This change improves it to:
We still have global arguments showing up (like
-h
) that will error but itsan improvement! In general, I'd like to find a way to leverage clap's stanard
behavior for implementing this so we don't have to worry about any of these
corner cases in the future.
Note: compared to clap2, I changed
<subcommand>
to<SUBCOMMAND>
because Ibelieve the standard convention is for value names to be all caps (e.g.
clap_derive
has been updated to default to that).