[yaml]: Update YAML parser to yaml.v3 #72
Conversation
| @@ -13,3 +13,5 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+ | |||
| gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= | |||
| gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE= | |||
| gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= | |||
There was a problem hiding this comment.
Any idea why these gopkg.in/yaml.v2 v2.2.1 lines stuck around? Is there an import still pulling in yaml.v2 somewhere?
| @@ -41,20 +41,20 @@ func TestSkyToYaml(t *testing.T) { | |||
| `, | |||
| }, | |||
| YamlTestCase{ | |||
| skyExpr: `{"a": 5, 13: 2, "k": {"k2": "v"}}`, | |||
| skyExpr: `{13: 2, "k": {"k2": "v"}, "a": 5.11}`, | |||
There was a problem hiding this comment.
What caused this reordering? Is something changing in how map values get parsed/serialized?
| @@ -230,9 +230,9 @@ func TestYamlToSky(t *testing.T) { | |||
| if testCase.want != got.String() { | |||
| t.Error( | |||
| "Bad return value from yaml.unmarshal", | |||
| "\nExpected:", | |||
There was a problem hiding this comment.
Does this squash the test failure message onto one line? That can be hard to read for the larger input expressions.
| case starlark.Float: | ||
| return &yaml.Node{ | ||
| Kind: yaml.ScalarNode, | ||
| Value: fmt.Sprintf("%g", v), |
There was a problem hiding this comment.
Just to verify -- the Go docs for %g say it may format in scientific notation for some values. Does Go's format match YAML? yaml/pyyaml#173 suggests that at least some popular YAML implementations don't handle this well.
Good test cases would be a long positive number (enough to trigger the Go science mode), a long negative number, checking both the output text and asserting that go-yaml can parse the formatted value.
| case starlark.String: | ||
| return &yaml.Node{ | ||
| Kind: yaml.ScalarNode, | ||
| Value: string(v), |
There was a problem hiding this comment.
What happens for special-ish strings like v == "0" or v == "true"? go-yaml/yaml#490 suggests that gopkg.in/yaml.v3 might not support this properly.
A test case with some known-challenging strings would be useful to make sure Skycfg's user-visible behavior doesn't regress here.
| for i, n := 0, starlark.Len(v); i < n; i++ { | ||
| nn, err := starlarkToYAMLNode(v.Index(i)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to convert %d-th element of %v to YAML: %v", i, v, err) |
There was a problem hiding this comment.
Is this going to result in really really really big error messages for nested structures? Since the container gets printed in its entirety plus the nested error, which would be the entire second-level container plus its nested error, etc.
| return nil, err | ||
| } | ||
| return starlark.String(yamlBytes), nil | ||
| enc.Close() |
There was a problem hiding this comment.
Does the error from Close() need to be checked?
|
|
Migrate to https://github.com/go-yaml/yaml/tree/v3 and don't roundtrip through JSON to support stable order of YAML maps (perviously they would be shuffled due to round-tripping via Go map).
This can also enable support for alias YAML nodes but is not part of this PR.
This change is