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

rename_all on a Subcommand applies to variant-struct fields #3282

Closed
2 tasks done
epage opened this issue Jan 11, 2022 · 2 comments · Fixed by #3708
Closed
2 tasks done

rename_all on a Subcommand applies to variant-struct fields #3282

epage opened this issue Jan 11, 2022 · 2 comments · Fixed by #3708
Labels
A-derive C-bug E-medium M-breaking-change
Milestone

Comments

@epage
Copy link
Member

@epage epage commented Jan 11, 2022

Please complete the following tasks

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

Rust Version

rustc 1.57.0 (f1edd0429 2021-11-29)

Clap Version

v3.0.6

Minimal reproducible code

use clap::{Parser, Subcommand};

#[derive(Parser, Debug)]
#[clap(author, version, about, long_about = None)]
struct Cli {
    #[clap(long)]
    out_dir: Option<String>,

    #[clap(subcommand)]
    command: Commands,
}

#[derive(Subcommand, Debug)]
#[clap(rename_all = "snake_case")]
enum Commands {
    /// Adds files to myapp
    Add {
        #[clap(long)]
        out_dir: Option<String>,
    },
}

fn main() {
    let cli = Cli::parse();
    dbg!(cli);
}

Steps to reproduce the bug with the above code

$ cargo run -- add --help

Actual Behaviour

test-clap-add 
Adds files to myapp

USAGE:
    test-clap add [OPTIONS]

OPTIONS:
    -h, --help                 Print help information
        --out_dir <OUT_DIR>    

Expected Behaviour

test-clap-add 
Adds files to myapp

USAGE:
    test-clap add [OPTIONS]

OPTIONS:
    -h, --help                 Print help information
        --out-dir <OUT_DIR>    

Additional Context

Found via rust-lang/rustc-perf#1142

Debug Output

No response

@epage epage added C-bug E-medium A-derive labels Jan 11, 2022
@epage epage added the M-breaking-change label Feb 9, 2022
@epage epage added this to the 4.0 milestone Feb 9, 2022
@epage
Copy link
Member Author

@epage epage commented May 6, 2022

Since clap specifically tests for this behavior, I got worried about what precedent is set for this so I tested serde

#!/usr/bin/env rust-script

//! ```cargo
//! [dependencies]
//! serde = { version = "1", features = ["derive"] }
//! serde_json = "1"
//! ```

#[derive(serde::Serialize, serde::Deserialize, Debug)]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
enum Args {
    One,
    Two { field: String },
}

fn main() {
    let args = Args::One;
    let json = serde_json::to_string(&args).unwrap();
    println!("{}", json);
    let args = Args::Two {
        field: "value".into(),
    };
    let json = serde_json::to_string(&args).unwrap();
    println!("{}", json);
}
$ ./clap-3282.rs
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `/home/epage/src/personal/cargo-script-mvs/target/debug/rust-script ./clap-3282.rs`
"ONE"
{"TWO":{"field":"value"}}

Looks like serde does not apply rename_all recursively which makes sense because variants and fields are very different and can have different naming needs.

@epage
Copy link
Member Author

@epage epage commented May 8, 2022

Thinking about it more, I think with it being an id, it is fine being skipped by rename_all, especially since we set value_name. However, I could see us providing more specialized renames, rename_flags, rename_commands, rename_values

epage added a commit to epage/clap that referenced this issue May 9, 2022
This will make it easier to reference arguments with different
attributes.

Fixes clap-rs#3282
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive C-bug E-medium M-breaking-change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant