Conversation
ccoVeille
left a comment
ccoVeille
left a comment
There was a problem hiding this comment.
Thanks for PR.
I reviewed it.
| { | ||
| "\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", |
There was a problem hiding this comment.
Maybe you could use string literal (here and everywhere) to improve readability
| "\"\\tThis starts with tab\\nand is long enough\\nfor literal style\"\n", | |
| `"\tThis starts with tab\nand is long enough\nfor literal style"`+"\n", |
| withoutLeadingBreaks := strings.TrimLeft(s, "\r\n\u0085\u2028\u2029") | ||
| if strings.HasPrefix(withoutLeadingBreaks, "\t") { | ||
| return false | ||
| } |
There was a problem hiding this comment.
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
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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
2 participants
Fixes #383.
Multiline values whose first content character is a tab are now emitted as double-quoted scalars, so output from Marshal can be read back by Unmarshal. The same selection is used for unstyled Nodes created with SetString.
Tabs on later lines continue to use literal style. Tests cover direct leading tabs, initial line breaks, map values, and Nodes.