Skip to content

yamlpatch: apply newline to EOF consistently - #1372

Merged
woodruffw merged 4 commits into
mainfrom
ww/fix-trailing-nl
Nov 27, 2025
Merged

yamlpatch: apply newline to EOF consistently#1372
woodruffw merged 4 commits into
mainfrom
ww/fix-trailing-nl

Conversation

@woodruffw

Copy link
Copy Markdown
Member

Follows #1323. Fixes #1370.

Signed-off-by: William Woodruff <william@yossarian.net>
Comment on lines +471 to 473
if !patched_content.ends_with('\n') {
patched_content.push('\n');
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the main change: instead of trying to preserve a newline in a replacement span, we now unconditionally insert one at EOF if not already present.

This ends up being an overproduction if a patch doesn't span the EOF, but it's a strictly correct one.

@woodruffw woodruffw added the bugfix Fixes a known bug label Nov 27, 2025
@woodruffw woodruffw changed the title yamlpatch: apply newline to EOD consistently yamlpatch: apply newline to EOF consistently Nov 27, 2025
Comment on lines -222 to -227
// Only include trailing newline if it's the last character in the document.
// This preserves document-level trailing newlines without interfering with
// newlines that are part of the document structure.
if end + 1 == content.len() && content.as_bytes()[end] == b'\n' {
end += 1;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't try to prove it out, but I think the reason this didn't work is because some incoming patches didn't have a trailing newline, and so if tree-sitter-yaml didn't include one in the captured node for whatever reason we'd end up trimming it.

Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
@woodruffw
woodruffw marked this pull request as ready for review November 27, 2025 22:03
@woodruffw
woodruffw merged commit b8ad092 into main Nov 27, 2025
9 checks passed
@woodruffw
woodruffw deleted the ww/fix-trailing-nl branch November 27, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a known bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1.17.0: Test test_preserve_trailing_newline_replace_multiline_at_end failing on openSUSE with

1 participant