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

add TokenStreamRewriter to Python2 runtime #1273

Merged
merged 2 commits into from
Nov 22, 2016
Merged

add TokenStreamRewriter to Python2 runtime #1273

merged 2 commits into from
Nov 22, 2016

Conversation

lygav
Copy link
Contributor

@lygav lygav commented Sep 2, 2016

Hello, i've added the TokenStreamRewriter to Python2 runtime.
If all goes well i hope to expand on this contribution.

Please advise about how to integrate into the auto-generated test suit if this is necessary.

P.S: Closes ISSUE #1115

@parrt
Copy link
Member

parrt commented Nov 19, 2016

@ericvergnaud this would be a nice addition. can you bless it?

@ericvergnaud
Copy link
Contributor

blessed

1 similar comment
@ericvergnaud
Copy link
Contributor

blessed

iop.text += prevIop.text
rewrites[i] = None
# look for replaces where iop.index is in range; error
prevReplaces = [op for op in rewrites[:i] if isinstance(rop, TokenStreamRewriter.ReplaceOp)]
Copy link
Contributor

Choose a reason for hiding this comment

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

should this not be is instance(op, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems you are right.
I'll fix that, and will add tests and will review the code again.

@ericvergnaud
Copy link
Contributor

@lygav
Copy link
Contributor Author

lygav commented Nov 20, 2016

@ericvergnaud Thank you for taking time to review the PR.
I'll add the needed tests.

@lygav
Copy link
Contributor Author

lygav commented Nov 20, 2016

@ericvergnaud Hello, although TokenStreamRewriter belongs to the 'runtime' it looks like TokenStreamRewriter has tests only as part of the "tool" testsuite, and only for Java.
The only other implementation is in C# 'runtime' and i just cannot see where is the test for it?
How do you propose to create test for it then:

  • create python test in the Pyhon2 runtime dir (will not run with the build?)
  • try to create TokenStreamDescriptor (or similar name..)

@ericvergnaud
Copy link
Contributor

Hi,
create TokenStreamRewriterDescriptor is the way to go
Eric

@lygav
Copy link
Contributor Author

lygav commented Nov 21, 2016

Thanks Eric, i am going to try to figure this out.

@parrt
Copy link
Member

parrt commented Nov 21, 2016

@lygav I'll move my java tests to runtime.

@parrt
Copy link
Member

parrt commented Nov 21, 2016

Actually, it looks like a major pain to make this general. So far we test ANTLR itself and not runtime lib stuff that is accessed only with code. I'm not sure it's worth the effort for me at this time to make a general mechanism. It would be easier to have runtime tests access just library stuff in the target language. Better than not having tests I guess. @ericvergnaud open to suggestions.

@ericvergnaud
Copy link
Contributor

Ok so let's leave as is and create independent tests in the respective runtimes.
We'll see on the long term whether there is a pattern that is worth sharing across runtimes.

@lygav
Copy link
Contributor Author

lygav commented Nov 30, 2016

Hello @ericvergnaud and @parrt , please look into the pull request with all the tests
replicated after the Java runtime, and fixed errors across the runtime.
Plus further enhancements to make Python2 runtime to be of the same consistent feel with Java (the original) as much as possible.
Also fixed another issue from some time ago (#550), turned out to be simple, i think can be easily replicated into Java.

Thank you

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.

3 participants