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

clap_derive casing change stringly type #2475

Open
2 tasks done
pickfire opened this issue May 9, 2021 · 6 comments
Open
2 tasks done

clap_derive casing change stringly type #2475

pickfire opened this issue May 9, 2021 · 6 comments
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. 💸 $5 S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Milestone

Comments

@pickfire
Copy link
Contributor

pickfire commented May 9, 2021

Maintainer's notes:


Please complete the following tasks

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

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.
@pickfire pickfire added the C-bug Category: Updating dependencies label May 9, 2021
@pksunkara pksunkara added 💸 $5 C: asserts C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much and removed C-bug Category: Updating dependencies labels May 9, 2021
@pksunkara pksunkara added this to the 3.0 milestone May 9, 2021
@pksunkara pksunkara removed this from the 3.0 milestone May 26, 2021
@epage
Copy link
Member

epage commented Oct 4, 2021

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
Copy link
Member

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

@epage
Copy link
Member

epage commented Oct 9, 2021

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 epage added this to the 4.0 milestone Oct 9, 2021
@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Oct 19, 2021
@epage epage added A-derive Area: #[derive]` macro API and removed C: asserts labels Dec 8, 2021
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
This adjusts names.  Adjusting the derive naming (and re-naming) is left
to clap-rs#2475.

Fixes clap-rs#3335
@epage
Copy link
Member

epage commented Feb 15, 2022

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

@epage
Copy link
Member

epage commented May 6, 2022

Needing to re-think this

rename_all:

  • Args:
    • Changes the id, long, and short
    • value_name is hard coded to screaming-case
    • Without rename_all, we already apply a default casing for id, long, and short
  • Subcommands:
    • Changes the name which serves as both the user value and the id
  • ArgEnums:
    • Changes the name

So what is

  • The most commonly wanted behavior for rename_all
  • The most obvious behavior for rename_all

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 epage removed this from the 4.0 milestone May 6, 2022
@epage epage added S-waiting-on-decision Status: Waiting on a go/no-go before implementing and removed E-easy Call for participation: Experience needed to fix: Easy / not much labels May 6, 2022
@epage
Copy link
Member

epage commented Jun 8, 2022

#3709 proposes more specific rename attributes

@epage epage added this to the 5.0 milestone Jun 8, 2022
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-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. 💸 $5 S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

3 participants