JavaScript support for a Hermes WDL grammar #94

Merged
merged 3 commits into from Feb 22, 2017

Conversation

Projects
None yet
2 participants
Contributor

sidoruka commented Feb 21, 2017

General idea

Currently Hermes WDL grammar fully supports compilers generation only for Python and Java languages.

We would like to propose improved JavaScript support for a Hermes WDL grammar and hope these improvements to be useful for generating a WDL compiler for JavaScript (JS).

Changes summary for grammar.hgr

  • Adding JS regex
    • JS uses [\s\S] instead of '.' because JS doesn't have DOTALL flag for regex
  • Adding JS enum support for wf_output
  • Adding JS enum suport for task_fqn
  • Adding JS support for generation (Lexer overloaded functions)
    • Renaming unescape to wdl_unescape because of name conflict with JS native function

WDL Parser.JS, tests and sample code

The generated WDL parser, together with tests and sample code are added to javascript folder

  • javascript
    • tests
      • cases a set of WDL files and expected ast files to use for test runs (WDL files are copied from pywdl repository)
      • parser-test.js tests to compare generated ast with expected one (mocha framework is used for testing)
    • sample.js example of wdl_parser.js usage
    • wdl_parser.js parser that was generated from new grammar.hgr
    • README.md describes sample.js usage and running tests
Member

geoffjentry commented Feb 21, 2017

@sidoruka This is great, thanks! I've got one note and one question.

  • Q: To what extent will we need to maintain the grammar file for the JS stuff? If we are making changes to the grammar in general will we need to muck around w/ the JS portions?

  • Note: We'd been using a grammar.hgr in another repo and it diverged slightly. It's being updated in #95

Member

geoffjentry commented Feb 21, 2017

Took a closer look. We'll need to verify the unescape change doesn't break wdl4s, at first glance I don't think it'll be an issue but I'll check.

@@ -0,0 +1 @@
+tests/node_modules
@geoffjentry

geoffjentry Feb 22, 2017 edited

Member

add a \n here please (and in all the other files not ending in one)

Member

geoffjentry commented Feb 22, 2017

@sidoruka Hi - can you rebase this PR? It'll pick up the updated grammar.hgr file

Contributor

sidoruka commented Feb 22, 2017 edited

@geoffjentry thanks for the review,

  • Branch is rebased to incorporate #95
  • New lines added, sorry for that

Regarding your question, I would say that there are two things that could affect JS implementation and require its support:

  • Possible JS naming conflicts (this is a reason for unescape -> wdl_unescape change). But if some kind of prefixes will be used in future (it seems to be useful anyway) then I don't think much JS - specific problems could arise
  • If a new token is added it also need to be propagated for JS (regexps). This issue does not affect syntax stage, as it does not depend on any language.

Also I'll try to build wdl4s parser from new grammar and run tests from wdl4s repository to make sure that nothing is broken.

Member

geoffjentry commented Feb 22, 2017 edited by briandmaher

@sidoruka If building wdl4s is a pain for you don't worry about it, it was something I was going to try to do when i got a chance over the next day or so but I certainly wouldn't mind if someone else did it ;)

Your PR did make me think that we could just autogenerate parsers for all of the Hermes targets on update to grammar.hgr and store them here, the one thing we really aren't equipped to do is to validate that they work (outside of the java one). I need to see what folks here think about that. As someone who is obviously interested in a non-Java parser, would a "we think this works, it might break - but if you're willing to help us fix it we'll work with you to do so" policy be agreeable for you?

As for the status of this PR I'm 👍 in general - behind the scenes it raised a couple of infrastructural/administrative issues which we'd been putting off handling so I am working to get that squared away first.

Approved with PullApprove

Contributor

sidoruka commented Feb 22, 2017 edited

@geoffjentry I've tried building java parser from new grammar and running tests from wdl4s. To make things easier, this was done in a fork using travis ci. As a result - 3 tests failed.
Latest commit to this PR introduces minor changes to grammar that fix failed tests. Current version works correctly with wdl4s (see build log ) and JS as well.

Regarding your question and a proposed policy - it is ok to me.

Member

geoffjentry commented Feb 22, 2017

@sidoruka Perfect, thanks. I'm going to merge this as-is for now although note that we'll likely move things around a bit in the near future. I'll leave the structure of what you have for the JS subdir intact, it just might move to e.g. a parsers subdir or something like that.

@geoffjentry geoffjentry merged commit 86f4916 into broadinstitute:develop Feb 22, 2017

1 check was pending

code-review/pullapprove Approval required by 1 of: cjllanwarne, Horneth, katevoss, kcibul, kshakir, mcovarr, ruchim
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment