Fix MergeYaml sequence entry indentation with objectIdentifyingProperty#7113
Closed
Fix MergeYaml sequence entry indentation with objectIdentifyingProperty#7113
Conversation
…ty (#7107) When merging new entries into a YAML sequence via nested MergeYamlVisitor calls (e.g. key: $ with a nested sequence), autoFormat produced incorrect indentation causing new entries to appear as children of the last existing entry's sub-sequence. Fix by re-indenting the auto-formatted prefix to match existing sequence entries while preserving comments and blank lines.
AutoFormatVisitor forks the cursor (discarding messages) and IndentsVisitor lacks sequence-specific indent awareness, so fixing the root cause would require changes to shared YAML formatting infrastructure. Document this trade-off in the method javadoc.
Jenson3210
reviewed
Mar 24, 2026
| List<Yaml.Sequence.Entry> entries = concatAll( | ||
| filter(s1.getEntries(), it -> !mutatedEntries.contains(it)), | ||
| map(mutatedEntries, it -> autoFormat(it, p, cursor)), | ||
| map(mutatedEntries, it -> reindentEntry(autoFormat(it, p, cursor), sequenceEntryIndent)), |
Contributor
There was a problem hiding this comment.
If we would pass the correct parent entry cursor here, would that make the formatting not do the correct things here?
Member
Author
There was a problem hiding this comment.
Went through some iterations here that culminated in the Javadoc in reindentEntry. Welcome to have a go as well if you think the cursor approach is doable too; that had my preference, but didn't want any ripple effects from a small change here.
Jenson3210
reviewed
Mar 24, 2026
Add a test merging a new entry with nested content (specs sequence) into an existing sequence with objectIdentifyingProperty, as requested in PR review. Extend reindentEntry to also shift indentation of nested content within the entry's block via ShiftIndentVisitor, so that mapping entries and inner sequences are corrected by the same delta as the entry prefix. Note: autoFormat always uses indented-sequence style (- indented by indentSize relative to parent key), even when the existing document uses same-column style. The shift corrects the absolute column but cannot change the relative style.
- Replace replaceFirst("^\n", "") with startsWith/substring
- Replace replaceAll("^\\s+", "") with StringUtils.indexOfNonWhitespace
- Replace char[]/Arrays.fill with StringUtils.repeat, matching IndentsVisitor
Contributor
|
@timtebeek I wrote a unit test that verifies we simply do not support same-column sequences in any formatting. Questioning if we should not try to add support rather than only have some backport that works on this one? |
Contributor
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
MergeYamlVisitorcalls (e.g.key: $with a nested sequence),autoFormatproduced incorrect indentation, causing new entries to appear as children of the last existing entry's sub-sequence rather than as siblings.