fix: Remove overriden occurrences as we go #3225
Merged
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
This makes some naming more explicit and allows us to increment occurrences before adding values (which is the logical mental order and allows other improvements).
This is the logical order people think in terms of (`--foo` comes before `val` in `--foo val`). This also opens things up for using occurrence incrementing as a place to handle occurrence transitions, like overrides.
This will have a slight performance hit as `AllArgsOverrideSelf` will allocate a single-element `Vec` per `Arg`. This can be lessened for positional and multiple occurrences by shifting those checks up into the propagation check. Long term, I'd like to see all of the `Arg`-in-`App` settings switch to this which will make it easier for callers, like completions and man pages, to figure out whats going on without duplicating the logic. Short term, this is motivated by wanting to change the override logic to accomodated interleaved overrides from clap-rs#3217. Moving this logic will make it easier to change the override logic.
This allows two overriding args to interact with each other mid-line. This was broken in 7673dfc / clap-rs#1154. Before that, we duplicated the override logic in several places. Now we plug into the start of a new arg which allows us to do this incrementally without making the logic complex or inefficient, thanks to prior refactors. Fixes clap-rs#3217
pksunkara
reviewed
Jan 1, 2022
| // | ||
| // We can evaluate switching this to a debug assert at a later time (though it will | ||
| // require changing propagation of `AllArgsOverrideSelf`). Being conservative for now | ||
| // due to where we are at in the release. |
There was a problem hiding this comment.
Now that we released, can we add a debug assert for this?
There was a problem hiding this comment.
I think doing so would be a breaking change. This was written assuming we wouldn't be changing it for 3.x because it didn't seem worth addressing in our current time frame.
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.
This allows two overriding args to interact with each other mid-line.
This was broken in 7673dfc / #1154. Before that, we duplicated the override
logic in several places.
Now we plug into the start of a new arg which allows us to do this
incrementally without making the logic complex or inefficient, thanks to
the refactors in this PR.
Fixes #3217