Multiplatform CQL parser #1443
Multiplatform CQL parser #1443
Conversation
JPercival
commented
Nov 18, 2024
JPercival
commented
- Change the output of ANTLR to Kotlin, rather than Java
- Use Kotlin multiplatform to generate Java and JS lexer/parsers for CQL
- Add a demo application showing the parser working in JS
- See this PR for details on how to test the app: Add demo UI for CQL parser #1439
* Try out jsRun * Compile Kotlin to JS with type definitions. Export parseToTree method.
* small cleanup * fix the jvm toolchain version --------- Co-authored-by: Anton Vasetenkov <[email protected]>
* Add demo for the UI that uses Kotlin/JS- and Kotlin/WASM-based CQL parser * Tweak config * Update README.md * Update docs * Update docs
|
Formatting check succeeded! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-kotlin #1443 +/- ##
====================================================
+ Coverage 64.47% 66.02% +1.54%
- Complexity 2180 3001 +821
====================================================
Files 492 486 -6
Lines 27972 28447 +475
Branches 5537 5463 -74
====================================================
+ Hits 18036 18782 +746
+ Misses 7701 7467 -234
+ Partials 2235 2198 -37 ☔ View full report in Codecov by Sentry. |
…1445) Remove redundant override
There was a problem hiding this comment.
This is way cool. The JS parser demo app worked great. I also ran a regression test of the Java translator against a set of CQL libraries and everything came out clean.
I left a few questions and minor comments/suggestions for your consideration. Some of it we discussed a bit already (like Gradle 8.11) but I had already made the comment by then, so I left it for posterity.
| /** | ||
| * GrammarTest ensures that the grammar (and generated parsers) work as expected. If non-compatible changes are made | ||
| * to the grammar, these tests should fail. If the change is intentional, modify the tests to pass-- otherwise, fix | ||
| * the grammar. | ||
| */ |
There was a problem hiding this comment.
How do you run this test? As far as I can tell, it's not included in the normal test run.
| * to the grammar, these tests should fail. If the change is intentional, modify the tests to pass-- otherwise, fix | ||
| * the grammar. | ||
| */ | ||
| internal class JsTest { |
There was a problem hiding this comment.
Except for the classname, this file is exactly the same as the GrammarTest.kt file in commonTest. Is it necessary to have both? If so, is there a better way to re-use code (e.g., can JsTest extend GrammarTest or literally call the test functions from GrammarTest)?
If we need both files and both files each need to repeat all the test code verbatim, I'd suggest that you add a comment in each one pointing to the other. I assume that if you need to make a change in one, you'll probably want to make the same change in the other -- so we should make it hard to forget to do that.
| cqlLexer lexer = new cqlLexer(input); | ||
| CommonTokenStream tokens = new CommonTokenStream(lexer); | ||
| tokens.fill(); | ||
| cqlParser parser = new cqlParser(tokens); | ||
| parser.setBuildParseTree(true); | ||
| ParserRuleContext tree = parser.library(); | ||
| Trees.inspect(tree, parser); | ||
| // Trees.inspect(tree, parser); |
There was a problem hiding this comment.
If we don't call Trees.inspect then what is the point of this file? It builds the tree but then does nothing with it. Should it be modified to at least print a text tree to the console?
There was a problem hiding this comment.
Yeah, good idea. Because of this little gui application we had inadvertently added some GUI dependencies to the compiler. That was discovered when we refactored to make it multiplatform. Another option is to simple break this out into its own "tree viewer" tool in the tools subproject, alongside the formatter.
Src/java/buildSrc/src/main/kotlin/cql.kotlin-multiplatform-conventions.gradle.kts
Show resolved
Hide resolved
|
Very cool to see all of this! |
|
