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

value_name appends, rather than overrides #2634

Closed
2 tasks done
epage opened this issue Jul 28, 2021 · 2 comments · Fixed by #2690
Closed
2 tasks done

value_name appends, rather than overrides #2634

epage opened this issue Jul 28, 2021 · 2 comments · Fixed by #2690
Labels
C-bug Category: Updating dependencies
Milestone

Comments

@epage
Copy link
Member

epage commented Jul 28, 2021

Please complete the following tasks

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

Rust Version

rustc 1.53.0 (53cb7b09b 2021-06-17)

Clap Version

3.0.0-beta.2

Minimal reproducible code

#[clap(
number_of_values = 1,
long = "using",
value_name = "pm",
value_name = "foo",
)]
using: Option,

Steps to reproduce the bug with the above code

Run it

Actual Behaviour

  • pm and foo show up in --help
  • Clap errors if there aren't two arguments

Expected Behaviour

  • Last one wins

Additional Context

Split out of #2632. Depending on what solution we go with #2633 could be simplified

There is a separate value_names that is documented for providing multiple. That also appends

This is either a bug in the documentation or in the code.

Related: documentation also says value_name is cosmetic only but value_name, under certain situations, implies number_of_values.

Most of the time, people will not run into this. Its when programmatically creating an argument, like in the derive macro.

Debug Output

No response

@epage epage added the C-bug Category: Updating dependencies label Jul 28, 2021
@pksunkara pksunkara added this to the 3.0 milestone Jul 30, 2021
@epage
Copy link
Member Author

epage commented Aug 13, 2021

@pksunkara any opinion on whether this is a documentation or implementation bug?

@pksunkara
Copy link
Member

Related: documentation also says value_name is cosmetic only

Docs say that the name is cosmetic. Not the function.

This is either a bug in the documentation or in the code.

While most of the other methods just extend existing data, default_value and default_values which is probably the most similar to these functions override. That is probably what we want to do here too.

epage added a commit to epage/clap that referenced this issue Aug 13, 2021
Instead they should behave like `default_value`/`default_values`.

In implementingt this, I didn't see any reason to be using a `VecMap`.
In fact, this helped simplify the code / make intent clearer.

With this, we are also able to simplify the derive macro work from clap-rs#2633.

Fixes clap-rs#2634

BREAKING CHANGE: `value_name`/`value_names` always overwrite, rather
than append.  We expect the impact to be minimal.
epage added a commit to epage/clap that referenced this issue Aug 13, 2021
Instead they should behave like `default_value`/`default_values`.

In implementingt this, I didn't see any reason to be using a `VecMap`.
In fact, this helped simplify the code / make intent clearer.

With this, we are also able to simplify the derive macro work from clap-rs#2633.

Fixes clap-rs#2634

BREAKING CHANGE: `value_name`/`value_names` always overwrite, rather
than append.  We expect the impact to be minimal.
epage added a commit to epage/clap that referenced this issue Aug 13, 2021
Instead they should behave like `default_value`/`default_values`.

In implementingt this, I didn't see any reason to be using a `VecMap`.
In fact, this helped simplify the code / make intent clearer.

With this, we are also able to simplify the derive macro work from clap-rs#2633.

Fixes clap-rs#2634

BREAKING CHANGE: `value_name`/`value_names` always overwrite, rather
than append.  We expect the impact to be minimal.
epage added a commit to epage/clap that referenced this issue Aug 13, 2021
Instead they should behave like `default_value`/`default_values`.

In implementingt this, I didn't see any reason to be using a `VecMap`.
In fact, this helped simplify the code / make intent clearer.

With this, we are also able to simplify the derive macro work from clap-rs#2633.

Fixes clap-rs#2634

BREAKING CHANGE: `value_name`/`value_names` always overwrite, rather
than append.  We expect the impact to be minimal.
epage added a commit to epage/clap that referenced this issue Aug 13, 2021
Instead they should behave like `default_value`/`default_values`.

In implementingt this, I didn't see any reason to be using a `VecMap`.
In fact, this helped simplify the code / make intent clearer.

With this, we are also able to simplify the derive macro work from clap-rs#2633.

Fixes clap-rs#2634

BREAKING CHANGE: `value_name`/`value_names` always overwrite, rather
than append.  We expect the impact to be minimal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants