search

maetl / calyx

A Ruby library for generating text with recursive template grammars.
MIT License
62 stars 5 forks source link

YAML Support for Calyx #9

Closed tra38 closed 8 years ago

tra38 commented 8 years ago

This is my first attempt at #7. I have not yet tested to see if this can work for JSON as well, but since YAML is a superset of JSON, it could still work out. I may also need to write some documentation for this feature as well; I just want to show this code first to see whether it matches your original concept.

maetl commented 8 years ago

Looks like a good starting point. However, there are a lot of edge cases with parsing invalid files to consider as there’s no docs/spec for what the YAML format actually looks like, except the implied Hash<Symbol,String> structure from the core library.

In particular, the weighted rule selection needs a bit more thought, as the current Ruby DSL isn’t well suited to translation into YAML format.

maetl commented 8 years ago

You can avoid having to do merge commits like 102ca84 if you git pull --rebase from origin master. Assuming that there’s no conflicts, it should work absolutely fine.

tra38 commented 8 years ago

Thanks for the tip, I'll see if I can clean up my commit history to remove that merge commit. If there are going to be a lot of edge cases with parsing invalid files, then I suspect it may be better to split this YAML/JSON support into its own separate RubyGem. But I'll see how to handle the weighed rule section in the meantime.

EDIT: When I wrote the idea of coming up with a new RubyGem, I was merely thinking of separation of concerns/unique unit tests/making Calyx itself small and lightweight/etc. But I also realize that this might not be an advisable move in the light of the "Leftpad" debacle. And if we want to encourage YAML/JSON use in the future, we may want to bundle in "yaml_load"/"json_load"/etc. rather than just create a dependency everyone has to install anyway.

Actually, this little tangent could be a sign that I should probably just create a separate module within Calyx proper that handles the "conversions" of data formats. (That way, we can have a separate file just for handling the testing of that module.)

tra38 commented 8 years ago

Okay, my attempts to clean up history apparently had made it even more messy and unsuitable for merging this in (having done the cardinal sin of rewriting history and then trying to get the rewritten history to conform with the main branch). I'm closing this pull request and opening up a new one later. This will probably have to happen anyway, as I can see most of this code being able to be generalized to handle both JSON and YAML files.