-
Notifications
You must be signed in to change notification settings - Fork 413
Fix #2226 #2249
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 #2226 #2249
Conversation
| var availableHelpOptions = | ||
| parseResult | ||
| .CommandResult | ||
| .Command | ||
| .RecurseWhileNotNull(c => c.Parents.OfType<CliCommand>().FirstOrDefault()) | ||
| .Select(c => c.Options.OfType<HelpOption>().FirstOrDefault()); |
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.
I feel this recursion should be on CommandResult.Parent rather than CliCommand.Parents. So that if the same CliCommand is reachable as app subcommand1 leafcommand and app subcommand2 leafcommand, then it picks up a HelpOption from either subcommand1 or subcommand2, depending on how the user ran the command.
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.
I mean something like this, but I did not try compiling:
var availableHelpOptions =
parseResult
.CommandResult
.RecurseWhileNotNull(r => r.Parent as CommandResult)
.Select(r => r.Command.Options.OfType<HelpOption>().FirstOrDefault());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.
That works. Thanks for the suggestion!
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.
On second thought, this does not work: if some of the commands in the result hierarchy do not have a HelpOption, then the availableHelpOptions sequence will have null references in the corresponding positions; and if the first item is null (because the leaf command does not have its own HelpOption) then the subsequent availableHelpOptions.FirstOrDefault() call will return that null and ignore any HelpOption references later on in the sequence. So you need something to filter the nulls out too.
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.
Thanks. I added a test and fixed this in the latest commit.
| private static void WriteHelp(ParseResult parseResult) | ||
| { | ||
| new HelpAction().Invoke(parseResult); | ||
| var availableHelpOptions = |
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.
I have fixed this bug once: #2211, how is it possible that it's back? Some merge issue?
Since ParseResult exposes RootCommandResult and RootCommand should be the only command having it, we can simplify it to:
HelpOption helpOption = parseResult.RootCommandResult.Command.Options.FirstOrDefault(option => option is HelpOption) as HelpOption ?? new HelpOption();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.
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.
I'm not sure RootCommand should be the only one with a HelpOption. Isn't it possible for a subcommand to shadow the parent's HelpOption with one that has a different syntax (e.g. supporting --help=online) or different behaviour (e.g. formatting the help from some markup language in resources, rather than from the children of the CliCommand)?
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.
Since ParseResult exposes RootCommandResult and RootCommand should be the only command having it...
This isn't the case. Different help options can be defined at different levels of the hierarchy.
|
I've also removed the direct usage of a new |
This fixes #2226.