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

Validate conflicts_with #2307

Closed
2 tasks done
HalfVoxel opened this issue Jan 22, 2021 · 5 comments
Closed
2 tasks done

Validate conflicts_with #2307

HalfVoxel opened this issue Jan 22, 2021 · 5 comments
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations 💸 $5
Milestone

Comments

@HalfVoxel
Copy link

Make sure you completed the following tasks

Describe your use case

Currently, it can sometimes be tricky to know which name to give to conflicts_with, in particular in combination with structopt.
Take a look at this issue for context: TeXitoi/structopt#459

Clap currently doesn't seem to care if conflicts_with is given a name which isn't even a valid field.

Describe the solution you'd like

Clap validates that the field conflicts_with is given actually exists.

@pksunkara
Copy link
Member

Have you checked this with master? And also please provide a minimal sample code so it's easier for everyone.

@HalfVoxel
Copy link
Author

I have not yet, my testing was done with clap v2.33.3 which is the latest stable version afaik (though I see that there's a 3.0 beta now).

I use structopt so that generates most of the code for me. You can find an example of that in the linked issue.

Here is the code that structopt generates which corresponds to this issue:

let app = app.about("Runs a module");
let app = app.arg(
    ::structopt::clap::Arg::with_name("module-name")
        .takes_value(true)
        .multiple(false)
        .validator(|s| {
            ::std::str::FromStr::from_str(s.as_str())
                .map(|_: String| ())
                .map_err(|e| e.to_string())
        })
        .help("Name of the cmdlet module to run")
        .short("n")
        .long("name"),
);
let app = app.arg(
    ::structopt::clap::Arg::with_name("module-cid")
        .takes_value(true)
        .multiple(false)
        .validator(|s| {
            ::std::str::FromStr::from_str(s.as_str())
                .map(|_: Cid| ())
                .map_err(|e| e.to_string())
        })
        .help("CID of the cmdlet module to run")
        .short("c")
        .long("cid")
        .conflicts_with("module_name"),
);
app.version("0.1.0")

Note that conflicts_with is given the name module_name while the generated name is actually module-name.

@pksunkara
Copy link
Member

Can you please test it on clap master and report back?

@HalfVoxel
Copy link
Author

Hi

I can confirm that this still happens with master.

[dependencies]
clap = { git = "https://github.com/clap-rs/clap.git", rev= "d7f6748" }
use clap::{App, Arg};

fn main() {
    let app = App::new("My Super Program")
        .about("Runs a module")
        .arg(
            Arg::new("module-name")
                .takes_value(true)
                .multiple(false)
                .short('n')
                .long("name"),
        )
        .arg(
            Arg::new("module-cid")
                .takes_value(true)
                .multiple(false)
                .short('c')
                .long("cid")
                .conflicts_with("module_name"),
        )
        .version("0.1.0");

    let matches = app.get_matches();
    println!("{:#?}", matches);
}
 ~/p/e/t/claptest (master) [2]> cargo run --release -- --name=NAME --cid=CID                                   (base)
   Compiling claptest v0.1.0 (/home/arong/projects/embark/test/claptest)
    Finished release [optimized] target(s) in 0.40s
     Running `target/release/claptest --name=NAME --cid=CID`
ArgMatches {
    args: {
        [hash: 49BCF4FC57DA0A4]: MatchedArg {
            occurs: 1,
            ty: CommandLine,
            indices: [
                1,
            ],
            vals: [],
        },
        [hash: 110F162FD0016129]: MatchedArg {
            occurs: 1,
            ty: CommandLine,
            indices: [
                2,
            ],
            vals: [],
        },
    },
    subcommand: None,
}

@pksunkara pksunkara added this to the 3.1 milestone Jan 26, 2021
@pksunkara pksunkara added 💸 $5 C: asserts C-enhancement Category: Raise on the bar on expectations and removed T: new feature labels Jan 26, 2021
@epage epage added A-builder Area: Builder API and removed C: asserts labels Dec 8, 2021
@epage
Copy link
Member

epage commented Dec 13, 2021

This has since been resolved

We have an existing test conflicts_with_invalid_arg but I turned the above example code into a test to double check

#[cfg(debug_assertions)]
#[test]
#[should_panic]
fn non_existent_conflict_asserts() {
    App::new("My Super Program")
        .about("Runs a module")
        .arg(
            Arg::new("module-name")
                .takes_value(true)
                .multiple(false)
                .short('n')
                .long("name"),
        )
        .arg(
            Arg::new("module-cid")
                .takes_value(true)
                .multiple(false)
                .short('c')
                .long("cid")
                .conflicts_with("module_name"),
        )
        .version("0.1.0")
        .debug_assert();
}

And it passes on 3.0.0-rc.4

@epage epage closed this as completed Dec 13, 2021
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 💸 $5
Projects
None yet
Development

No branches or pull requests

3 participants