clap-rs / clap

A full featured, fast Command Line Argument Parser for Rust
docs.rs/clap
Apache License 2.0
13.93k stars 1.03k forks source link

clap_derive casing change stringly type #2475

Open pickfire opened 3 years ago

pickfire commented 3 years ago

Maintainer's notes:

Rust Version

rustc 1.52.0 (88f19c6da 2021-05-03)

Clap Version

3.0.0-beta.2

Minimal reproducible code

use clap::Clap;

#[derive(Debug, Clap)]
struct Opts {
    #[clap(long, conflicts_with = "hello-world")] // I thought it should be hello_world
    one: Option<String>,
    #[clap(long)]
    hello_world: Option<String>,
}

fn main() {
    Opts::parse();
}

It is a bit confusing that conflicts_with only work with the transformed string and the issue is that it only errors on runtime, during compile-time there is no error saying that that is incorrect. Can it be checked during compile-time? It is also confusing that I put hello_world in the struct but I need to use hello-world in the option.

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

thread 'main' panicked at 'Argument or group 'hello_world' specified in 'conflicts_with*' for 'one' does not exist', /home/ivan/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-3.0.0-beta.2/src/build/app/debug_asserts.rs:158:13

Expected Behaviour

First, using hello_world should work instead of hello-world.

Second, it should error during compile-time.

Additional Context

No response

Debug Output

error[E0432]: unresolved import `clap::Clap`
 --> src/main.rs:1:5
  |
1 | use clap::Clap;
  |     ^^^^^^^^^^ no `Clap` in the root

error: cannot determine resolution for the derive macro `Clap`
 --> src/main.rs:3:17
  |
3 | #[derive(Debug, Clap)]
  |                 ^^^^
  |
  = note: import resolution is stuck, try simplifying macro imports

error: cannot find attribute `clap` in this scope
 --> src/main.rs:5:7
  |
5 |     #[clap(long, conflicts_with = "hello_world")] // I thought it should be helllo_world
  |       ^^^^

error: cannot find attribute `clap` in this scope
 --> src/main.rs:7:7
  |
7 |     #[clap(long)]
  |       ^^^^

error[E0599]: no function or associated item named `parse` found for struct `Opts` in the current scope
  --> src/main.rs:12:11
   |
4  | struct Opts {
   | ----------- function or associated item `parse` not found for this
...
12 |     Opts::parse();
   |           ^^^^^ function or associated item not found in `Opts`

error: aborting due to 5 previous errors

Some errors have detailed explanations: E0432, E0599.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `hello`

To learn more, run the command again with --verbose.
epage commented 2 years ago

Agreed on not being in v3

Structopt passes cased_name to Arg::with_name, so I think its "broken" there too. So by leaving this, we don't introduce any more of a migration problem than if we change it now.

pksunkara commented 2 years ago

I am not exactly sure what the expected behaviour should be.

epage commented 2 years ago

I am not exactly sure what the expected behaviour should be.

As a user solely of clap_derive, I do not know of the existence of the "name" field, just what my variables are. I have a variable named hello_world, I assume when working with conflicts_with, I would pass in that variable name.

epage commented 2 years ago

With #3453, I expect rename_all for args will change the value_name and not what is passed to Arg::new

epage commented 2 years ago

Needing to re-think this

rename_all:

So what is

For example, this issue talks about the id becoming morphed but the equivalent of the id for subcommands can't be passed up. If we do this, then it will be inconsistent.

rename_all is generally meant to workaround the language's casing from mismatching with the problem domain so from that perspective, renaming the id can make sense since its happening before it gets to the problem domain.

short casing can be handled naiively (verbatim) but since long casing has to change anyways, we need a way to control that, right?

value name seems the most likely to need a rename_all because its pretty split between SCREAMING_CASE and kebab-case.

With this level of uncertainty, I'm going to hold off for now.

epage commented 2 years ago

3709 proposes more specific rename attributes