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

Marshal YAML 1.1 bool-like to strings #583

Merged
merged 2 commits into from
Mar 13, 2020
Merged

Conversation

tcolgate
Copy link
Contributor

I'm not 100% sure if this is the right place to do this, it works, and
is somewhat in the spirit of decode.go. I've put an explanation of the
reasoning in the commit, not sure if you'd rather have something in the README.

@tcolgate tcolgate changed the base branch from v2 to v3 March 13, 2020 09:02
@tcolgate
Copy link
Contributor Author

Fixes #582

encode.go Outdated
@@ -320,6 +320,15 @@ func (e *encoder) stringv(tag string, in reflect.Value) {
// there's no need to quote it.
rtag, _ := resolve("", s)
canUsePlain = rtag == strTag && !isBase60Float(s)

// Maintain some backward compatibility with YAML 1.2
if rtag == strTag {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be slightly nicer if we had it inside something like isOldBool and put the test next to the isBase60Float above, as that method is the exactly equivalent except for base 60 floats (!!!, and you thought the on/off situation was bad).

Checking for isBase60Float usage should find us the exact places we need to care about. Well, either that or we have two bugs instead of one.. ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, have moved the check out and added the test to the above line. it does read more clearly.

We adhere to the YAML 1.3 for input, but only parsing "On", "Off", and
friends, as strings rather than bools. When outputting however we will
ensure that we quote these cases. Failure to do so forces makes the
output ambiguous if it then to be parsed by a YAML 1.2 parser.
@niemeyer
Copy link
Contributor

niemeyer commented Mar 13, 2020

Thanks, glad to merge this.

Have you signed the contributor agreement before? It's trivial to sign online, and it's a non-draconian licensing with you retaining ownership:

https://ubuntu.com/legal/contributors

@tcolgate
Copy link
Contributor Author

I think I just signed the form, I put you down as "Canonical Project Manager or contact"

@niemeyer
Copy link
Contributor

Thanks!

@niemeyer niemeyer merged commit 9f266ea into go-yaml:v3 Mar 13, 2020
@niemeyer niemeyer changed the title Marshal YAML 1.2 bool-like to strings. Marshal YAML 1.1 bool-like to strings Mar 13, 2020
@niemeyer
Copy link
Contributor

For the record, please note that those are YAML 1.1 strings, vs. the new 1.2 style that is more strictly implemented in go-yaml v3 (not 1.2 and 1.3).

Thanks again for your help.

@tcolgate
Copy link
Contributor Author

Right, thanks. I had it my head the "fix" to the spec was in 1.3, I think I saw that on the issue that pointed out they've been removed. I think I fixed the comments and the PR title, but not the commit message.
Thanks very much for go-yaml v3, exposing the AST made a port of a python tool to Go we have possible.

@laverya
Copy link
Contributor

laverya commented Mar 20, 2020

IMO this removes the need for #574 and #490, too - happy to see one of these finally get merged!

@laverya
Copy link
Contributor

laverya commented Mar 20, 2020

(And fixes issues #489 and #214)

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.

3 participants