Skip to content
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

Adding support for handling YAML Merge Key (#41) - proper keys comparison for merging #1279

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

azat
Copy link

@azat azat commented May 3, 2024

NOTE: this is based on #1243, but includes a fix for proper merging (by comparing the key itself, instead of shared_ptrs, see the last patch).

Support for YAML Merge keys ( <<: [*dict1, *dict2] ) is added. The merge
key is a specific scalar with value << (and tag !!merge) that implies
that during node construction, the map (or sequence of maps) are merged
into the current map. The priority rules are that each key from maps
within the value associated with << are added iff the key is not yet
present in the current map (and first map gets higher priority). Test
cases have been added accordingly.

nlescoua and others added 2 commits November 4, 2023 10:08
Support for YAML Merge keys ( <<: [*dict1, *dict2] ) is added. The merge
key is a specific scalar with value << (and tag !!merge) that implies
that during node construction, the map (or sequence of maps) are merged
into the current map. The priority rules are that each key from maps
within the value associated with  << are added iff the key is not yet
present in the current map (and first map gets higher priority). Test
cases have been added accordingly.
azat added 2 commits May 3, 2024 14:36
…the node)

The problem is that const_map_to.get(*j->first) compares only the
shared_ptr's, while we need to compare the key itself.

v2: remove const iterator
v3: fix use-after-free due to const reference
Consider the following YAML:

    trait1: &t1
      foo: 1

    trait2: &t2
      foo: 2

    merged:
      <<: *t1
      <<: *t2

yq reports:

    $ yq .merged.foo < /tmp/yaml
    2

while the order that yaml-cpp returns is different, since it will
firstly handle 1, and will not replace it with 2:

    $ util/parse < /tmp/yaml
    trait1:
      ? &1 foo
      : &2 1
    trait2:
      foo: 2
    merged:
      *1 : *2

(Don't mix up "*2" with "2", it is trait1)
Copy link
Contributor

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested Change:

  • MergeMapCollection must be static or in an anonymous namespace.

I think, the R-Value (&&)/std::move suggestion is a matter of taste. This library is not very strong on move semantics anyways.

src/nodebuilder.cpp Outdated Show resolved Hide resolved
src/nodebuilder.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants