-
Notifications
You must be signed in to change notification settings - Fork 3
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
Simplify the generation of parsers & bump antlr to 4.9.3 #58
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🚀
|
||
# Run speedy-antlr-tool to generate parse accelerator | ||
python3 <<EOF | ||
from speedy_antlr_tool import generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing installs speedy_antlr_tool
right now; I would either:
- Add it to the instructions
- Add it as a
pip
dependency inenvironment.yml
- (as you suggested, and I would encourage that) Add a conda feedstock for it and specify it as a
conda
dependency 🏆
P.S. Also see #57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I've proposed adding it to conda-feedstock here. I'll wait within this PR until it's added there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: https://github.com/conda-forge/speedy-antlr-tool-feedstock has arrived. Hence, I propose within this PR to add it to the environment yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimeonStoykovQC could you help with guidance here?
If I recall your ☎️ correctly, we discussed that speedy-antlr-tools=1.3
[#] is needed for this package; however, if I inspect the setup.py
of [#] of this version correctly, it seems to require antlr4-python3-runtime >=4.9.3
(same for v.1.2
...). This is different from our current one (4.9.2
).
Should we remove the addition to the env.yml from this PR; and open a separate one where we try to update antlr to 4.9.3?
Or, more likely, am I missing something fundamentally here?
See the error of our CI test,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to lift the version of the antlr runtime to 4.9.3 and also regenerating the grammars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 ✨
…n_of_parser # Conflicts: # src/pytsql/grammar/cpp_src/tsqlParser.cpp
Done. @SimeonStoykovQC I'm requesting your review given that the extend of this PR changed quite a bit 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the commit history, I see you have tried the whole process again and the tests seem to pass 💯 Thanks a lot, Sven!
Motivation: I tried to extend the grammar manually and found following all single steps to generate the parser(s) tediously. So, I would like to propose bundling the generation within one single Bash script.
Changes: Add a Bash script in the fashion of speedy-antlr-example; update documentation accordingly.
Update: I added
speedy-antlr-tool=1.3.1
to conda-forge, which requiresantlr=4.9.3
(instead of.2
). So @SimeonStoykovQC & I decided to bump the entire usedantlr
version within this PR.