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
38 changes: 33 additions & 5 deletions internal/libyaml/emitter.go
Original file line number Diff line number Diff line change
Expand Up @@ -1987,11 +1987,39 @@ func (emitter *Emitter) writeDoubleQuotedScalar(value []byte, allow_breaks bool)
// writeBlockScalarHints writes the indentation and chomping indicators for
// block scalars.
func (emitter *Emitter) writeBlockScalarHints(value []byte) error {
if isSpace(value, 0) {
// https://github.com/yaml/go-yaml/issues/65
// isLineBreak(value, 0) removed as the linebreak will only
// write the indentation value.
indent_hint := []byte{'0' + byte(emitter.BestIndent)}
// A parser infers a block scalar's indentation from its first non-empty
// line, so an explicit indentation indicator is needed whenever that line
// begins with a space. Leading line breaks only produce empty lines, which
// carry no indentation of their own, so skip past them first: this keeps
// the #65 behavior (no indicator when the first content line is not
// indented) while covering leading-blank-then-indented scalars.
// https://github.com/yaml/go-yaml/issues/65
// https://github.com/yaml/go-yaml/issues/76
i := 0
for i < len(value) && isLineBreak(value, i) {
i += width(value[i])
}
if i < len(value) && isSpace(value, i) {
// The indicator is the content's indentation relative to the parent
// node, not BestIndent: a block sequence item only adds 2 columns, so
// emitting BestIndent produces an unparseable document. It must also be
// a single digit 1..9, so clamp it to that range and align the actual
// content indentation to the clamped value; that keeps the document
// valid and round-trippable even when a caller requests a wider indent
// (e.g. SetIndent(10)).
// https://github.com/go-yaml/yaml/issues/1071
parent_indent := 0
if len(emitter.indents) > 0 && emitter.indents[len(emitter.indents)-1] > 0 {
parent_indent = emitter.indents[len(emitter.indents)-1]
}
Comment on lines +2011 to +2014

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.

Please use what I had suggested in another PR

#393 (comment)

Suggested change
parent_indent := 0
if len(emitter.indents) > 0 && emitter.indents[len(emitter.indents)-1] > 0 {
parent_indent = emitter.indents[len(emitter.indents)-1]
}
parent_indent := 0
if n := len(emitter.indents); n > 0 && emitter.indents[n-1] > 0 {
parent_indent = emitter.indents[n-1]
}

indent := emitter.indent - parent_indent
if indent < 1 {
indent = 1
} else if indent > 9 {
indent = 9
}
emitter.indent = parent_indent + indent
indent_hint := []byte{'0' + byte(indent)}
if err := emitter.writeIndicator(indent_hint, false, false, false); err != nil {
return err
}
Expand Down
8 changes: 8 additions & 0 deletions internal/libyaml/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,14 @@ func shouldUseLiteralStyle(s string) bool {
if !strings.Contains(s, "\n") || len(s) < 2 {
return false
}
// A block scalar cannot express leading-tab indentation, so a value whose
// first content character (after any leading line breaks) is a tab must
// not use literal style; it is emitted double-quoted instead.
// https://github.com/yaml/go-yaml/issues/383
if strings.HasPrefix(
strings.TrimLeft(s, "\r\n\u0085\u2028\u2029"), "\t") {
return false
}
// Must contain at least one non-whitespace character
for _, r := range s {
if !unicode.IsSpace(r) {
Expand Down
7 changes: 6 additions & 1 deletion internal/libyaml/serializer.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,13 @@ 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"):
// Multiline but not literal-eligible (e.g. first content line
// starts with a tab): a block scalar would not round-trip, so
// emit a double-quoted scalar instead.
style = DOUBLE_QUOTED_SCALAR_STYLE
case forceQuoting:
style = s.quotePreference.ScalarStyle()
}
Comment on lines +218 to 227

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 unsure I saw test for the priority of shoulduseLiteral or new line against forceQuoting ?

Do you think you could add tests for this?

Expand Down
44 changes: 44 additions & 0 deletions internal/libyaml/testdata/emitter.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,50 @@
intro

indented tail
# Regression tests for block scalars whose first non-empty line is indented.
# Without an explicit indentation indicator the output does not parse back.
- roundtrip:
name: Literal scalar with leading empty lines and indented content
yaml: |
key: |2


more indented
regular
want: |
key: |2


more indented
regular

- roundtrip:
name: Folded scalar with leading empty lines and indented content
yaml: |
key: >2


more indented
regular
want: |
key: >2


more indented
regular

- roundtrip:
name: Literal scalar with a leading empty line and indented content
yaml: |
key: |2

indented
regular
want: |
key: |2

indented
regular

# Writer test
- emit-writer:
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
46 changes: 46 additions & 0 deletions testdata/encode.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -986,3 +986,49 @@
want: |
v: hello

# Regression tests for block scalars whose first non-empty line is indented.
# Without an explicit indentation indicator the output does not parse back.
- encode:
name: block scalar with leading empty lines and indented content
data:
v: "\n\n more indented\nregular\n"
type: map[string]string
want: |
v: |4


more indented
regular

- encode:
name: block scalar with a leading empty line and indented content
data:
v: "\n indented\nplain\n"
type: map[string]string
want: |
v: |4

indented
plain

- encode:
name: block scalar with an indented first line
data:
v: " first indented\nregular\n"
type: map[string]string
want: |
v: |4
first indented
regular

- encode:
name: block scalar with a leading empty line and unindented content
data:
v: "\nno indent\nhere\n"
type: map[string]string
want: |
v: |

no indent
here

82 changes: 77 additions & 5 deletions yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2739,6 +2739,78 @@ func TestSetIndent(t *testing.T) {
assert.Equal(t, "a:\n b:\n c: d\n", buf.String())
}

// A block scalar whose first non-empty line is indented needs an explicit
// indentation indicator, and that indicator is relative to the parent node's
// indentation, not BestIndent.
// This covers both the missing-indicator case (a leading empty line followed
// by indented content) and the wrong-value case (a scalar nested in a block
// sequence item, which only adds two columns of indentation).
// See https://github.com/yaml/go-yaml/issues/76
// and https://github.com/go-yaml/yaml/issues/1071
func TestBlockScalarIndentIndicatorRoundTrip(t *testing.T) {
type params struct {
Description string `yaml:"description"`
}
type spec struct {
Parameters []params `yaml:"parameters"`
}

// Includes indents at and beyond the 1..9 indicator range (9, 10) to
// cover clamping of the indentation indicator.
for _, indent := range []int{0, 1, 2, 3, 4, 8, 9, 10} {
for _, description := range []string{
" a\nb",
" a\nb",
" a\n b",
"\n indented\nregular",
"\n\n more indented\nregular",
} {
in := spec{Parameters: []params{{Description: description}}}

var buf bytes.Buffer
enc := yaml.NewEncoder(&buf)
if indent > 0 {
enc.SetIndent(indent)
}
assert.NoError(t, enc.Encode(&in))
assert.NoError(t, enc.Close())

var out spec
err := yaml.Unmarshal(buf.Bytes(), &out)
assert.NoErrorf(
t, err, "indent %d, encoded as:\n%s", indent, buf.String())
assert.DeepEqualf(
t, in, out, "indent %d, encoded as:\n%s", indent, buf.String())
}
}
}
Comment on lines +2742 to +2786

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 can see the loop on indentation, but could you try using the .yanl test file to validatw them.

The yaml test suite is intended to be reused on other projects using YAML.

We would appreciate


// A multiline scalar whose first content line begins with a tab cannot be
// expressed as a block scalar (block indentation is spaces only), so it must
// be emitted double-quoted and still round-trip exactly.
// See https://github.com/yaml/go-yaml/issues/383
func TestTabLeadingScalarRoundTrip(t *testing.T) {
type doc struct {
Text string `yaml:"text"`
}

for _, text := range []string{
"\tthis\nis\nmultiline",
"\tB\n\tC\n",
"\n\tindented by tab\nregular",
"first\n\tsecond\n", // tab on a later line: stays literal
} {
in := doc{Text: text}
out, err := yaml.Marshal(&in)
assert.NoError(t, err)

var back doc
assert.NoErrorf(t, yaml.Unmarshal(out, &back),
"encoded as:\n%s", out)
assert.DeepEqualf(t, in, back, "encoded as:\n%s", out)
}
}
Comment on lines +2788 to +2812

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.

Can't we express this in one of the .yaml test file ?


func TestSortedOutput(t *testing.T) {
order := []any{
false,
Expand Down Expand Up @@ -2984,27 +3056,27 @@ 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.

Here and everwhere. Please use raw string literal for 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",
},
}
Expand Down