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

YAML support for emitting tags and style #3608

Closed
wants to merge 2 commits into from
Closed

YAML support for emitting tags and style #3608

wants to merge 2 commits into from

Conversation

trans
Copy link

@trans trans commented Nov 30, 2016

This code adds support for tag and style control to the YAML Emitter class and #to_yaml methods. This addition to the YAML library will make is possible to add tag and style support to YAML.mapping in the future. It is also fully backward compatible, so no worries there.

@trans
Copy link
Author

trans commented Dec 10, 2016

If there is some other way this is supposed to be done, let me know. As far as I can tell, there are only two ways to do it. Either we pass the tag and style in as parameters, as this pull request does, or it gets this info from a "TagSchema" object, passed in though the Emitter initializer, which dictates the details how objects translate into YAML. The later is actually more flexible and probably more in accordance to YAML spec, however it is also more complex. Also, the two approaches aren't mutually exclusive, so taking the simpler approach for now seemed the best way forward.

I can rebase this to one commit, but I am awaiting feed back to finalize.

@asterite
Copy link
Member

@trans I didn't have time to look at this specifically. I think the changes to the YAML::Emitter are OK, but I don't see a strong reason to add these options to Array, Hash and other types.

Eventually we'll add support for different YAML schemas, and there we'll use the tag attribute. But tag attributes should normally be used in relation to a schema, not loosely (as in, use this tag with this array instance)

@trans trans closed this Dec 12, 2016
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.

None yet

2 participants