Fix MergeYaml wildcard, flow-style sequence, and comment handling#7291
Fix MergeYaml wildcard, flow-style sequence, and comment handling#7291
Conversation
…ng comments - Fix JsonPathMatcher wildcard handling to expand trailing wildcards (e.g. `$.list.services.*`) to individual child entries (#3310) - Fix MergeYamlVisitor.mergeSequence() to handle flow-style sequences by using comma separators instead of newline-based indent (#2834) - Fix MergeYamlVisitor.mergeMapping() to preserve inline comments stored on Document.End when appending to deeply nested mappings (#5135) - Add documenting test for DeleteKey + MergeYaml 2-cycle behavior (#3950) - Add confirmation test for fixed multi-document merge (#3539)
|
The rest seems good aside from the question I had above. |
The YAML AST stores flow-style separators as trailingCommaPrefix on entries and spaces as the scalar block's prefix, not the entry's prefix. Updated mergeSequence() to correctly set trailingCommaPrefix on the last existing entry and the block prefix on new entries. Added tests for merging multi-element flow-style sequences and sequences where some incoming values already exist.
| "$.value", | ||
| //language=yaml | ||
| "[16, 17]", | ||
| false, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| null | ||
| )), | ||
| yaml( | ||
| """ | ||
| value: [17] | ||
| """, | ||
| """ | ||
| value: [17, 16] | ||
| """ |
There was a problem hiding this comment.
I think for a simple example like this, this is probably fine. I guess I'm wondering if this gets messy for more complicated structures where the ordering requested may actually matter. Perhaps I'm just overthinking it, though.
There was a problem hiding this comment.
Yeah there's definitely limits, but seeing how this is for flow style object I'm hoping it's rare that folks use complex values there. Do you see any object to merge this as is?
There was a problem hiding this comment.
I suppose it would also fit with scenarios where simple values order matters, but yeah, it may be stretching the use case beyond where it typically would exist. No concerns other than that for merging.
2 participants
Summary
Fixes three
MergeYamlbugs and adds test coverage for two more issues:JsonPathMatchertrailing wildcards (e.g.$.list.services.*) now expand to individual child entries instead of matching the parent mapping as a wholeMergeYamlVisitor.mergeSequence()now handles flow-style sequences ([17]+[18]→[17, 18]) by using comma separators instead of newline-based indentationMergeYamledge case: keeps comment at the end of the doc after appending an entry #5135MergeYamlVisitor.mergeMapping()now preserves inline comments stored onDocument.Endwhen appending entries to deeply nested mappings (e.g. inside sequence entries)new DeleteKey("$.*")leads to two cycles if followed bynew MergeYaml#3950 Added documenting test confirmingDeleteKey("$.*")+MergeYamlinherently requires 2 cycles (working as designed)Test plan
MergeYamlTesttests passJsonPathMatcherTesttests pass (no regressions)