Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions internal/libyaml/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ func shouldUseLiteralStyle(s string) bool {
if !strings.Contains(s, "\n") || len(s) < 2 {
return false
}
withoutLeadingBreaks := strings.TrimLeft(s, "\r\n\u0085\u2028\u2029")
if strings.HasPrefix(withoutLeadingBreaks, "\t") {
return false
}
Comment on lines +253 to +256

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.

I'm surprised to see the Unicode separators in addition of \r\n

Could you cover this with tests?

Also I would like to see these Unicode character to constants with clear name and comment.

I'm a bit surprised by the fact it trims everything. It could have consequences for multi line maybe.

Something like \n\n\n\t

Maybe I'm wrong.

I feel like the issue you try to fix is about having some characters in front of \t

I feel like using strings.HasPrefix with iterative checks such as \r\n\t, \n\t, \uxxxx\t

// Must contain at least one non-whitespace character
for _, r := range s {
if !unicode.IsSpace(r) {
Expand Down
4 changes: 3 additions & 1 deletion internal/libyaml/serializer.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,10 @@ func (s *Serializer) node(node *Node, tail string) {
style = LITERAL_SCALAR_STYLE
case node.Style&FoldedStyle != 0:
style = FOLDED_SCALAR_STYLE
case strings.Contains(value, "\n"):
case shouldUseLiteralStyle(value):
style = LITERAL_SCALAR_STYLE
case strings.Contains(value, "\n"):
style = DOUBLE_QUOTED_SCALAR_STYLE
case forceQuoting:
style = s.quotePreference.ScalarStyle()
}
Expand Down
15 changes: 15 additions & 0 deletions internal/libyaml/testdata/node.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,21 @@
from: "hello\nworld"
want: true

- name: shouldUseLiteralStyle with leading tab
type: should-literal
from: "\tcontent\nnext"
want: false

- name: shouldUseLiteralStyle with leading break then tab
type: should-literal
from: "\n\tcontent\nnext"
want: false

- name: shouldUseLiteralStyle with tab on later line
type: should-literal
from: "first\n\tsecond"
want: true

- name: shouldUseLiteralStyle with single char
type: should-literal
from: "a"
Expand Down
67 changes: 62 additions & 5 deletions yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2984,29 +2984,39 @@ func TestScalarStyleWithTabs(t *testing.T) {
},
{
"\tThis starts with tab\nand is long enough\nfor literal style",
"|-\n \tThis starts with tab\n and is long enough\n for literal style\n",
"\"\\tThis starts with tab\\nand is long enough\\nfor literal style\"\n",

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.

Maybe you could use string literal (here and everywhere) to improve readability

Suggested change
"\"\\tThis starts with tab\\nand is long enough\\nfor literal style\"\n",
`"\tThis starts with tab\nand is long enough\nfor literal style"`+"\n",

"Multiline starting with tab",
},
{
"\tB\n\tC\n",
"|\n \tB\n \tC\n",
"\"\\tB\\n\\tC\\n\"\n",
"Tab B newline tab C newline",
},
{
"\ta\n",
"|\n \ta\n",
"\"\\ta\\n\"\n",
"Tab + char + newline",
},
{
"\thello\n",
"|\n \thello\n",
"\"\\thello\\n\"\n",
"Tab + text + newline",
},
{
"\t\nhello",
"|-\n \t\n hello\n",
"\"\\t\\nhello\"\n",
"Tab + newline + text",
},
{
"\n\tthis\nnext",
"\"\\n\\tthis\\nnext\"\n",
"Tab after initial newline",
},
{
"first\n\tsecond",
"|-\n first\n \tsecond\n",
"Tab after content line",
},
}

for i, testCase := range testCases {
Expand All @@ -3018,6 +3028,53 @@ func TestScalarStyleWithTabs(t *testing.T) {
}
}

func TestLeadingTabScalarRoundTrip(t *testing.T) {
testCases := []struct {
value string
mapYAML string
scalarYAML string
}{
{
"\tthis\nis\nmultiline",
"text: \"\\tthis\\nis\\nmultiline\"\n",
"\"\\tthis\\nis\\nmultiline\"\n",
},
{
"\n\tthis\nnext",
"text: \"\\n\\tthis\\nnext\"\n",
"\"\\n\\tthis\\nnext\"\n",
},
{
"first\n\tsecond",
"text: |-\n first\n \tsecond\n",
"|-\n first\n \tsecond\n",
},
}

for _, testCase := range testCases {
wantMap := map[string]string{"text": testCase.value}
data, err := yaml.Marshal(wantMap)
assert.NoError(t, err)
assert.Equal(t, testCase.mapYAML, string(data))

var gotMap map[string]string
err = yaml.Unmarshal(data, &gotMap)
assert.NoError(t, err)
assert.DeepEqual(t, wantMap, gotMap)

var node yaml.Node
node.SetString(testCase.value)
data, err = yaml.Marshal(&node)
assert.NoError(t, err)
assert.Equal(t, testCase.scalarYAML, string(data))

var gotScalar string
err = yaml.Unmarshal(data, &gotScalar)
assert.NoError(t, err)
assert.Equal(t, testCase.value, gotScalar)
}
}
Comment on lines +3031 to +3076

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.

I think we have something in the test suite that only uses .yaml files to handle this round trip check. I think it's in emitter.yaml

Could you please check?

Thanks


func TestUnicodeWhitespaceHandling(t *testing.T) {
// Test cases for Unicode whitespace characters that should be properly handled
// by the shouldUseLiteralStyle function using unicode.IsSpace()
Expand Down
Loading