Repository navigation
fix(document): preserve comments, plain-scalar leaders and comment co… - #5
Merged
Merged
Conversation
meezaan
force-pushed
the
fix/round-trip-fidelity
branch
from
September 15, 2026 06:51
2792bdf to
154cc12
Compare
…ns and line endings on round trip
Four defects that each rewrote part of a document the author did not write that
way, so a load/emit cycle was not byte-identical. All four are in the trivia and
formatting paths; none affects the data model.
1. Multi-byte characters were truncated in comments and directives.
The scanner advances by whole UTF-8 characters, but the trivia accumulators
appended a single byte, so every continuation byte was dropped: "# see §4.3"
was collected as "# see \xC24.3". The orphaned lead byte renders as U+FFFD,
which made it look like character substitution rather than truncation.
currentChar() already existed for exactly this, and its docblock says so -
it was used for scalar content but not for trivia. Seven call sites.
2. "?", ":" and "-" were treated as unconditionally reserved plain-scalar
leaders.
YAML 1.2 ns-plain-first admits them when followed by a plain-safe character;
they are indicators only when standing alone or preceding a space. So
"--cert-dir=/tmp" and "-single-dash" are valid plain scalars, and quoting
them on emit changes the document's text. Values that would reparse as a
non-string ("-5") are still quoted by the existing check.
3. Standalone comments were re-indented to their surrounding container.
The emitter already honoured CommentNode::getIndent() and the scanner
already captured each comment's column, but the parser passed a hardcoded 0.
The indent is now nullable rather than zero-defaulted, because column 1 is a
real position: a flush-left comment inside an indented block is a deliberate
style that "0 means unset" could not express. A programmatically created
CommentNode has no recorded column and still follows its container.
4. CRLF documents were rewritten with LF.
The scanner normalises CRLF to LF so the rest of the parser sees one line
ending, and the emitter already writes whatever YamlStream::getLineEnding()
names - but nothing ever called setLineEnding(), so the ending the source
used was discarded. Pipeline::parse() already derives setTrailingNewline()
from the raw source and now derives the line ending the same way.
It is set only when every newline in the source is CRLF. A file with mixed
endings has no single ending to preserve, and quietly picking one would
rewrite the other.
Raw source needed the same treatment: it is captured after normalisation, so
a block scalar re-emitted verbatim carried LF into a CRLF document. It now
goes through a helper that rewrites newlines to the document's ending, which
is a no-op for LF.
Three of the four are omissions rather than disagreements: currentChar() was
written for exactly this truncation, CommentNode::getIndent() was already
honoured by the emitter, and YamlStream::getLineEnding() was already read by it.
In each case the mechanism existed and nothing wired it up. Only 2 changes
behaviour the code had deliberately chosen.
Tests: 32 cases across four files, covering Latin-1, accented Latin, Arabic,
CJK, emoji, end-of-line and indented comments; plain and genuinely-indicator
leaders; comment columns above, below and matching their siblings; and CRLF
with indentation, comments, sequences and block scalars. 21 of the 32 fail
without these changes.
The existing suite is unchanged at 1022 tests with the same single pre-existing
error and failure (ConformanceTest fixtures and a PHPUnit 12 data-provider
arity issue), neither related to these changes.
meezaan
force-pushed
the
fix/round-trip-fidelity
branch
from
September 15, 2026 06:57
154cc12 to
6ff1a9c
Compare
ralflang
approved these changes
Oct 2, 2026
ralflang
left a comment
ralflang
left a comment
Member
There was a problem hiding this comment.
Thank you. This is very helpful. I will release today.
Contributor
Author
Thank you! This package saved me a few days of work, and I'm immensely grateful for that. |
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.
…lumns on round trip
Three defects that each rewrote a line the author did not write that way, so a load/emit cycle was not byte-identical. All three are in the trivia path; none affects the data model.
Multi-byte characters were truncated in comments and directives.
The scanner advances by whole UTF-8 characters, but the trivia accumulators appended a single byte, so every continuation byte was dropped: "# see §4.3" was collected as "# see \xC24.3". The orphaned lead byte renders as U+FFFD, which made it look like character substitution rather than truncation.
currentChar() already existed for exactly this, and its docblock says so - it was used for scalar content but not for trivia. Seven call sites.
"?", ":" and "-" were treated as unconditionally reserved plain-scalar leaders.
YAML 1.2 ns-plain-first admits them when followed by a plain-safe character; they are indicators only when standing alone or preceding a space. So "--cert-dir=/tmp" and "-single-dash" are valid plain scalars, and quoting them on emit changes the document's text. Values that would reparse as a non-string ("-5") are still quoted by the existing check.
Standalone comments were re-indented to their surrounding container.
The emitter already honoured CommentNode::getIndent() and the scanner already captured each comment's column, but the parser passed a hardcoded 0.
The indent is now nullable rather than zero-defaulted, because column 1 is a real position: a flush-left comment inside an indented block is a deliberate style that "0 means unset" could not express. A programmatically created CommentNode has no recorded column and still follows its container.
Tests: 24 cases across three files, covering Latin-1, accented Latin, Arabic, CJK, emoji, end-of-line and indented comments; plain and genuinely-indicator leaders; and comment columns above, below and matching their siblings. 15 of the 24 fail without these changes.
The existing suite is unchanged at 1022 tests with the same single pre-existing error and failure (ConformanceTest fixtures and a PHPUnit 12 data-provider arity issue), neither related to this change.