Skip to content

add global options, make -h a proper option #771

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

Conversation

jonsequitur
Copy link
Contributor

@jonsequitur jonsequitur commented Jan 27, 2020

This reimplements help as a proper Option, which fixes #691. In order for the help option to be a single instance, looked up using FindResultFor after parsing, I also added support for global options (#587).

This should also fix #766, the root cause of which is #691.

This will also fix #506.

@jonsequitur jonsequitur requested a review from Keboo January 27, 2020 14:23
public void There_are_no_parse_errors_when_help_is_invoked_on_root_command()
{
var parser = new CommandLineBuilder()
.UseDefaults()
Copy link
Member

Choose a reason for hiding this comment

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

This makes the assumption that .UseDefaults() includes help. Though probably reasonable, I think it might be more clear to only test with .UseHelp()

};

var parser = new CommandLineBuilder(command)
.UseDefaults()
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. Consider use .UseHelp() rather than .UseDefaults()

{
if (symbol is Option option)
if (child is Command childCommand)
Copy link
Member

Choose a reason for hiding this comment

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

Consider .OfType<Command>() on the foreach to reduce nesting.

@jonsequitur
Copy link
Contributor Author

A consequence of this change is that it also adds help output to --help which is a fairly common behavior and has been requested (#506), but it might be a little too helpful.

Usage:
  myapp [options]

Options:
  --version                Show version information
  -h, /h, -?, /?, --help   Show help and usage information
  -f                       Do some other stuff

@Keboo
Copy link
Member

Keboo commented Jan 28, 2020

Any thoughts on changing the output on help to not display items that only differ by prefix? I would assume that would then make the output look something like:

Usage:
  myapp [options]

Options:
  --version               Show version information
  -h, -?, --help          Show help and usage information
  -f                      Do some other stuff

@jonsequitur
Copy link
Contributor Author

I like that. It does leave aside the question of Windows-style option prefixes (#351).

@KathleenDollard
Copy link
Contributor

  1. Looking at the issues list, a trifecta is always nice :)
  2. I think this is a good change. I do think it highlights that we have more work to do on the help output (including prefixes and how --help appears from this conversation). I don't think that should hold up this PR.

Let's do it!

@jonsequitur jonsequitur force-pushed the add-global-options-and-make-help-a-proper-option branch from d46ba83 to c9d2900 Compare January 29, 2020 15:42
@jonsequitur
Copy link
Contributor Author

Any thoughts on changing the output on help to not display items that only differ by prefix? I would assume that would then make the output look something like:

Usage:
  myapp [options]

Options:
  --version               Show version information
  -h, -?, --help          Show help and usage information
  -f                      Do some other stuff

@Keboo Looks what you've made me do...

c9d2900#diff-ed0644cdb2d64176756014bb5d1a7115R335-R342

@Keboo
Copy link
Member

Keboo commented Jan 29, 2020

@jonsequitur nice I like it

@jonsequitur jonsequitur force-pushed the add-global-options-and-make-help-a-proper-option branch from c9d2900 to a325d88 Compare January 29, 2020 17:55
@jonsequitur jonsequitur merged commit 6274e0a into dotnet:master Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants