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 Calyx #9

Closed
wants to merge 9 commits into from
Closed

YAML Support for Calyx #9

wants to merge 9 commits into from

Conversation

tra38
Copy link
Contributor

@tra38 tra38 commented Mar 20, 2016

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.

While the code will still work fine without this edit, it is due to an undocumented feature mentioned in Issue maetl#8. I want to hedge against the possibility of the code breaking against the future if that undocumented feature also breaks.
@maetl
Copy link
Owner

maetl commented Mar 24, 2016

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
Copy link
Owner

maetl commented Mar 24, 2016

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
Copy link
Contributor Author

tra38 commented Mar 24, 2016

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 and others added 5 commits March 25, 2016 12:13
Very minimal hook for potentially defining grammar rules implicitly
based on the method name rather than manually passing a symbol.

`start` already works like this, but is a hard-coded special case and it
probably doesn’t need to be.
@tra38
Copy link
Contributor Author

tra38 commented Mar 25, 2016

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.

@tra38 tra38 closed this Mar 25, 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