fix(usage)!: Switch positionals... from multi-val to mulit-occur #2977
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
epage
changed the title
|
Since this is written, I thought I'd go ahead and post it. We can decide if we want this or if we should wait on #1772 before making that decision. |
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`.
pksunkara
approved these changes
Nov 1, 2021
epage
added a commit
to epage/clap
that referenced
this pull request
Nov 1, 2021
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 pull request
Nov 1, 2021
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 pull request
Nov 1, 2021
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 pull request
Nov 2, 2021
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.
kbknapp
approved these changes
Nov 3, 2021
|
bors r=pksunkara |
epage
added a commit
to epage/clap
that referenced
this pull request
Nov 3, 2021
epage
added a commit
to epage/clap
that referenced
this pull request
Nov 3, 2021
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 pull request
Nov 3, 2021
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
added a commit
to epage/clap
that referenced
this pull request
Nov 3, 2021
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.
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed this while investigating #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 (#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 usemultiple_occurrencesfor a positional
..., rather thanmultiple_values.