Skip to content

Honor quote preference for legacy-quoted scalars - #380

Open
oguzkilcan wants to merge 1 commit into
yaml:mainfrom
oguzkilcan:quote-preference-legacy-fix
Open

oguzkilcan wants to merge 1 commit into
yaml:mainfrom
oguzkilcan:quote-preference-legacy-fix

Conversation

@oguzkilcan

Copy link
Copy Markdown
Contributor

stringv() method of Representer hardcoded single-quote style whenever a string needed quoting to avoid being misread as an old-style bool, base60 float, or merge key, bypassing the configured QuotePreference entirely. Marshal() always requests WithV3Defaults() (QuoteLegacy) for backward compatibility with the legacy encoder, so every caller of plain Marshal() silently got single-quoted output no matter what preference it asked for.

Copilot AI review requested due to automatic review settings July 11, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes quoting behavior for legacy-quoted scalar strings by ensuring the representer respects the configured QuotePreference when it must force quotes for YAML 1.1-ambiguous values (e.g., old-style bools like off, base60-float-like values like 1:1, and merge-key-like <<). This aligns Marshal() output (which uses WithV3Defaults() / QuoteLegacy for backward compatibility) with the actual legacy quoting expectations instead of always producing single-quoted scalars in these cases.

Changes:

  • Update Representer.stringv() to select single vs double quotes based on r.quotePreference when needsQuoting is true.
  • Update golden expectations for affected legacy-quoted outputs in testdata/encode.yaml.
  • Add explicit encode-opts test coverage for the "off" YAML 1.1 bool-like string across QuoteSingle, QuoteDouble, and QuoteLegacy.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
testdata/encode.yaml Updates expected YAML output quoting and adds new quote-preference test cases for YAML 1.1 bool-like strings (off).
internal/libyaml/representer.go Ensures representer-forced quoting (YAML 1.1 compatibility) honors the configured QuotePreference rather than hardcoding single quotes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/libyaml/representer.go Outdated
Comment thread testdata/encode.yaml
Comment on lines 396 to +399
'<<': []
type: map[string]any
want: |
'<<': []
"<<": []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is something strange to me here.

There is no reason to use the double quote unless we are in v3 compatibility, right?

Or is it our test suite that forces this setting.

What do you think @colinjlacy ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Sorry @ccoVeille I missed the question mark there, I thought you were expecting @colinjlacy to answer.

Yes, it is the test suite. runEncodeTest uses yaml.Marshal, which always passes WithV3Defaults(), so the preference is QuoteLegacy, and QuoteLegacy means double quotes in the representer.

About v3 compatibility: v3 does not quote << at all, so it cannot tell us single or double here. I kept double because QuoteLegacy already gives it without a special case.

I also added two encode-opts tests for <<, one with single and one with double, so both preferences are now covered.

❯ make test-internal
go test ./internal/...    
ok      go.yaml.in/yaml/v4/internal/libyaml     (cached)
ok      go.yaml.in/yaml/v4/internal/testutil/assert     (cached)
?       go.yaml.in/yaml/v4/internal/testutil/datatest   [no test files]
ALL INTERNAL FILES PASS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK thank you. I feel like we should move the test suites to v4 default then.

I'm not expecting this to be done in your PR.

We need to discuss it with other maintainers

@oguzkilcan
oguzkilcan force-pushed the quote-preference-legacy-fix branch 3 times, most recently from 8052b16 to 8d41bc2 Compare July 13, 2026 07:33
@oguzkilcan
oguzkilcan force-pushed the quote-preference-legacy-fix branch from 8d41bc2 to 6d51a81 Compare July 29, 2026 16:49
@oguzkilcan
oguzkilcan force-pushed the quote-preference-legacy-fix branch from 6d51a81 to 7fb5aab Compare August 19, 2026 14:51
`stringv()` method of `Representer` hardcoded single-quote style whenever a string needed quoting to avoid being misread as an old-style bool, base60 float, or merge key, bypassing the configured QuotePreference entirely. `Marshal()` always requests `WithV3Defaults()` (`QuoteLegacy`) for backward compatibility with the legacy encoder, so every caller of plain `Marshal()` silently got single-quoted output no matter what preference it asked for.
@oguzkilcan
oguzkilcan force-pushed the quote-preference-legacy-fix branch from 7fb5aab to 5b94ec7 Compare August 20, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants