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

Fix Rakefile to add __tag__: code for ~lambda test data #121

Merged
merged 2 commits into from
Mar 31, 2021

Conversation

softmoth
Copy link
Contributor

Note: this PR is built on top of #119. Please just look at the 2nd commit, the first is from #119.

This was broken when Ruby switched YAML engines from Syck to Psych
around Ruby 1.9.3. Syck was removed completely around version 2.0.
The code didn't fail, because Psych also implements the add_builtin_type
routine, but it does not recognize !code as a builtin type (because it
isn't; builtins look like !!str or !!map).

Instead, Psych requires a custom class to handle decoding the tagged
YAML item to Ruby.

And then, to get JSON to actually display the object, it is easiest to
just inherit from Hash.

I am not a Ruby developer. I welcome any code changes that would improve this.

@softmoth
Copy link
Contributor Author

I should probably add just a few notes about this for posterity, since there's zero documentation for Psych's add_tag.

Learning resources:

I tried to def initialize(tag_name) and then call add_tag('code', TaggedMap.new('code'), and def yaml_tag: '!' + @tag, but couldn't find a combo that works, and it would have made the code a lot harder to read without providing any real benefits. So I left the !code tag name hard-coded.

I'd rather not have inherited from Hash, but I could not get as_json to be called properly on the object. Calling doc.as_json() fails since Hash doesn't implement it, and it makes no sense to try to do so for this use case. There may well be a better solution for this, but I couldn't find anything. Anyways, I doubt that any proper solution would be as concise and legible, so perhaps it's best as is.

Copy link
Member

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a Ruby coder, either, but the change looks very reasonable to me, at least in terms of the amount of code added. Also highly valuable to have these tags back in the JSON.

Thanks a lot @softmoth!

Having the data all on one line makes it hard for implementations that
rely on the JSON format to locate the source of a failure.

Text files that don't end with a newline can be troublesome or
unacceptable in some contexts.

The only down side is a bit of merging hassle for existing pull
requests. But that happens anyways, and of course the YAML file is
authoritative so JSON conflicts can be ignored in practice.
This was broken when Ruby switched YAML engines from Syck to Psych
around Ruby 1.9.3. Syck was removed completely around version 2.0.
The code didn't fail, because Psych also implements the add_builtin_type
routine, but it does not recognize `!code` as a builtin type (because it
isn't; builtins look like !!str or !!map).

Instead, Psych requires a custom class to handle decoding the tagged
YAML item to Ruby.

And then, to get JSON to actually display the object, it is easiest to
just inherit from `Hash`.
@softmoth
Copy link
Contributor Author

I've force-pushed an update of the specs/*.json to match the latest revision of #119.

@Danappelxx Danappelxx merged commit b1329a2 into mustache:master Mar 31, 2021
@Danappelxx
Copy link
Collaborator

Thanks!

@softmoth softmoth deleted the json-code-tag branch March 31, 2021 04:09
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.

3 participants