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

Confusing how to override version or helps behavior #3405

Closed
2 tasks done
bcantrill opened this issue Feb 5, 2022 · 7 comments · Fixed by #4056
Closed
2 tasks done

Confusing how to override version or helps behavior #3405

bcantrill opened this issue Feb 5, 2022 · 7 comments · Fixed by #4056
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@bcantrill
Copy link

bcantrill commented Feb 5, 2022

Maintainer's notes

  • We'd remove the pre-generated help / version arguments
    • Instead we'd add a version and help flag if needed.
    • This makes it so we have one way to customize
  • We'd switch to an Action system where you set an arguments action to Action::Set, Action::Extend, Action::SetTrue, Action::Count, etc. The ones relevant here would be Action::Help and Action::Version.
    • We'd default arguments to Action::Set or Action::SetTrue (depending on other settings) unless they were named version or help for which we'd default their actions to Action::Help and Action::Version
    • We can leverage Actions for also giving users control over short vs long help

Original title: "cannot have an option named "version""

  • See #[clap(global_setting(AppSettings::NoAutoVersion))] for solving that

Please complete the following tasks

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

Rust Version

rustc 1.59.0-nightly (34926f0a1 2021-12-22)

Clap Version

3.0.14

Minimal reproducible code

use clap::Parser;

#[derive(Parser)]                                                                  
#[clap(name = "foobar")]
pub struct Args {
    /// verbose messages
    #[clap(long, short)]
    pub verbose: bool,

    /// version the thing
    #[clap(long, short = 'x')]
    pub version: bool,
}

fn main() {
    let args = Args::parse();
    println!("verbose is {:?}, version is {:?}", args.verbose, args.version);
}

Steps to reproduce the bug with the above code

cargo run -- -x or cargo run -- --version

Actual Behaviour

foobar

Expected Behaviour

verbose is false, version is true

Additional Context

We need to have a --version option that does more than the Clap default. Currently, Clap special-cases the string version, and seemingly overrides its behavior (changing the option name to anything else results in the expected behavior). This seems to be a version manifestation of behavior similar to #3403.

Debug Output

[            clap::build::app] 	App::_do_parse
[            clap::build::app] 	App::_build
[            clap::build::app] 	App::_propagate:foobar
[            clap::build::app] 	App::_check_help_and_version
[            clap::build::app] 	App::_check_help_and_version: Removing generated version
[            clap::build::app] 	App::_propagate_global_args:foobar
[            clap::build::app] 	App::_derive_display_order:foobar
[clap::build::app::debug_asserts] 	App::_debug_asserts
[clap::build::arg::debug_asserts] 	Arg::_debug_asserts:help
[clap::build::arg::debug_asserts] 	Arg::_debug_asserts:verbose
[clap::build::arg::debug_asserts] 	Arg::_debug_asserts:version
[clap::build::app::debug_asserts] 	App::_verify_positionals
[         clap::parse::parser] 	Parser::get_matches_with
[         clap::parse::parser] 	Parser::get_matches_with: Begin parsing 'RawOsStr("--version")' ([45, 45, 118, 101, 114, 115, 105, 111, 110])
[         clap::parse::parser] 	Parser::get_matches_with: Positional counter...1
[         clap::parse::parser] 	Parser::get_matches_with: Low index multiples...false
[         clap::parse::parser] 	Parser::possible_subcommand: arg=RawOsStr("--version")
[         clap::parse::parser] 	Parser::get_matches_with: sc=None
[         clap::parse::parser] 	Parser::parse_long_arg
[         clap::parse::parser] 	Parser::parse_long_arg: cur_idx:=1
[         clap::parse::parser] 	Parser::parse_long_arg: Does it contain '='...
[         clap::parse::parser] 	No
[         clap::parse::parser] 	Parser::parse_long_arg: Found valid opt or flag '--version'
[         clap::parse::parser] 	Parser::check_for_help_and_version_str
[         clap::parse::parser] 	Parser::check_for_help_and_version_str: Checking if --RawOsStr("version") is help or version...
[         clap::parse::parser] 	Version
[         clap::parse::parser] 	Parser::get_matches_with: After parse_long_arg VersionFlag
[         clap::parse::parser] 	Parser::version_err
[            clap::build::app] 	App::_render_version
[            clap::build::app] 	App::color: Color setting...
[            clap::build::app] 	Auto
foobar 

The debug output is very helpful about indicating what it's doing, it's just that this is behavior that we do not want.

@bcantrill bcantrill added the C-bug Category: Updating dependencies label Feb 5, 2022
@bcantrill
Copy link
Author

For anyone else that runs across this: you can disable this behavior by adding this:

#[clap(global_setting(AppSettings::NoAutoVersion))]

Even though this behavior can be disabled, I would argue that the existing behavior violates the Principle of Least Surprise -- not least because it exits without providing any real indicator as to why. (I would also note that StructOpt seems to have had no_version that seems to not have made the transition to Clap 3.)

@epage
Copy link
Member

epage commented Feb 5, 2022

The intent of the current behavior is to allow you to custmize the flag while still getting the same behavior. We also offer customization by, behind the scenes, pre-creating some flags for you to modify with mut_arg.

That said, this is still not great.

My current thought

  • We'd remove the pre-generated help / version arguments
    • Instead we'd add a version and help flag if needed.
    • This makes it so we have one way to customize
  • We'd switch to an Action system where you set an arguments action to Action::Set, Action::Extend, Action::SetTrue, Action::Count, etc. The ones relevant here would be Action::Help and Action::Version.
    • We'd default arguments to Action::Set or Action::SetTrue (depending on other settings) unless they were named version or help for which we'd default their actions to Action::Help and Action::Version

The main concern is the discoverability of the defaults but I think this moves us in the right direction.

Going to rename this Issue to reflect the confusion over the existing API.

@epage epage changed the title cannot have an option named "version" Confusing how to override version or helps behavior Feb 5, 2022
@epage epage added A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change. and removed C-bug Category: Updating dependencies labels Feb 5, 2022
@bcantrill
Copy link
Author

Totally agreed about the change in issue name (as one might guess, I had not yet discovered NoAutoVersion when I filed it!) and about the course of action. Thank you -- and thank you for your work on Clap which has been a pleasure to use!

epage added a commit to epage/clap that referenced this issue Feb 9, 2022
For now, we are going to provide a better debug assert in this case.
Resolving clap-rs#3405 is the better long term route.

Fixes clap-rs#3403
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
Like was said in clap-rs#2435, this is what people would expect.

While we should note this in a compatibility section in the changelog, I
do not consider this a breaking change since we should be free to adjust
the help output as needed.  We are cautious when people might build up
their own content around it (like clap-rs#3312) but apps should already handle
this with `--help` so this shouldn't be a major change.

We aren't offering a way for people to disable this, assuming people
won't need to.  Longer term, we are looking at support "Actions" (clap-rs#3405)
and expect those to help customize the flags.  We'll need something
similar for the `help` subcommand.

Fixes clap-rs#3440
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
Like was said in clap-rs#2435, this is what people would expect.

While we should note this in a compatibility section in the changelog, I
do not consider this a breaking change since we should be free to adjust
the help output as needed.  We are cautious when people might build up
their own content around it (like clap-rs#3312) but apps should already handle
this with `--help` so this shouldn't be a major change.

We aren't offering a way for people to disable this, assuming people
won't need to.  Longer term, we are looking at support "Actions" (clap-rs#3405)
and expect those to help customize the flags.  We'll need something
similar for the `help` subcommand.

Fixes clap-rs#3440
@epage
Copy link
Member

epage commented May 9, 2022

The change in pre-generated help and version cannot be done until clap 4. We have the unstable-v4 flag but I worry this would be too invasive of a change for that flag. Might still be worth experimenting with.

I'd like to get Actions in for clap v3 users, independent of whats going on for clap 4. In that case, we should probably do it first so we don't couple the design to any changes for v4.

Python has

  • store: overwrite current entry
  • store_const: overwrite with a hardcoded value
  • store_true: overwrite with true
  • store_false: overwrite with false
  • append
  • append_const
  • count: increment value stored
  • help: display help
    • We'd probably want Action::Help, Action::HelpShort, and Action::HelpLong
  • version: display version
    • Ditto about short and long
  • extend
  • custom ones by sub-classingAction

Python also includes a non-native action, BooleanOptionalAction which implicitly creates a --no- variant and stores true/false.

Items like store_true and count can't be done until #2683.

#815 could be implemented like BooleanOptionalAction using aliases without having to get into #3146. We'd need to pass to the action what flag or arg was used. This then helps with other cases like Help.

So the question is what can be supported without blocking on #2683 and is it worth going ahead without it?

Without inheritance exposed, removing of pre-generated help, or support for #2683, I'm thinking there isn't a use case that makes this worth it enough. Going to focus on those other areas first.

@epage
Copy link
Member

epage commented May 26, 2022

To implement this sanely, I'm going to need to do a major parser refactor

Random notes

  • With multiple values
    • Attached values (e.g. after =) disallow any further values
    • A delmited value (e.g. bar,baz) disallows any further values (so foo bar,baz is legal)

@epage
Copy link
Member

epage commented Jun 1, 2022

Working on integrating actions (#3774, #3775, #3777) into the derive API and hitting a hurdle.

  • Users are currently defining whatever type they want for storing occurrences (i8, u32, usize, etc)
  • We'll define the value parser and get_one::<T> based on that type
  • As-implemented, Count only supports u64 for the stored type
  • We want to support users providing their own value parsers for Count so they can do custom validation (range restrictions)

How do we support the user using whatever type they want like they can today?

Ideas

  • When no parser is specified, we could ask the action for a parser and then fallback to value_types!.
    • but we won't know what type to use with get_one
  • We bake counting directly into ValueParser so we don't have to know about it
  • Count supports multiple storage types, one for each integer type
  • We key off of the type name to do custom logic for Count
    • This runs into issues with whether the user typed it in the way we expect (is it fully qualified?)
  • We make each action a distinct type with Action just being a "box" for them (internally, just an enum). We use deref specialization on the passed-in type to customize Counts behavior
    • If someone uses Action rather than CountAction, then we won't be able to help them
    • Really wish types could have associated types

ytmimi added a commit to ytmimi/rustfmt that referenced this issue Jun 21, 2022
Fixes 5395

In PR 5239 we switched from using `structopt` to `clap`. It seems that
the default behavior for `clap` is to override the `--version` flag,
which prevented our custom version display code from running.

The fix as outlined in clap-rs/clap#3405 was
to set `#[clap(global_setting(AppSettings::NoAutoVersion))]` to prevent
clap from setting its own default behavior for the `--version` flag.
calebcartwright pushed a commit to rust-lang/rustfmt that referenced this issue Jun 22, 2022
Fixes 5395

In PR 5239 we switched from using `structopt` to `clap`. It seems that
the default behavior for `clap` is to override the `--version` flag,
which prevented our custom version display code from running.

The fix as outlined in clap-rs/clap#3405 was
to set `#[clap(global_setting(AppSettings::NoAutoVersion))]` to prevent
clap from setting its own default behavior for the `--version` flag.
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 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
@thewh1teagle
Copy link

For anyone who look to override default version:

#[derive(Parser, Debug)]
#[command(author, version, about, long_about = None, disable_version_flag = true)]
struct Args {
  ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants