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

Fix bug with resolving merging scalars #815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sergey-gru
Copy link

This request adds a patch to the problem I described in topic #814

I went back to the BaseConstructor class and analyzed method construct_mapping. What I ended up with was a new method, which I put in a safe constructor since it involved tags.

I described both the case of merging with a single scalar and with an array of scalars, each of which, after construction, should become a dictionary. Scalars are merged sequentially and in the order in which they appear in the Yaml code.

I hope I was able to create a simple code.

@perlpunk
Copy link
Member

perlpunk commented Jul 7, 2024

There are existing issues about this, and it is not that trivial to fix it. I tried it myself but my code isn't ready yet.

There are a few problems with the PR as you can see from the test failures (although the actual failures are hard to find in those very verbose stacktraces):

Handling of value tags

You removed handling of tag:yaml.org,2002:value completely. Although probably rarely used, this needs to stay. This is responsible for most of the test failures.

The order of the merge key inserting is now wrong

Example:

- &defaults
  a: default a
  b: default b
  c: default c
- &defaults2
  a: default2 a
  b: default2 b
  c: default2 c
- a: new a
  <<: [*defaults, *defaults2]
  <<: { INLINE: MAPPING }
  d: new d
- a: new a
  <<: [*defaults2, *defaults]
  <<: { INLINE: MAPPING }
  d: new d

The expected result is (dumped as YAML for readability):

- a: default a
  b: default b
  c: default c
- a: default2 a
  b: default2 b
  c: default2 c
- INLINE: MAPPING
  a: new a
  b: default b
  c: default c
  d: new d
- INLINE: MAPPING
  a: new a
  b: default2 b
  c: default2 c
  d: new d

With this PR, I get:

- a: default a
  b: default b
  c: default c
- a: default2 a
  b: default2 b
  c: default2 c
- a: default2 a
  b: default2 b
  c: default2 c
  d: new d
- a: default a
  b: default b
  c: default c
  d: new d

1. INLINE: MAPPING is missing completely

2. Order of aliases in a sequence

In <<: [*defaults2, *defaults], *defaults2 should win over *defaults. The PR handles it in the wrong order

Also, when there are multiple merge keys, e.g.

<<: [*A, *B]
<<: *C

it should be handled as if it was

<<: [*A, *B, *C]

3. Precedence of inline keys

- a: new a
  <<: *foo
  d: new d

For this the inline a: new a and d: new d should both win. Merge keys have lower priority. The position of the merge key(s) in the mapping doesn't matter at all.

That's why in the original code, the merge keys are collected first and then are together put at the beginning of the mapping nodes list at the end of flatten_mapping.
Also, flatten_mapping should stay its own function.
Also it would be good to actually have a test case for this (it doesn't need to implement an include, just a constructor that returns a mapping).

See also https://yaml.org/type/merge.html

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.

2 participants