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

Ditching antlr4ts #103

Merged
merged 9 commits into from
Dec 21, 2023
Merged

Ditching antlr4ts #103

merged 9 commits into from
Dec 21, 2023

Conversation

Janther
Copy link
Collaborator

@Janther Janther commented Dec 19, 2023

Removing antlr4ts that hasn't been updated in 3 years and relying in the official antlr4 Typescript target.

Main changes.

  • generated names of function that return an array are now prepended by _list
  • instead of returning undefined now generated functions return null
  • loc and range attributes follow the naming standard

Some details:

  • src/ASTBuilder.ts has a check for ErrorNode but antlr4 doesn't expose this class nor is this check triggered in any tests.
  • to build for node, I had to inject a fix for import.meta.url that is wrongly processed by esbuild

The built size is cut in half thanks to this.

@Janther Janther requested a review from fvictorio December 19, 2023 18:16
@Janther Janther force-pushed the ditching_antlr4ts branch 2 times, most recently from b84b4db to 7322745 Compare December 19, 2023 18:38

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this, I didn't know that there was an official typescript target now!

@@ -38,6 +38,8 @@ jobs:
cache: 'npm'
- name: Install
run: npm install
- name: Install ANTLR4
run: pip install antlr4-tools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind updating the https://github.com/solidity-parser/parser/blob/master/DEVELOPING.md document? I know it wasn't up to date to begin with, but I think it would be a good idea to do it now that we (again) use something other than npm.

Also, I think we can remove the scripts/antlr.sh file (mentioned in that outdated document).

sourcemap: true,
format: 'cjs',
platform: 'node',
target: 'node16',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I hadn't updated this in a while.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…build on prettier-plugin-solidity
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