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

Derive should only show help when argc==1 rather than missing subcommand #3280

Closed
2 tasks done
epage opened this issue Jan 11, 2022 · 4 comments · Fixed by #3969
Closed
2 tasks done

Derive should only show help when argc==1 rather than missing subcommand #3280

epage opened this issue Jan 11, 2022 · 4 comments · Fixed by #3969
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much
Milestone

Comments

@epage
Copy link
Member

epage commented Jan 11, 2022

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.57.0 (f1edd0429 2021-11-29)

Clap Version

v3.0.6

Minimal reproducible code

use clap::{AppSettings, Parser, Subcommand};

#[derive(Parser)]
#[clap(author, version, about, long_about = None)]
#[clap(global_setting(AppSettings::PropagateVersion))]
#[clap(global_setting(AppSettings::UseLongFormatForHelpSubcommand))]
struct Cli {
    #[clap(short, long, parse(from_occurrences), global = true)]
    verbose: usize,

    #[clap(subcommand)]
    command: Commands,
}

#[derive(Subcommand)]
enum Commands {
    /// Adds files to myapp
    Add { name: Option<String> },
}

fn main() {
    let cli = Cli::parse();

    // You can check for the existence of subcommands, and if found use their
    // matches just as you would the top level app
    match &cli.command {
        Commands::Add { name } => {
            println!("'myapp add' was used, name is: {:?}", name)
        }
    }
}

Steps to reproduce the bug with the above code

$ cargo run -- -v

Actual Behaviour

$ cargo run -- -v
   Compiling test-clap v0.1.0 (/home/epage/src/personal/test-clap)
    Finished dev [unoptimized + debuginfo] target(s) in 0.70s
     Running `target/debug/test-clap -v`
test-clap 0.1.0
Me
From Cargo.toml

USAGE:
    test-clap [OPTIONS] <SUBCOMMAND>

OPTIONS:
    -h, --help       Print help information
    -v, --verbose    
    -V, --version    Print version information

SUBCOMMANDS:
    add     Adds files to myapp
    help    Print this message or the help of the given subcommand(s)

Expected Behaviour

$ cargo run -- -v
   Compiling test-clap v0.1.0 (/home/epage/src/personal/test-clap)
    Finished dev [unoptimized + debuginfo] target(s) in 0.67s
     Running `target/debug/test-clap -v`
error: 'test-clap' requires a subcommand, but one was not provided

USAGE:
    test-clap [OPTIONS] <SUBCOMMAND>

For more information try --help

Additional Context

To reproduce the desired behavior:

#[clap(setting(AppSettings::SubcommandRequired))]
#[clap(setting(AppSettings::ArgRequiredElseHelp))]
#[clap(unset_setting(AppSettings::SubcommandRequiredElseHelp))]

This was inspired by a conversation in #2513

In my mind, showing of help should be reserved for when the user has taken no action but SubcommandRequired allows the user to pass some arguments. In those cases, we should be showing a more specific error message than a long, generic message that takes the user some work to figure out what the next steps are.

Debug Output

No response

@epage epage added C-bug Category: Updating dependencies S-waiting-on-decision Status: Waiting on a go/no-go before implementing A-builder Area: Builder API A-derive Area: #[derive]` macro API labels Jan 11, 2022
@epage
Copy link
Member Author

epage commented Jan 11, 2022

I do not think this dramatically breaks user expectations that this would be considered a breaking change and can happen anytime,

@epage
Copy link
Member Author

epage commented Jan 11, 2022

Personally, I think we should deprecate and remove SubcommandRequiredElseHelp in favor of SubcommandRequired | ArgRequiredElseHelp (resolving this issue doesn't need to be blocked on a decision about this but we will then need to create a separate issue). The behavior seems too close to justify having both. We are working to find ways to simplify clap to reduce API overhead (didn't know you could get this better behavior and actually thought this was what I was getting) and code bloat from too much built-in customization.

29ca7b2 and #133 don't include details as to why we have both.

@epage epage added E-easy Call for participation: Experience needed to fix: Easy / not much and removed S-waiting-on-decision Status: Waiting on a go/no-go before implementing A-builder Area: Builder API labels Jan 11, 2022
@epage
Copy link
Member Author

epage commented Jan 11, 2022

I guess a question for 4.0 is if we should stop including ArgRequiredElseHelp by default and require users to opt-in. I'm mixed on that.

@epage
Copy link
Member Author

epage commented Feb 11, 2022

Experimented with this recently. SubcommandRequired has higher precedence than ArgRequiredElseHelp. We'd need to swap these.

epage added a commit to epage/clap that referenced this issue Feb 11, 2022
Showing help on `$ cmd` makes sense but when an arg is present but not
subcommand, that suggests the user missed something, so we should show a
more specific error message.

Compatibility note: Settings we apply for subcommands have changed.

Fixes clap-rs#3280
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
With clap-rs#3280 resolved where we can use `SubcommandRequired |
ArgRequiredElseHelp`, this setting offers little value but requires we
track required subcommands with two different settings.  Deprecating as
the cost is not worth the benefit anymore.
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
Now that we can use `SubcommandRequired |
ArgRequiredElseHelp`, this setting offers little value but requires we
track required subcommands with two different settings.  Deprecating as
the cost is not worth the benefit anymore.

Issue clap-rs#3280 will see the derive updated
@epage epage added this to the 3.x milestone May 9, 2022
@epage epage modified the milestones: 3.x, 4.0 Jun 8, 2022
epage added a commit to epage/clap that referenced this issue Jul 22, 2022
This also let us remove the deprecated attribute

Fixes clap-rs#3280
epage added a commit to epage/clap that referenced this issue Jul 22, 2022
This also let us remove the deprecated attribute

Fixes clap-rs#3280
epage added a commit to epage/clap that referenced this issue Jul 22, 2022
This also let us remove the deprecated attribute

Fixes clap-rs#3280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much
Projects
None yet
1 participant