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

Special characters shouldn't force double quoting for multi-line strings #246

Conversation

ahgittin
Copy link

@ahgittin ahgittin commented Mar 5, 2021

the logic added in c427465 for #116 to force double-quoting of strings should only apply to single-line strings. the quoting of multi-line strings look naff, and because they take literal mode, the characters aren't special and the quotes are not necessary.

the logic added in c427465 for FasterXML#116 to force double-quoting of strings should only apply to single-line strings.
the quoting of multi-line strings look naff, and because they take literal mode, the characters aren't special and the quotes are not necessary.
@ahgittin
Copy link
Author

ahgittin commented Mar 5, 2021

BTW ideally the characters []{}, should only be treated as special if used in a key or in a flow collection [1]. They are permitted in strings in plain style when used as a value in a map. Not working on that now but thought I'd raise it.

[1] https://yaml.org/spec/1.2/spec.html#id2788859

@cowtowncoder
Copy link
Member

Apologies for slow follow-up here. Yes, I think you are right.

On bracket/curly-braces case... I suspect one should only have to worry about opening ones, too? Since it's only pairs that matter, and so if there's no { or [, it should not matter if there's ] or }?
Except that perhaps that, too, depends on Key/Object-value/Array-Value difference.

Now, it is possible to distinguish key handling (it's separate method already); and we know from context Array/Object difference as well (so could/should be moved to separate method too?).

With that I assume that linefeeds should still force quoting of keys for sure.

Anyway. I hope to get back to this one soon and hope we can figure out improvements. Quoting/escaping has proven to be a rather problematic thing....

@cowtowncoder
Copy link
Member

Patch itself is simple: but it would probable make sense to add this to handling of StringQuotingChecker. Not 100% sure how the flow should go. I also realize there is bit of backwards-compatibility challenge, but since this was only introduced in 2.12, I think there's slightly lower risk of breakage if we have to change something.

Specifically I see that no info is passed on context (Array vs Object vs Root value (not sure if we have root values in YAML but theoretically)). Passing JsonWriteContext as extra argument would give that information for code that cares; or, alternatively could split needToQuoteValue() into 2 methods.

@cowtowncoder
Copy link
Member

Sigh. Quoting/style checking in 2.12.x is a royal mess.

I think this specific change makes sense, and I think I'll actually backport it in 2.12.3.

But for 2.13 (and esp. 3.0) this really should be rethought. I also realize that check for key quoting is not considering "funny characters" at all, which is probably not correct... although not sure what SnakeYAML at low level will do.

@cowtowncoder
Copy link
Member

As per above comment, will manually merge suggested fix for 2.12. Also filed #252 to improve StringQuotingChecker so that users could conceivably override handling to their liking (currently abstraction is too limited to allow that).

@cowtowncoder cowtowncoder changed the title special characters shouldn't force double quoting for multi-line strings Special characters shouldn't force double quoting for multi-line strings Apr 9, 2021
@cowtowncoder cowtowncoder added this to the 2.12.3 milestone Apr 9, 2021
cowtowncoder added a commit that referenced this pull request Apr 9, 2021
@cowtowncoder
Copy link
Member

As per above noted, merged manually in 2.12 branch, for inclusion 2.12.3. Thanks!

@ahgittin
Copy link
Author

I agree with all your points -- esp that keys do still need quoted and that makes life more painful. Thanks for fixing this!

@cowtowncoder
Copy link
Member

@ahgittin Thank you for the fix! I will be releasing 2.12.3 patch today.

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.

2 participants