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

Provide Javascript port of TokenStreamRewriter #3560

Merged
merged 1 commit into from
Feb 26, 2023

Conversation

eirikur-grid
Copy link
Contributor

@eirikur-grid eirikur-grid commented Feb 26, 2022

What

Add a JavaScript port of the TokenStreamRewriter that includes most, but not all, of the methods from the Java implementation.

Why

So that JavaScript developers can benefit from having it to implement all sorts of neat things (refactorings, translation etc.)

Scope

Included

  • insertAfter
  • insertBefore
  • replace
  • delete
  • getText
  • getProgram
  • getTokenStream

Omitted

The following methods were omitted because
a) I couldn't find any tests for them and
b) their utility is not immediately obvious to me.

  • rollback
  • deleteProgram
  • getLastRewriteTokenIndex

If someone needs them then we can obviously add support for them in a subsequent PR.

How

By

  • Porting most of the TokenStreamRewriter class
  • Porting tests and supporting fixtures (there are a couple of toy grammars used by the tests)
  • Adding more tests to get close to 100% coverage

Credit to @sharwell for having ported the TokenStreamRewriter and the corresponding unit tests to TypeScript (see https://github.com/tunnelvisionlabs/antlr4ts). That has made creating a JavaScript port relatively straight-forward.

Drive-bys

  • For added convenience, I added a Makefile within the spec folder to automate the code generation from the toy grammars.
  • I added a dev dependency for c8 and a run target in package.json that generates a code coverage report. This I used to find uncovered sections in the TokenStreamRewriter.
  • I added an explicit dev dependency on eslint (seems it was transitive before) and upgraded it to the latest version. I then tweaked the parser options in order for it to recognize class static fields. See this SO thread for more context.
  • I made a few additions to .gitignore to exclude autogenerated files that don't need to be placed under source control.
  • I added an npm run lint target

Closes #1767

@eirikur-grid eirikur-grid force-pushed the javascript-token-stream-rewriter branch from 9d078cd to 0d0987f Compare May 3, 2022 20:05
@eirikur-grid eirikur-grid changed the base branch from master to dev July 27, 2022 22:59
@eirikur-grid eirikur-grid force-pushed the javascript-token-stream-rewriter branch 2 times, most recently from 18dbf92 to 8f82b2b Compare July 28, 2022 11:04
@eirikur-grid eirikur-grid changed the title WiP: Provide Javascript port of TokenStreamRewriter Provide Javascript port of TokenStreamRewriter Jul 30, 2022
@eirikur-grid eirikur-grid marked this pull request as ready for review July 30, 2022 12:09
@eirikur-grid eirikur-grid force-pushed the javascript-token-stream-rewriter branch from cf4d564 to fba190e Compare July 30, 2022 12:10
@@ -148,7 +152,7 @@ export default class BufferedTokenStream extends TokenStream {
return n;
}

// Get all tokens from start..stop inclusively///
// Get all tokens from start..stop inclusively///
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Automatic code formatting kicked in on this file.

@ericvergnaud
Copy link
Contributor

@eirikur-grid sorry if it's taken us so long to look at this
would you be able to rebase from this PR #4027
and provide an associated d.ts file

@eirikur-grid
Copy link
Contributor Author

This week is going to be quite hectic for me. I'll take a look shortly after Christmas.

@eirikur-grid eirikur-grid force-pushed the javascript-token-stream-rewriter branch 2 times, most recently from 9e5cb39 to bb0a42b Compare December 27, 2022 19:52
@eirikur-grid
Copy link
Contributor Author

@ericvergnaud I've rebased and added the type definitions file.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Feb 14, 2023

@eirikur-grid we're about to release, and this is a good time to finalize things.
Could you:

  • squash your commits and gpg-sign off the squashed commit (this replaces the contributors.txt strategy)
  • move your changes in index.js to index.node.js and index.web.js
  • add an export in index.d.ts
  • move your spec/* files under spec/rewriter/* to clarify their purpose
  • not mandatory but a .ts test would also be welcome (to validate the ts api)

If you can do that rapidly enough, this PR will make it into the next release

@eirikur-grid
Copy link
Contributor Author

@eirikur-grid we're about to release, and this is a good time to finalize things. Could you:

* squash your commits and gpg-sign off the squashed commit (this replaces the contributors.txt strategy)

* move your changes in index.js to index.node.js and index.web.js

* add an export in index.d.ts

* move your spec/* files under spec/rewriter/* to clarify their purpose

* not mandatory but a .ts test would also be welcome (to validate the ts api)

If you can do that rapidly enough, this PR will make it into the next release

You caught me at a very busy time, hence the late reply. I should be able to get around to this before the end of this week.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Feb 19, 2023 via email

Signed-off-by: Eiríkur Fannar Torfason <[email protected]>
@eirikur-grid eirikur-grid force-pushed the javascript-token-stream-rewriter branch from e51de80 to c9995f9 Compare February 24, 2023 18:46
@eirikur-grid
Copy link
Contributor Author

@ericvergnaud I believe I've addressed all remarks.

@@ -0,0 +1,16 @@
ANTLR_VERSION = 4.12.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a small problem here
The purpose if the CI is not to test against the latest official version, rather against the version being tested.
So the makefile should use the tool from the local maven repo, not from the antlr web site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, how do I do that? Are there any examples within the repo that I can follow? I can see that the Java tests are capable of generating lexers/parsers from a grammar at run-time but what about other languages?

When I started working on this, I found Python tests using generated code (runtime/Python2/tests/mocks/TestLexer.py) that has been commited to the repo, so I assumed this was OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my contribution ends here. Over a year ago, I found myself needing a JavaScript version of the TokenStreamRewriter. That is no longer the case, so I have no vested interest in putting more effort into this.

Copy link
Contributor

@ericvergnaud ericvergnaud Feb 25, 2023

Choose a reason for hiding this comment

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

All generic grammar tests generate parsers at CI time, but they are run from java.

Every GitHub action runs the "Build ANTLR with Maven" step, which builds the tool and registers it in the local maven repo, which should be found at ~/m2/repository/org/antlr/antlr4/${version}/antlr4-${version}-complete.jar.

You are correct in observing that many target-specific tests do not follow the expected policy, but let's not make things worse :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to hear that, and apologies for taking so long to consider this PR. I hope your interest will grow again soon. This is useful work for the community.

@@ -0,0 +1,37 @@
// Generated from abc.g4 by ANTLR 4.12.0
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be regenerated during tests or it will fail with a version mismatch

@@ -0,0 +1,50 @@
// Generated from calc.g4 by ANTLR 4.12.0
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@ericvergnaud
Copy link
Contributor

@parrt ready to merge, I suggest we deal with the versioning issue at a global level since it affects all targets

@parrt
Copy link
Member

parrt commented Feb 26, 2023

Re "derived from antlr4ts", license appears to be "The ANTLR Project" so we're good there: https://github.com/tunnelvisionlabs/antlr4ts/blob/master/LICENSE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TokenStreamRewriter for javascript?
3 participants