Detect duplicate keys of mapping nodes #141
Open
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
…in a mapping node
…icate keys in a mapping node are handled
kescobo
reviewed
May 29, 2024
kescobo
approved these changes
May 29, 2024
There was a problem hiding this comment.
I'm fine with this approach, but given the clarity of the spec, this does seem like a clear bug, which IMO could be fixed in a patch release even if it breaks existing code - that code shouldn't work.
That said, I can also see the argument for this approach which gives folks a flashing red light, but doesn't stop their code from running. I don't feel strongly one way or the other.
…epending on `strict_unique_keys` Co-authored-by: Kevin Bonham <[email protected]>
|
Depending on implementation details, this may or may not conflict with the override functionality of merge tags, https://yaml.org/type/merge.html. Edit: It's probably fine since there's a merge test which doesn't fail. |
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 (at least partially) fixes #140 by checking for the existence of a
keybefore inserting it intomapping, which requires thedicttypeto implementsetindex!(since it's inside thetry catchI thought it was safe to just assume that all "proper dicttypes" should support that, would otherwise just get caught).I suppose that "bug fix" could be potentially seen as breaking change to some code bases (but maybe a good one?), which is why I've opted to do it in a non-breaking way, defaulting to not throwing and sticking to the previous behavior. Could be worth to consider always throwing here though...