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

Allow quoting of specialized strings like null/true/false/numbers etc #699

Merged
merged 4 commits into from Jul 15, 2022

Conversation

EdwardCooke
Copy link
Collaborator

Title says it all :) It fixes a long running discussion in a non-breaking of existing behavior way. All tests passed as expected including new ones to account for the new functionality. I'm really excited about this one as it has been biting me for a long time with Kubernetes manifests.

Fixes
#611
#591
#526
#387

Uses parts of #532

@aaubry
Copy link
Owner

aaubry commented Jul 15, 2022

I'm not sure that detecting that a scalar is a key should be made at the scanner level. In principle, the scanner's job is just to turn a stream of characters into a stream of tokens, without much semantics. Then the parser takes that stream and with knowledge of the grammar turns that stream of tokens into a stream of semantic events. In this context, I would expect the parser to be responsible for marking an event as a key.

Also, you are assuming that a key will always be a scalar, but YAML doesn't impose such restrictions. A key can be any node, including another mapping. For example, this is a mapping on which the first key is a scalar, the second is a mapping, and the third is a sequence:

scalar: foo
{ a: mapping }: bar
[ a, sequence ]: baz

http://ben-kiki.org/ypaste/data/56801/index.html

@aaubry
Copy link
Owner

aaubry commented Jul 15, 2022

I haven't tried, but since you added the IsKey property to the scalar token, you may have problems with anchors as well:

a: &some_scalar this is also a key
*some_scalar : "will this key be handled correctly?"

http://ben-kiki.org/ypaste/data/56802/index.html

@EdwardCooke
Copy link
Collaborator Author

@aaubry, those were some awesome YAML test cases. I added them to the great collection of test cases you already had created. The YAML you specified worked, along with adding numbers in to the mix, so it kept the datatypes the way I was expecting them to. The IsKey is probably named incorrectly, maybe something better would have been ParseDataType or something along those lines. Not sure on that though.

@aaubry
Copy link
Owner

aaubry commented Jul 15, 2022

@aaubry, those were some awesome YAML test cases. I added them to the great collection of test cases you already had created. The YAML you specified worked, along with adding numbers in to the mix, so it kept the datatypes the way I was expecting them to. The IsKey is probably named incorrectly, maybe something better would have been ParseDataType or something along those lines. Not sure on that though.

Good! I'm glad that this works 👍

@EdwardCooke EdwardCooke merged commit 38b6ff5 into aaubry:master Jul 15, 2022
@aaubry
Copy link
Owner

aaubry commented Jul 23, 2022

This feature has been released in version 12.0.0.

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

Successfully merging this pull request may close these issues.

None yet

3 participants