-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiple merge #976
base: v3
Are you sure you want to change the base?
Multiple merge #976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to match how other applications of libyaml¹ handle https://yaml.org/type/merge.html by supporting multiple merge keys like this 👍🏼
It's true that the Merge Key spec shows passing an array of anchors to achieve this result, rather than multiple merge keys in a single mapping, but the fact that it's supported in other popular languages, and therefore used out in the wild², makes this an interoperability issue.
¹ While go-yaml isn't strictly an application of libyaml, it is a Go port of libyaml plus the Go application/integration thereof, so I think it's the same category.
² I work with @moskyb and we have examples of multiple customers using the multiple-Merge-Key style.
decode.go
Outdated
for j := i + 2; j < l; j += 2 { | ||
nj := n.Content[j] | ||
if ni.Kind == nj.Kind && ni.Value == nj.Value { | ||
// Key collisions are allowed if both of the keys are merges - each merge will be applied in order, leading to no collisions in the final decoded document | ||
if ni.Kind == nj.Kind && ni.Value == nj.Value && !isMerge(nj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably hoist this above the for j := …
loop, and say:
if isMerge(ni) {
// Merge keys ("<<") are discarded during processing, so don't need to be unique.
continue
}
Can we release it? |
Fixes #966
This PR adds the capability to decode multiple merge nodes on a given document - eg, the document
will now decode to
where previously, it would have returned an error saying that the
merged
mapping had a duplicate key<<
.While this behaviour isn't in the YAML spec (the blessed way to do multiple merges is
<<: [*one, *two]
), it's also not disallowed by it, and many yaml libraries for other languages - notably Python's pyyaml and Ruby's psych, which is backed by libyaml - do support this behaviour.Multiple issues (#966, #918) on this have been raised on this repo before, which leads me to believe that using multiple merge keys is a relatively common expectation to have for a yaml library.