-
Notifications
You must be signed in to change notification settings - Fork 33
Implement YAML module. #400
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
Conversation
Based off of JSON module. Fixes #399.
|
@blackwinter please merge |
|
So we're not going to discuss the intrinsic relationship between the two modules? Are we content with the duplication? |
You are right. We got confused about where we are in the process. @fsteeg will review. |
fsteeg
left a comment
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.
Yay, YAML support, many thanks! I think the partial duplication of the JSON module is fine, we can always extract a superclass later, when/if we see that we're actually building/fixing stuff in both modules.
Well, let's hope we don't forget... 🙂 |
One could argue that we should wait for supporting a third JSON-like format before we even consider extracting it: https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) |
Based off of JSON module. But not every feature of the JSON implementation can be (easily/reasonably) supported in the YAML implementation (decoder: optional comments, JsonPath; encoder: root value separator, (optional) pretty printing).
Maybe there's room for consolidation, though, with YAML being a superset of JSON. Both pairs of classes could (should?) have a common superclass implementing the actual decoding/encoding.
Fixes #399.