fix(parser): Make behavior more consistent #3765
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 is a step towards user-visible actions
Inferred flags can make it hard for a future action to trigger behavior off of the selected alias, like we might want to do for negations, so we are now translating to the intended arg. This will also help for debugging.
We were independently starting occurrences and starting value groups. Now we do them at the same time. COMPATIBILITY: This changes us from counting occurrences per positional when using `multiple_values` to one occurrence. This is user visible and tests were written against it but it goes against the documentation and doesn't quite make sense.
Doesn't make sense to respect how the command line ended
I wrote these tests expecting to highlight a bug but it turns out things were structured just right to not exhibit it. The fact that the code looks like its broken is a problem, so I restructured it (put it first, changed the source) so it doesn't look suspicious anymore.
Before, if we were in trailing values that aren't delimite, we wouldn't respect this flag and end processing of the value, now we do. This also has a slight perf benefit of us only splitting the value if the delimiter is present. We checked for the delimiter anyways, so doing it first removes a slight bit of work. I also feel this helps clarify the intended behavior and ooches us towards a unified code path for actions.
Especially important is not inferring intent from occurrences as hopefully that will change with the introduction of actions.
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.
Overall this is refactoring to slowly prepare the parser for customizable responses to arguments needed for #3405. Some of these fixed bugs along the way while others just cleaned up the parser, making it easier to understand what is going on.
The line between a bug fix and a breaking change is awkward at times. The part that seems most questionable to me is changing how we count occurrences for positional arguments. This gets us closer to the documented behavior and makes the parser more consistent. I'm trying to stop short of #3763 because I worry that resolving that would be too much of a change for the 3.x line since it ties into validation.