Merge alias values instead of overwriting when both forms are used - #32
Open
afonsojanu wants to merge 1 commit into
Open
Merge alias values instead of overwriting when both forms are used#32afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
When a flag has an alias and both the canonical name and its alias show up in the same invocation with different values (say -H one --header=two), the final alias-sync step was blindly copying one key's value onto the other based on object-key iteration order. That meant one of the two values just got dropped, and the outcome was order-dependent rather than defined behavior. Reworked that step to actually merge values coming from independently parsed aliases in a group (using the same array-accumulation shape toVal already uses for repeated flags), falling back to a plain copy when only one side of the group was ever touched, which keeps every existing single-source case working exactly as before. Had to be careful that the accumulator variable actually resets between groups, since var doesn't do that on its own inside a loop unless you assign it explicitly. Also had to make sure the merge only looks at keys that came from real parsing, not ones a default value backfilled afterward -- those still get broadcast to their aliases the old way, otherwise a default would incorrectly count as a second value to merge against. Added a regression test covering the mixed short/long form case from the reported bug plus the two single-source cases it must not affect.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #23.
When a flag has an alias and both the canonical name and the alias show up in the same command with different values, the final alias-sync step picks whichever key comes later in object-key iteration order and copies its value over the other. So
-H one --header=twowithalias: { H: 'header' }ends up as{ H: 'one', header: 'one' }-- the'two'from the long form just disappears, and which value survives depends on iteration order rather than anything meaningful.Every other combination already worked correctly (
-H one -H two,--header one --header two, mixing short and long forms of an unaliased pair), because in those cases only one canonical key was ever actually written during parsing. It's specifically the "both sides of an alias get a value in the same invocation" case that hit the blind overwrite.I reworked the alias-sync step to merge values from a group's members that were genuinely parsed, using the same array-accumulation shape
toValalready uses when the same flag repeats, and only fall back to a plain copy when a group has just one real source (which covers every case that already worked, unchanged). Had to be a little careful that the accumulator resets between groups -- avardeclared without an initializer inside a loop body doesn't actually reset on each pass, only on first hoist, so I make that reset explicit now.One more wrinkle: a value backfilled from
opts.defaultshouldn't count as a second independent source for merging, otherwisedefault: { arg: '' }would look like a real second value and get concatenated in. I snapshot which keys came from actual parsing before defaults get applied, and only treat those as merge sources.Added a test for the mixed-form case plus the two single-source variants, to make sure those keep behaving the same. Ran the full suite locally, 92/92 passing.