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

Derive does not automatically capture multiple parameters #3248

Closed
2 tasks done
chrisduerr opened this issue Jan 3, 2022 · 7 comments
Closed
2 tasks done

Derive does not automatically capture multiple parameters #3248

chrisduerr opened this issue Jan 3, 2022 · 7 comments
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-wont-fix Status: Closed as there is no plan to fix this

Comments

@chrisduerr
Copy link

Please complete the following tasks

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

Clap Version

3

Describe your use case

With structopt when defining a type as a Vec<T>, it would automatically capture multiple values. Generally I'd say this makes sense because rarely would one want to specify an array differently on the command line. However the new clap derive does not do this, which I'd assume might also cause some regressions for people migrating that aren't careful about their CLI.

While it is currently possible to just specify multiple_values = true, it seems unnecessary.

Describe the solution you'd like

A field annotated with #[clap(...)] of the type Vec<T> for automatic derive should imply multiple_values = true.

Alternatives, if applicable

No response

Additional Context

No response

@chrisduerr chrisduerr added the C-enhancement Category: Raise on the bar on expectations label Jan 3, 2022
@chrisduerr
Copy link
Author

Looking at what seems to be the relevant code (https://github.com/clap-rs/clap/blob/master/clap_derive/src/derives/args.rs#L311-L320), it might be intentional that a vec only implies multiple_occurences, not necessarily multiple_values?

I'd still really like to see this because I think only having multiple_occurences mostly makes sense for flags, but for a Vec<T> which actually takes a string as an argument it should pretty much always be convenient to the user to not have to specify --option again just to pass another thing when --option x y z would come at probably no disadvantage?

@epage
Copy link
Member

epage commented Jan 4, 2022

It was intentional that we deviated from structopt. See #1772

Most applications either take

  • Multiple occurrences (cargo build --package foo --package bar)
  • Multiple delimited values (cargo build --features "derive wrap_help")
  • Or both (cargo build --features "derive wrap_help" --features suggestions)

Making multiple occurrences by default

  • Is a solid base to then build up other behavior (usually if you do delimited values, you also do multiple occurrences)
  • Is less surprising since it is what most developers are expecting (personal anecdote: I wrote many applications with Vec in structopt thinking I was getting multiple occurrences only but I was also getting multiple values)
  • Has fewer gotchas (see Arg::multiple_values)

For the cases when users want the older / structopt experience, they can still easily get it by opting in with multiple_values(true).

@epage epage closed this as completed Jan 4, 2022
@epage
Copy link
Member

epage commented Jan 4, 2022

btw this was called out in the breaking changes from structopt: https://github.com/clap-rs/clap/blob/master/CHANGELOG.md#breaking-changes

@chrisduerr
Copy link
Author

I can't say I agree with that choice, but I suppose multiple_values can still be specified manually.

@epage epage added A-derive Area: #[derive]` macro API S-wont-fix Status: Closed as there is no plan to fix this labels Jan 11, 2022
@tdudz
Copy link

tdudz commented Jan 11, 2022

@chrisduerr is there a way i can use derive to get clap v3 to capture multiple values?

@chrisduerr
Copy link
Author

I'm a bit confused because this issue has multiple comments outlining how to do this already.

But just to make it 100% clear: https://github.com/alacritty/alacritty/blob/master/alacritty/src/cli.rs#L61

@epage
Copy link
Member

epage commented Jan 12, 2022

You can also look at our tests for ideas of what can be done
https://github.com/clap-rs/clap/blob/master/tests/derive/options.rs#L297

dfinity-bot pushed a commit to dfinity/ic that referenced this issue Apr 7, 2022
[rc] Bump clap version v2

The last MR caused issues in the release pipeline, specifically with `ic-admin`. The CLI bump caused an unexpected behavior change. I located the issue and decided to try again 😄 

The breaking change that caused issues is the following:\
The new version changed the default for passing multiple values and Clap now complains if multiple values are passed to an argument. It affects CLI options that are `Vec<_>`. In the CI it was the `--replacement-nodes` that is a `Vec<_>`.

In the previous version you could do:
`ic-admin --values v1 v2 v3`  -> values: `Some("v1","v2","v3")` \
In the current version (v3) they changed the default behavior to:\
`ic-admin --values v1 --values v2 --values v3` or `#derive[clap(multiple_values(true))]`

To keep the old behaviour I added `#derive[clap(multiple_values(true))]` to the arguments that use a `Vec<_>`. It should now behave the same way as with the older `clap` version.

Ref:
- [Similar issue](clap-rs/clap#3283)
- [Similar issue](clap-rs/clap#3248) 
- [Clap changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md#breaking-changes)

[Issue](https://dfinity.slack.com/archives/C024WJPPX5W/p1648646423132219?thread_ts=1648605012.761379&cid=C024WJPPX5W): 
```
Start: Wed Mar 30 13:08:14 UTC 2022
error: Found argument '4evoo-kgi22-4rxri-rtibv-ny4u4-qy6bw-7ivmv-vw3iw-ij3jy-dxrof-tae' which wasn't expected, or isn't valid in this context
USAGE:
    ic-admin --nns-url <NNS_URL> propose-to-update-recovery-cup [OPTIONS] --subnet <SUBNET> --height <HEIGHT> --time-ns <TIME_NS> --state-hash <STATE_HASH>
For more information try --help
```
Next steps:
-  remove `structopt` since it now merged with clap v3 

See merge request dfinity-lab/public/ic!4095
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 S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

No branches or pull requests

3 participants