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.

Sign up for GitHub

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

Jump to bottom

Make sure positionals have MultipleOccurrences set rather than MultipleValues #2692

Closed
pksunkara opened this issue Aug 13, 2021 · 9 comments
Closed

Make sure positionals have MultipleOccurrences set rather than MultipleValues #2692

pksunkara opened this issue Aug 13, 2021 · 9 comments
Milestone
3.0

Comments

@pksunkara
Copy link
Member

Currently, positionals use MultipleValues during building of the arg and parsing. We need to change that to rely on MultipleOccurrences
@pksunkara pksunkara added this to the 3.0 milestone Aug 13, 2021
@epage
Copy link
Member

epage commented Aug 17, 2021

Could you clarify which cases you are referring to?

@pksunkara
Copy link
Member Author

Hmm.. all the code whenever dealing with positionals which are multiple? Not exactly sure what you are asking.

@epage
Copy link
Member

epage commented Aug 18, 2021

Can you point me to a specific example?

@pksunkara
Copy link
Member Author

pksunkara commented Sep 23, 2021 •
edited

Examples:

  1. clap/src/parse/parser.rs

    Line 182 in 5580e8c

    a.is_set(ArgSettings::MultipleValues) && (a.index.unwrap_or(0) != highest_idx)

  2. clap/src/parse/parser.rs

    Line 395 in 5580e8c

    a.is_set(ArgSettings::MultipleValues)

@epage
Copy link
Member

epage commented Sep 24, 2021

I'd have to dig in deeper, those might still be valid cases for multiple values.

@epage epage mentioned this issue Sep 24, 2021
Closed
2 tasks
@pksunkara
Copy link
Member Author

Yeah, they might be valid use cases, but I was just show casing that we need to use the MultipleOccurrences as the main setting to imply that a positional can be used multiple times.

epage added a commit to epage/clap that referenced this issue Oct 15, 2021
@epage
docs: Encourage multiple_occurrences
5949762 
There were fewer occasions than I expected where the use of
`multiple_values` was superfluous and we could instead use the more
predictable `multiple_occurrences`.

In terms of the remaining `multiple` split work, clap-rs#1772 will take care of the derive
behavior and clap-rs#2692 will resolve any remaining issues with values vs
occurrences in positional arguments.

Fixes clap-rs#2816
@epage epage mentioned this issue Oct 15, 2021
Merged
@epage epage mentioned this issue Oct 30, 2021
Merged
epage added a commit to epage/clap that referenced this issue Oct 30, 2021
@epage
fix(usage)!: Separate mutli-occ from multi-val syntax
53bd42a 
In looking at multiple occurrences and values for issues like clap-rs#2692, I
noticed that `...` can mean both multiple values and multiple
occurrences, like before we split them.

Pros
- No syntax change with clap3

Cons
- All the reasons we split `multiple` into two

Uncertain
- I originally started this as part of another branch but I lost track
  if something depended on this.  I'll have to do more digging

BREAKING CHANGE: If `--opt [val]...` was meant for
- only multiple occurrences, see `[opt]... --opt [val]`
- both multiple occurrences and values, see `[opt]... --opt [val]...`
@epage epage mentioned this issue Oct 30, 2021
Merged
epage added a commit to epage/clap that referenced this issue Nov 1, 2021
@epage
fix(usage): Switch positionals... from multi-val to mulit-occur
22d5b23 
I noticed this while investigating clap-rs#2692.  Since we are making
multiple-occurrences a thing for positional arguments, this allows us to
remove a special case.

Another way to look at this is that we should make the default whatever
we do for dervies (clap-rs#1772).  I'm going to propose we make the derive
always turn `Vec<i32>` into multiple occurences and not multiple values
(with users being able to change it through attributes), but that is an
in-work proposal and not decided yet.
@epage epage mentioned this issue Nov 1, 2021
Merged
epage added a commit to epage/clap that referenced this issue Nov 1, 2021
@epage
fix(usage)!: Switch positionals... from multi-val to mulit-occur
62daf1e 
I noticed this while investigating clap-rs#2692.  Since we are making
multiple-occurrences a thing for positional arguments, this allows us to
remove a special case.

Another way to look at this is that we should make the default whatever
we do for dervies (clap-rs#1772).  I'm going to propose we make the derive
always turn `Vec<i32>` into multiple occurences and not multiple values
(with users being able to change it through attributes), but that is an
in-work proposal and not decided yet.

BREAKING CHANGE: `Arg::from(...)` will now use `multiple_occurrences`
for a positional `...`, rather than `multiple_values`.
epage added a commit to epage/clap that referenced this issue Nov 1, 2021
@epage
fix: Always respect positional occurrences
02b7339 
When supporting multiple occurrences for positional arguments in clap-rs#2804,
I added some tests to cover this but apparently simpler cases fail
despite those more complicated tests.

This adds more multiple-occurrences tests for positional arguments,
fixes them, and in general equates multiple values with occurrences for
positional arguments as part of clap-rs#2692.  There are a couple more points
for consideration for clap-rs#2692 for us to decide on once this unblocks them
(usage special case in clap-rs#2977 and how subcommand help should be handled).

I fully admit I have not fully quantified the impact of all of these
changes and am heavily relying on the quality of our tests to carry this
forward.
epage added a commit to epage/clap that referenced this issue Nov 1, 2021
@epage
fix: Always respect positional occurrences
b259e07 
When supporting multiple occurrences for positional arguments in clap-rs#2804,
I added some tests to cover this but apparently simpler cases fail
despite those more complicated cases being tested.

This adds more multiple-occurrences tests for positional arguments,
fixes them, and in general equates multiple values with occurrences for
positional arguments as part of clap-rs#2692.  There are a couple more points
for consideration for clap-rs#2692 for us to decide on once this unblocks them
(usage special case in clap-rs#2977 and how subcommand help should be handled).

I fully admit I have not fully quantified the impact of all of these
changes and am heavily relying on the quality of our tests to carry this
forward.
@epage epage mentioned this issue Nov 1, 2021
Merged
epage added a commit to epage/clap that referenced this issue Nov 1, 2021
@epage
fix: Always respect positional occurrences
4f1a25b 
When supporting multiple occurrences for positional arguments in clap-rs#2804,
I added some tests to cover this but apparently simpler cases fail
despite those more complicated cases being tested.

This adds more multiple-occurrences tests for positional arguments,
fixes them, and in general equates multiple values with occurrences for
positional arguments as part of clap-rs#2692.  There are a couple more points
for consideration for clap-rs#2692 for us to decide on once this unblocks them
(usage special case in clap-rs#2977 and how subcommand help should be handled).

I fully admit I have not fully quantified the impact of all of these
changes and am heavily relying on the quality of our tests to carry this
forward.
epage added a commit to epage/clap that referenced this issue Nov 2, 2021
@epage
fix: Always respect positional occurrences
66341b3 
When supporting multiple occurrences for positional arguments in clap-rs#2804,
I added some tests to cover this but apparently simpler cases fail
despite those more complicated cases being tested.

This adds more multiple-occurrences tests for positional arguments,
fixes them, and in general equates multiple values with occurrences for
positional arguments as part of clap-rs#2692.  There are a couple more points
for consideration for clap-rs#2692 for us to decide on once this unblocks them
(usage special case in clap-rs#2977 and how subcommand help should be handled).

I fully admit I have not fully quantified the impact of all of these
changes and am heavily relying on the quality of our tests to carry this
forward.
epage added a commit to epage/clap that referenced this issue Nov 2, 2021
@epage
fix(usage)!: Switch positionals... from multi-val to mulit-occur
dd2c4c6 
I noticed this while investigating clap-rs#2692.  Since we are making
multiple-occurrences a thing for positional arguments, this allows us to
remove a special case.

Another way to look at this is that we should make the default whatever
we do for dervies (clap-rs#1772).  I'm going to propose we make the derive
always turn `Vec<i32>` into multiple occurences and not multiple values
(with users being able to change it through attributes), but that is an
in-work proposal and not decided yet.

BREAKING CHANGE: `Arg::from(...)` will now use `multiple_occurrences`
for a positional `...`, rather than `multiple_values`.
epage added a commit to epage/clap that referenced this issue Nov 3, 2021
@epage
fix: Switch sub-commands from multi-val to multi-occur
5b353be 
Similar to clap-rs#2977, this changes positional argument `<subcmd>` in
`help <subcmd>` to be multiple occurrences, from being multiple values.

This is part of clap-rs#2692 where we re-evaluate the usage of multiple values
for positionals now that we accept multiple occurrences.
epage added a commit to epage/clap that referenced this issue Nov 3, 2021
@epage
fix: Switch sub-commands from multi-val to multi-occur
9cbccb8 
Similar to clap-rs#2977, this changes positional argument `<subcmd>` in
`help <subcmd>` to be multiple occurrences, from being multiple values.

This is what identified the usage generation bug fixed in clap-rs#2978 and was
isolated into the test case `positional_multiple_values_is_dotted`.

This is part of clap-rs#2692 where we re-evaluate the usage of multiple values
for positionals now that we accept multiple occurrences.
@epage epage mentioned this issue Nov 3, 2021
Merged
epage added a commit to epage/clap that referenced this issue Nov 3, 2021
@epage
fix: Switch sub-commands from multi-val to multi-occur
8212647 
Similar to clap-rs#2977, this changes positional argument `<subcmd>` in
`help <subcmd>` to be multiple occurrences, from being multiple values.

This is what identified the usage generation bug fixed in clap-rs#2978 and was
isolated into the test case `positional_multiple_occurrences_is_dotted`.

This is part of clap-rs#2692 where we re-evaluate the usage of multiple values
for positionals now that we accept multiple occurrences.
@epage
Copy link
Member

epage commented Nov 3, 2021 •
edited

I'm not aware of any work remaining on this issue once we merge #2978, #2977, and #2984,

Does anyone want to double check or should we go ahead and close this out once those are merged?

(of course we might find more things, like as I work on my experiments for #1772 but those can be responded to as they come up rather than keeping this open without a clear end goal).

@pksunkara
Copy link
Member Author

pksunkara commented Nov 3, 2021 •
edited

I will double check over the weekend and close this of there is nothing else.

@pksunkara
Copy link
Member Author

Double checked and everything looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
Development

No branches or pull requests
2 participants
@epage @pksunkara