-
Notifications
You must be signed in to change notification settings - Fork 46
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
Fix issue with special keys that require quoting in mappings #237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
Is this definitely valid YAML? I don't necessarily trust python implementations to be strict about this kind of thing 😆 @Paalon Do you know?
Thanks for taking a look! For context: I deal with hand-written YAML files with keys like: "temperature": { ">": 0.0 } They currently parse just fine. Most of the time, I only need to read them, but occasionally, they need to be read, manipulated, written back out for caching, and then re-read. But with the current writer, they do not successfully round-trip. None of the YAML parsers I have tried are able to parse a block like: >: 0.0 because Note: this PR also only deals with string keys and tries to minimize changes to written YAMLs to affect just those that were previously invalid. Personally, I'd be in favour of just always quoting string keys and skipping the check for special characters, but that might be breaking - though easier to maintain. I can also imagine that the current behaviour of calling |
I have no objection in principle, but I'm just a merge-jocky and don't have much skin in the game. But there's been talking of trying to adhere to a particular spec (#133), and I don't want to be overly permissive. On the other hand, it seems like this is implementing a more strict version of writing rather than changing the permissiveness of reading, so maybe I'm jumping at shadows? I'd love for @GunnarFarneback or @Paalon to weigh in, but if they don't by the end of the week, I'll just YOLO-merge 😆 |
Yes, this is needed and probably correct. I haven't checked if it's the exact list of characters needing quoting but it's always valid to quote and sometimes necessary, so this is definitely an improvement of the existing code. |
@kescobo Do you have an idea of when a new release of YAML.jl will be tagged that includes this fix? 😊 |
How about now-ish? 😛 |
Fixes #236