Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yaml-to-dhall is erratic with unions #1197

Closed
vvv opened this issue Aug 3, 2019 · 8 comments · Fixed by #1420
Closed

yaml-to-dhall is erratic with unions #1197

vvv opened this issue Aug 3, 2019 · 8 comments · Fixed by #1420

Comments

@vvv
Copy link
Contributor

vvv commented Aug 3, 2019

$ cat U.dhall
< x : { x : Optional Natural } | y : { y : Optional Natural } >

Valid YAML, but inconsistent use of quoting:

$ dhall-to-yaml <<<'let U = ./U.dhall in U.x { x = Some 1 }'
x: 1
$ dhall-to-yaml <<<'let U = ./U.dhall in U.y { y = Some 1 }'
'y': 1

Correct:

$ echo 'x: 1' | yaml-to-dhall ./U.dhall
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x { x = Some 1 }

Wrong:

$ echo 'y: 1' | yaml-to-dhall ./U.dhall
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x
{ x = None Natural }
$ echo "'y': 1" | yaml-to-dhall ./U.dhall
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x
{ x = None Natural }
@vvv
Copy link
Contributor Author

vvv commented Aug 3, 2019

$ yaml-to-dhall --version ''
1.4.0
$ dhall version
1.25.0

@vvv
Copy link
Contributor Author

vvv commented Aug 3, 2019

Similar behaviour is observed with json-to-dhall:

$ echo '{"x":1}' | json-to-dhall ./U.dhall 
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x { x = Some 1 }
$ echo '{"y":1}' | json-to-dhall ./U.dhall 
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x
{ x = None Natural }

@Gabriella439
Copy link
Collaborator

The reason for the inconsistency in dhall-to-yaml's quoting is because YAML interprets an unquoted y as true, not the string/key 'y'. So dhall-to-yaml is actually behaving correctly here and protecting against the generated YAML being misinterpreted.

This is also the same reason why this example fails:

$ echo 'y: 1' | yaml-to-dhall ./U.dhall
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x
{ x = None Natural }

Because y: 1 is not what you think it is! It's essentially the YAML version of [ { mapKey = True, mapValue = 1 } ].

However, you are right to be surprised about this behavior:

$ echo "'y': 1" | yaml-to-dhall ./U.dhall
< x : { x : Optional Natural } | y : { y : Optional Natural } >.x
{ x = None Natural }

The root cause behind that is because yaml-to-dhall does not have the --records-strict flag on by default. I actually found this behavior surprising, too, so I just fixed it yesterday in this pull request:

#1181

... and indeed on master it gives the behavior you expected:

$ yaml-to-dhall '< x : { x : Optional Natural } | y : { y : Optional Natural } >' <<< "'y': 1"
< x : { x : Optional Natural } | y : { y : Optional Natural } >.y { y = Some 1 }

@sjakobi
Copy link
Collaborator

sjakobi commented Aug 28, 2019

According to https://stackoverflow.com/a/42284910/1013393 only true, True, TRUE, false, False, and FALSE should be parsed as bools in YAML1.2. YAML1.1 bools like y or off should now be interpreted as strings.

@Gabriella439
Copy link
Collaborator

@sjakobi: Maybe that will be fixed by #1248

@sjakobi
Copy link
Collaborator

sjakobi commented Oct 7, 2019

This was indeed fixed by #1248! Thanks @vijayphoenix! :)

It might be a good idea to add regression tests, so we can ensure that this keeps working even with the aeson-yaml flag that I understand @patrickmn will introduce.

@patrickmn
Copy link
Sponsor Collaborator

Agreed.

I plan to do a PR no later than the weekend.

@patrickmn
Copy link
Sponsor Collaborator

This slipped my mind when I was adding the list of special strings (aeson-yaml will quote 'y'/'n'/'on'/'off').

Will submit a fix and regression test soon.


FYI this seems like a general issue:

go-yaml/yaml#214

kubernetes/kubernetes#34146 (comment)

(Also, perhaps expectedly, when I remove the quoting of 'yes'/'no', my test to assert that libyaml decodes the original input fails.)

..but it seems reasonable to expect the Dhall user not to feed "yes" to Kubernetes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants