Skip to content

Fix marshaling of leading and trailing whitespace#792

Open
quentinmit wants to merge 3 commits intogoccy:masterfrom
quentinmit:leading-tabs
Open

Fix marshaling of leading and trailing whitespace#792
quentinmit wants to merge 3 commits intogoccy:masterfrom
quentinmit:leading-tabs

Conversation

@quentinmit
Copy link
Copy Markdown
Contributor

Before submitting your PR, please confirm the following.

  • Describe the purpose for which you created this PR.
  • Create test code that corresponds to the modification

Current go-yaml will marshal map[string]string{"a": "\tvalue"} as "a: \tvalue" (that is, a literal tab character). Unmarshaling that produces map[string]string{"a": "value"} instead of the original map.

In addition to fixing leading tabs and leading newlines, I also changed TestYAMLTestSuite to round-trip every value to make sure that they can be properly marshaled and unmarshaled without changing the value. This uncovered several broken test cases. All but one are fixed by this PR, since that last one doesn't (primarily, at least) involve whitespace.

This ensures that we can successfully marshal/unmarshal the value.

One test now fails. I marked it as a failing test instead of trying to
figure out how to fix it.
@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.00%. Comparing base (25e5d90) to head (32ec83b).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #792      +/-   ##
==========================================
+ Coverage   77.97%   78.00%   +0.03%     
==========================================
  Files          22       22              
  Lines        8108     8108              
==========================================
+ Hits         6322     6325       +3     
+ Misses       1370     1367       -3     
  Partials      416      416              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

"invalid-comment-after-end-of-flow-sequence",
"invalid-comma-in-tag",
"plain-dashes-in-flow-sequence",
"spec-example-8-17-explicit-block-mapping-entries",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why did this test case start to fail?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It did not start to fail; it was always broken, but because the test code never checks how the data remarshals (e.g. by checking OutYAML), we didn't know that it was broken.

The initial unmarshal (still) works, but it cannot be re-marshaled correctly:

        yaml_test_suite_test.go:132: failed to round-trip value:
            [yaml]
            |
              block key:
            - one
            - two
            explicit key: null
            
            [err]
            [3:1] value is not allowed in this context
               1 | |
               2 |   block key:
            >  3 | - one
                   ^
               4 | - two
               5 | explicit key: null

The original test input is:

explicit key:
? |
  block key
: - one
  - two

We were and are correctly unmarshaling that to

{
  "explicit key": null,
  "block key\n": [
    "one",
    "two"
  ]
}

but when we try to remarshal that, we generate

|
  block key:
- one
- two
explicit key: null

without a ? before |\nblock key, which is invalid YAML.

This is a pre-existing bug and this PR does not cause or change it.

@goccy goccy added the reviewed label Nov 29, 2025
@shoce
Copy link
Copy Markdown

shoce commented Mar 10, 2026

@goccy can we somehow merge is faster? any changes needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants