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

dump fails to quote strings with leading zeros if any chars are non-octal #394

Closed
tavisrudd opened this issue Feb 1, 2018 · 11 comments
Closed

Comments

@tavisrudd
Copy link

jsyaml = require('js-yaml');
console.log(jsyaml.dump("007"));
console.log(jsyaml.dump("008"));

Should output

'007'

'008'

but it outputs

'007'

008

I'm running into this with AWS account ids that start with two leading 0s and this is causing issues with CloudFormation templates as CloudFormation then mangles the numbers further.

This case should be handled in the testAmbiguousType function passed to chooseScalarStyle in dumper.js.

@tavisrudd
Copy link
Author

It looks like the fix needs to go here https://github.com/nodeca/js-yaml/blob/master/lib/js-yaml/dumper.js#L343

@puzrin
Copy link
Member

puzrin commented Feb 1, 2018

Everything works as expected. Quotes are not used when not needed.

@puzrin puzrin closed this as completed Feb 1, 2018
@tavisrudd
Copy link
Author

They are needed in this case. If the number isn't quoted downstream tooling fails as it is ambigous. I'm going to need to hack around this and am happy to provide a PR.

@puzrin
Copy link
Member

puzrin commented Feb 1, 2018

Could you provide a proof with refer to spec or well known parser implementations? 008 is NOT a valid octal number.

@tavisrudd
Copy link
Author

You're missing the point, I'm not suggesting 008 is a valid octal number. Rather, CloudFormation and probably other tools are parsing it as number when not quoted. If js-yaml were to quote such strings, this bad interaction could be avoided.

@puzrin
Copy link
Member

puzrin commented Feb 1, 2018

Rather, CloudFormation and probably other tools are parsing it as number when not quoted.

IMHO it's much better idea to report broken yaml parser in CloudFormation tool, instead of suggest adding kludge to working and correct implementation.

@tavisrudd
Copy link
Author

tavisrudd commented Feb 1, 2018

I'll be reporting it to CloudFormation as well. However, I'd still consider it a valid enhancement to your library. Which strings to consider ambiguous and need quoting, what flow style to use, when to line wrap, etc. are stylistic choices js-yaml is making rather than strict adherence to the YAML spec.

@puzrin
Copy link
Member

puzrin commented Feb 1, 2018

I respect your opinion, but that's subjective and not enough for changes.

@puzrin
Copy link
Member

puzrin commented Feb 1, 2018

If you need quick and dirty workaround - use JSON.stringify(), it's compatible with yaml.

@tavisrudd
Copy link
Author

tavisrudd commented Feb 1, 2018

Would you be open to a dump option to always force quoting of single line strings? That feels in line with the types of control the existing options provide and isn't a single-case kludge.

@puzrin
Copy link
Member

puzrin commented Feb 1, 2018

Would you be open to a dump option to always force quoting of single line strings?

I'm ok about any option if there are strong arguments why this option is nice & useful for all. But options "just for fun" are rejected to keep api simple.

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

No branches or pull requests

2 participants