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

Correctly quote all possible booleans. #53

Closed
hrach opened this issue Feb 15, 2017 · 5 comments · Fixed by #72
Closed

Correctly quote all possible booleans. #53

hrach opened this issue Feb 15, 2017 · 5 comments · Fixed by #72

Comments

@hrach
Copy link

hrach commented Feb 15, 2017

Currently, the library incorrectly doesn't add quotes to strings, which will be interpreted as booleans:

yaml-rust/src/emitter.rs

Lines 270 to 271 in bbe0696

|| string == "true"
|| string == "false"

Specs describes many other possiblities:
http://yaml.org/type/bool.html

Personally, I encounter this bug with string TRUE in https://github.com/InQuicker/ktmpl/ which produced such value without qutes and kubernetes doesn't accept that value.

@dtolnay
Copy link
Collaborator

dtolnay commented May 8, 2017

Thanks for catching this. We would love a PR to fix this!

@ghost
Copy link

ghost commented May 22, 2017

It would be great if these (previously) valid boolean values ("YES", for example) are always quoted anyway to assure the other side will always decode it as a string. kubectl for example validates the parsed yaml and aborts if booleans are encountered instead of strings.

Leaving them quoted is harmless for version 1.2, and means version 1.1 validations aren't broken unnecessarily.

hoodie added a commit to hoodie/yaml-rust that referenced this issue May 22, 2017
hoodie added a commit to hoodie/yaml-rust that referenced this issue May 22, 2017
@hoodie
Copy link
Contributor

hoodie commented May 22, 2017

@hrach sorry I misread your issue there
does this perhaps do the trick? (#72)

@ghost
Copy link

ghost commented May 22, 2017

@hoodie For me that would do it perfectly! Once that issue lands, my problem would be solved. For now we solved it by actually changing the consumer code, which luckily was in our reach.

@hrach
Copy link
Author

hrach commented May 22, 2017

Yes, that seems to be the correct fix. Sorry for writing so unclear issue :-)

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 a pull request may close this issue.

3 participants