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

name implies a user-facing meaning, causing confusion #3335

Closed
2 tasks done
pwinckles opened this issue Jan 24, 2022 · 4 comments · Fixed by #3453
Closed
2 tasks done

name implies a user-facing meaning, causing confusion #3335

pwinckles opened this issue Jan 24, 2022 · 4 comments · Fixed by #3453
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
Milestone

Comments

@pwinckles
Copy link

pwinckles commented Jan 24, 2022

Maintainer's notes:


Please complete the following tasks

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

Rust Version

rustc 1.58.1 (db9d1b20b 2022-01-20)

Clap Version

3.0.10

Minimal reproducible code

use clap::Parser;

#[derive(Debug, Parser)]
pub struct Example {
    #[clap(short = 'S', long, conflicts_with = "version")]
    pub staged: bool,

    #[clap(name = "OBJ_ID")]
    pub object_id: String,

    #[clap(name = "VERSION")]
    pub version: Option<String>,
}

fn main() {
    println!("{:?}", Example::parse());
}

Steps to reproduce the bug with the above code

cargo run -- asdf

Actual Behaviour

Panics:

thread 'main' panicked at 'App clap-example: Argument or group 'version' specified in 'conflicts_with*' for 'staged' does not exist', /home/pwinckles/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-3.0.10/src/build/app/debug_asserts.rs:171:13

Expected Behaviour

It should not panic. I would expect Clap to enforce that the staged flag cannot be set if a version positional arg is provided, and otherwise allow the flag to be present or not.

Additional Context

conflicts_with works as expected when conflicting with another flag. The problem seems to be when conflicting with an optional positional arg.

Debug Output

[            clap::build::app] 	App::_do_parse
[            clap::build::app] 	App::_build
[            clap::build::app] 	App::_propagate:clap-example
[            clap::build::app] 	App::_check_help_and_version
[            clap::build::app] 	App::_check_help_and_version: Removing generated version
[            clap::build::app] 	App::_propagate_global_args:clap-example
[            clap::build::app] 	App::_derive_display_order:clap-example
[clap::build::app::debug_asserts] 	App::_debug_asserts
[clap::build::arg::debug_asserts] 	Arg::_debug_asserts:help
[clap::build::arg::debug_asserts] 	Arg::_debug_asserts:staged
@pwinckles pwinckles added the C-bug Category: Updating dependencies label Jan 24, 2022
@pwinckles pwinckles changed the title conflicts_with on a flag does not work when conflicting with a positional arg conflicts_with on a flag panics when conflicting with a positional arg Jan 24, 2022
@epage
Copy link
Member

epage commented Jan 24, 2022

use clap::Parser;

#[derive(Debug, Parser)]
pub struct Example {
    #[clap(short = 'S', long, conflicts_with = "version")]
    pub staged: bool,

    #[clap(name = "OBJ_ID")]
    pub object_id: String,

    #[clap(name = "VERSION")]
    pub version: Option<String>,
}

fn main() {
    println!("{:?}", Example::parse());
}

should be

use clap::Parser;

#[derive(Debug, Parser)]
pub struct Example {
    #[clap(short = 'S', long, conflicts_with = "VERSION")]
    pub staged: bool,

    #[clap(name = "OBJ_ID")]
    pub object_id: String,

    #[clap(name = "VERSION")]
    pub version: Option<String>,
}

fn main() {
    println!("{:?}", Example::parse());
}

With name, you overrode the ID for referencing the version argument within clap, so you need to update references to that ID as well.

Most likely what you want is value_name instead of name (we do default value_name based on name).

use clap::Parser;

#[derive(Debug, Parser)]
pub struct Example {
    #[clap(short = 'S', long, conflicts_with = "version")]
    pub staged: bool,

    #[clap(value_name = "OBJ_ID")]
    pub object_id: String,

    #[clap(value_name = "VERSION")]
    pub version: Option<String>,
}

fn main() {
    println!("{:?}", Example::parse());
}

This kind of confusion is why I want to rename it from name to id.

@pwinckles
Copy link
Author

😊

Thank you! You are correct that I was confusing name with value_name. Sorry for the trouble.

@epage epage changed the title conflicts_with on a flag panics when conflicting with a positional arg name implies a user-facing meaning, causing confusion Jan 24, 2022
@epage epage added A-builder Area: Builder API S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jan 24, 2022
@epage
Copy link
Member

epage commented Jan 24, 2022

Keeping this open as a use case for why we should explore changing Arg::name.

@pwinckles
Copy link
Author

pwinckles commented Jan 24, 2022

For what it's worth, I experienced the issue because I migrated from clap 2 + structopt, and structopt includes an example like the following in its docs:

    /// File name: only required when `out-type` is set to `file`
    #[structopt(name = "FILE", required_if("out-type", "file"))]
    file_name: Option<String>,

I looked at the docs for clap 3, and I do think it's clear what name does, though I agree that it isn't intuitive without reading the docs.

@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Jan 24, 2022
@epage epage added this to the 4.0 milestone Jan 24, 2022
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
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-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants