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

4.0 regression: dashes are not accepted any more #4869

Closed
2 tasks done
est31 opened this issue Apr 28, 2023 · 6 comments
Closed
2 tasks done

4.0 regression: dashes are not accepted any more #4869

est31 opened this issue Apr 28, 2023 · 6 comments
Labels
C-bug Category: Updating dependencies

Comments

@est31
Copy link

est31 commented Apr 28, 2023

Please complete the following tasks

Rust Version

Clap Version

4.0

Minimal reproducible code

use clap::Parser;
use clap::CommandFactory;

fn main() {
    let matches = OptFoo::command().try_get_matches_from(std::env::args()).unwrap();

    println!("matches foo-bar: {:?}", matches.try_get_one::<bool>("foo-bar"));
    println!();
    println!("matches someflag: {:?}", matches.try_get_one::<bool>("someflag"));
}

#[derive(Parser, Debug)]
#[allow(dead_code)]
struct OptFoo {
	#[clap(long, action)]
	foo_bar: bool,
	#[clap(long, action)]
	someflag: bool,
}

Steps to reproduce the bug with the above code

Do cargo run -- --foo-bar --someflag

Actual Behaviour

It prints:

matches foo-bar: Err(UnknownArgument)

matches someflag: Ok(Some(true))

Expected Behaviour

It prints:

matches foo-bar: Ok(Some(true))

matches someflag: Ok(Some(true))

This is what Clap 3.0 did, you can confirm this by running the example with Clap 3.0, i've written it to compile fine there as well. This, this is a regression of 4.0.0.

Additional Context

This caused est31/cargo-udeps#151 where I list all the cargo check flags in a struct that derives Parser, so that they can be passed on to cargo, but where cargo internally does the equivalent of arg_matches.try_get_one::<bool>("all-targets") (it does it via an extension trait on ArgMatches, via a fn flag(&self, name: &str) -> bool function).

Debug Output

No response

@est31 est31 added the C-bug Category: Updating dependencies label Apr 28, 2023
est31 added a commit to est31/cargo-udeps that referenced this issue Apr 28, 2023
These are needed as a workaround for clap-rs/clap#4869 .

Closes #151
@est31 est31 changed the title 4.0 regression: underscores are not accepted any more 4.0 regression: dashes are not accepted any more Apr 28, 2023
@est31
Copy link
Author

est31 commented Apr 28, 2023

A workaround is to add name = "thing-without-underscores-but-dashes" as an attribute, which I did in cargo-udeps. It's not really nice though that you have to go through the entire list and check for names with _ inside.

@epage
Copy link
Member

epage commented Apr 28, 2023

We called this change out in the breaking changes for 4.0 under the "subtle" section, meaning these were the high priority ones to review when upgrading

(derive) Leave Arg::id as verbatim casing, requiring updating of string references to other args like in conflicts_with or requires (#3282)

As for the workaroumd, it is better to use id rather than name. Its an accident that namei is allowed and we'll be removing it in clap v5

@est31
Copy link
Author

est31 commented Apr 28, 2023

Note, that changelog line is very terse and unless you know clap's internals, or you have been pointed towards it, you won't really be able to derive actionable advice from it.

Also, even if it is pointed out, maybe it's not the best idea to have that change, given it makes such hybrid uses harder.

it is better to use id rather than name.

Thanks, will do!

Question: if #3709 is implemented, will I be able to just specify that?

@epage
Copy link
Member

epage commented Apr 28, 2023

Also, even if it is pointed out, maybe it's not the best idea to have that change, given it makes such hybrid uses harder.

The change was made to improve the pure-derive workflow. If you want to declare an arg to conflict with another arg, you now refer to it by the field's name, not what the long name or the value name would be, if it exists. This improves discoverability of the behavior.

Question: if #3709 is implemented, will I be able to just specify that?

We have no plans for controlling naming of id, for the reason given above.

est31 added a commit to est31/cargo-udeps that referenced this issue Apr 28, 2023
@est31
Copy link
Author

est31 commented Apr 29, 2023

Thanks for informing me, and thanks for the advice. I guess it's a tradeoff decision in the end, although of course I'm not happy that the only way to use arguments both in cargo and cargo-udeps is to either make cargo-udeps not use derives, or to put an explicit id attribute everywhere. The bug in cargo-udeps was especially hard to debug because i didn't know if the bug was in cargo, cargo-udeps or clap. I guess most projects have it easier, given that most projects probably only expose a CLI API surface in a fraction of the size that cargo uses.

@est31 est31 closed this as completed Apr 29, 2023
@epage
Copy link
Member

epage commented Apr 29, 2023

For my own projects, I recreate the cargo CLI, rather than reuse it (see clap-cargo). Granted, if you are using cargo-as-a-library for other things then that might make sense. I tend to avoid that as its not a well supported workflow (intentionally at the moment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

2 participants