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

arg_invalid_show_help(bool) option #4218

Closed
2 tasks done
ozgunozerk opened this issue Sep 15, 2022 · 7 comments
Closed
2 tasks done

arg_invalid_show_help(bool) option #4218

ozgunozerk opened this issue Sep 15, 2022 · 7 comments
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@ozgunozerk
Copy link

ozgunozerk commented Sep 15, 2022

Please complete the following tasks

Clap Version

3.2.21

Describe your use case

In case of supplying an invalid subcommand, help should be displayed with a default message.

See for example:
input: myApp invalidCommand
output:

myApp: 'invalidCommand' is not a valid command
*help is displayed here*

In order to achieve this, I have to use try_get_matches, and apply some error handling on the Result, which adds unnecessary complexity to my code, and makes it uglier.

This problem is very similar to having or not having the arg_required_else_help feature.
Since we have that, I expected to have this one as well, and spent a good amount of time searching this feature on docs, and in here.

Describe the solution you'd like

a similar feature is present for the absence of subcommands -> .arg_required_else_help(true)
so, there can be something like arg_invalid_show_help(bool) as well imho. Would be handy for many developers.

In my case, the custom code I needed to write for this made the code uglier, and I think it should be fairly easy the implement this feature.
In fact, I can volunteer for implementing it if you guys are busy with other things :)

Alternatives, if applicable

No response

Additional Context

No response

@ozgunozerk ozgunozerk added the C-enhancement Category: Raise on the bar on expectations label Sep 15, 2022
@epage
Copy link
Member

epage commented Sep 15, 2022

Since the Issue doesn't include a "current" case,

#!/usr/bin/env -S rust-script --debug

//! ```cargo
//! [dependencies]
//! clap = { version = "3.2.21", features = [] }
//! ```

fn main() {
    let cmd = clap::Command::new("myprog")
        .subcommand_required(true)
        .arg_required_else_help(true)
        .subcommand(clap::Command::new("foo"))
        .subcommand(clap::Command::new("bar").alias("foo"));
    cmd.get_matches();
}
$ ./clap-4218.rs hello
error: The subcommand 'hello' wasn't recognized

        Did you mean 'help'?

If you believe you received this message in error, try re-running with 'clap-4218_00274e25ccd5548e0be0e496 -- hello'

USAGE:
    clap-4218_00274e25ccd5548e0be0e496 <SUBCOMMAND>

For more information try --help

but you also get

$ ./clap-4218.rs kljkljkljkljkl
error: Found argument 'kljkljkljkljkl' which wasn't expected, or isn't valid in this context

USAGE:
    clap-4218_00274e25ccd5548e0be0e496 <SUBCOMMAND>

For more information try --help

If you update the code to

#!/usr/bin/env -S rust-script --debug

//! ```cargo
//! [dependencies]
//! clap = { version = "3.2.21", features = [] }
//! ```

fn main() {
    let cmd = clap::Command::new("myprog")
        .subcommand_required(true)
        .arg_required_else_help(true)
        .disable_help_flag(true)
        .disable_version_flag(true)
        .subcommand(clap::Command::new("foo"))
        .subcommand(clap::Command::new("bar").alias("foo"));
    cmd.get_matches();
}

you now get

$ ./clap-4218.rs kljkljkljkljkl
error: The subcommand 'kljkljkljkljkl' wasn't recognized

USAGE:
    clap-4218_00274e25ccd5548e0be0e496 <SUBCOMMAND>

For more information try help

@epage
Copy link
Member

epage commented Sep 15, 2022

It sounds like there are two parts to this

  1. "Don't show "argument not found" errors for subcommands when flags are present, especially built-in flags. We should update that check to only care about if positionals are present.
  2. Show help along with the error message (which is blocked on the first part)

help should be displayed with a default message.

I feel like this statements could use more elaboration, especially in a lot of case where showing the help will obscure the error message to the length of output.

I could see including the list of available subcommands in the help output. That could still be long for a bulleted list. We could treat it like possible values and instead do a comma-separated list.

@epage
Copy link
Member

epage commented Sep 15, 2022

Forgot to add, the reason I'm digging more into that assertion is because

  1. We are trying to be more intentional about what knobs we add to help with Binary size could be smaller #1365 and Reduce compilation time #2037
  2. We want to offer a polished experienced out of the box so we should consider if something is good enough to be a default or to be unconditionally on

epage added a commit to epage/clap that referenced this issue Sep 15, 2022
Just having `--help` or `--version` can make us get invalid args instead
of invalid subcommands.   It doesn't make sense to do this unless
positionals are used.  Even then it might not make sense but this is at
least a step in the right direction.

Unsure how I feel about this being backported to clap 3.  It most likely
would be fine?

This was noticed while looking into clap-rs#4218
epage added a commit to epage/clap that referenced this issue Sep 15, 2022
Just having `--help` or `--version` can make us get invalid args instead
of invalid subcommands.   It doesn't make sense to do this unless
positionals are used.  Even then it might not make sense but this is at
least a step in the right direction.

Unsure how I feel about this being backported to clap 3.  It most likely
would be fine?

This was noticed while looking into clap-rs#4218
@ozgunozerk
Copy link
Author

ozgunozerk commented Sep 15, 2022

I feel like this statements could use more elaboration, especially in a lot of case where showing the help will obscure the error message to the length of output.

Totally understandable and I had the same hesitation. If that is the case, following the git way can be helpful.

$ git kjkjkjkl                                                             
git: 'kjkjkjkl' is not a git command. See 'git --help'.

I'm okay with this one as well. This is probably as clean as it can be.

On top of that, appending the error texts (related to unknown/invalid arguments) with See 'appName --help' should not increase the size of the binary, and can be default. I can't imagine a use case where this is not preferred.

epage added a commit to epage/clap that referenced this issue Sep 15, 2022
We do it elsewhere but here it is only distinguished coloring.

Inspired by clap-rs#4218
@epage
Copy link
Member

epage commented Sep 15, 2022

We already suggest checking the help but its after the usage and when its suggesting the help subcommand, it isn't as obvious, so I have a PR to quote it.

@ozgunozerk
Copy link
Author

Alright, thank you very much for the fast responses and actions.
This is better for me. I still think there is room for some improvement in here, but it satisfies me as it is. Feel free to keep this issue closed/open :)

@epage
Copy link
Member

epage commented Sep 16, 2022

I still think there is room for some improvement in here

Feedback is always welcome! As I said, we are aiming to make programs feel polished / high quality out the gate. We recently, did a push on help output.

@epage epage closed this as completed Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants