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

Adding help alias modifies help command argument rendering? #4033

Closed
SUPERCILEX opened this issue Aug 6, 2022 · 5 comments · Fixed by #4056
Closed

Adding help alias modifies help command argument rendering? #4033

SUPERCILEX opened this issue Aug 6, 2022 · 5 comments · Fixed by #4056

Comments

@SUPERCILEX
Copy link
Contributor

Adding #[clap(mut_arg("help", |arg| arg.short_alias('?')))] changes the way the help command is rendered. See this diff: SUPERCILEX/ftzz@6daf6aa. Seems like a bug that adding the alias makes the help command show up inside itself. Or not (as long as its consistent I don't mind).

@epage
Copy link
Member

epage commented Aug 6, 2022

Could you follow the bug report template? Being able to have small reproduction cases, debug traces before/after, etc are a big help to triaging this.

@SUPERCILEX
Copy link
Contributor Author

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

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

use clap::{Parser, Subcommand};

#[derive(Parser)]
#[clap(mut_arg("help", |arg| arg.short_alias('?')))]
struct Cli {
  #[clap(subcommand)]
  command: Commands,
}

#[derive(Subcommand)]
enum Commands { Foo }

fn main() {
    let mut _command = Cli::parse();
}

Run:

$ ./foo.rs help help
foo_bcd390fa607fcb4d15f74b68-help 
Print this message or the help of the given subcommand(s)

USAGE:
    foo_bcd390fa607fcb4d15f74b68 help [SUBCOMMAND]...

ARGS:
    <SUBCOMMAND>...    The subcommand whose help message to display

OPTIONS:
    -h, --help    Print help information

@SUPERCILEX
Copy link
Contributor Author

Note that $ ./foo.rs help -h doesn't work so those options shouldn't show up.

@epage
Copy link
Member

epage commented Aug 9, 2022

FYI there are still parts of the template missing, like debug output.

My first guess is that this is because we do this

            help_subcmd = help_subcmd
                .setting(AppSettings::DisableHelpFlag)
                .unset_global_setting(AppSettings::PropagateVersion);

but when we _build_self, we overwrite settings with global settings. We need to also set the global setting to disable it.

@epage
Copy link
Member

epage commented Aug 10, 2022

Correction, the challenge is with

        {
            let generated_help_pos = self
                .args
                .args()
                .position(|x| x.id == Id::help_hash() && x.provider == ArgProvider::Generated);

            if let Some(index) = generated_help_pos {
                debug!("Command::_check_help_and_version: Removing generated help");
                self.args.remove(index);
            }

So if a user modifies it, it becomes GeneratedMutated.
The comments suggest that that is meant to deal with another problem,.

epage added a commit to epage/clap that referenced this issue Aug 10, 2022
epage added a commit to epage/clap that referenced this issue Aug 11, 2022
Before we introduced actions, it required specific setups to engage with
claps version and help printing.  With actions making that more
explicit, we don't get as much benefit from our multiple, obscure, ways
of users customizing help

Before
- Modify existing help or version with `mut_arg` which would
  automatically be pushed down the command tree like `global(true)`
- Create an new help or version and have it treated as if it was the
  built-in on (I think)
- Use the same flags as built-in and have the built-in flags
  automatically disabled
- Users could explicitly disable the built-in functionality and do what
  they want

Now
- `mut_arg` no longer works as we define help and version flags at the
  end
- If someone defines a flag that overlaps with the built-ins by id,
  long, or short, a debug assert will tell them to explicitly disable
  the built-in
- Any customization has to be done by a user providing their own.  To
  propagate through the command tree, they need to set `global(true)`.

Benefits
- Hopefully, this makes it less confusing on how to override help
  behavior.  Someone creates an arg and we then tell them how to disable
  the built-in
- This greatly simplifies the arg handling by pushing more
  responsibility onto the developer in what are hopefully just corner
  cases
- This removes about 1Kb from .text

Fixes clap-rs#3405
Fixes clap-rs#4033
epage added a commit to epage/clap that referenced this issue Aug 11, 2022
Before we introduced actions, it required specific setups to engage with
claps version and help printing.  With actions making that more
explicit, we don't get as much benefit from our multiple, obscure, ways
of users customizing help

Before
- Modify existing help or version with `mut_arg` which would
  automatically be pushed down the command tree like `global(true)`
- Create an new help or version and have it treated as if it was the
  built-in on (I think)
- Use the same flags as built-in and have the built-in flags
  automatically disabled
- Users could explicitly disable the built-in functionality and do what
  they want

Now
- `mut_arg` no longer works as we define help and version flags at the
  end
- If someone defines a flag that overlaps with the built-ins by id,
  long, or short, a debug assert will tell them to explicitly disable
  the built-in
- Any customization has to be done by a user providing their own.  To
  propagate through the command tree, they need to set `global(true)`.

Benefits
- Hopefully, this makes it less confusing on how to override help
  behavior.  Someone creates an arg and we then tell them how to disable
  the built-in
- This greatly simplifies the arg handling by pushing more
  responsibility onto the developer in what are hopefully just corner
  cases
- This removes about 1Kb from .text

Fixes clap-rs#3405
Fixes clap-rs#4033
@epage epage closed this as completed in f70ebe8 Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants