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

Initial support for String Templates, Unnamed Variables, Switch patte… #4357

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

binis2
Copy link

@binis2 binis2 commented Mar 30, 2024

…rn matching

@binis2
Copy link
Author

binis2 commented Mar 30, 2024

I would appreciate any insights if I'm doing something wrong

@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 1, 2024

Thank you for your hard work, but i can't validate this PR because it mixes several concepts and doesn't take into account all the JP components (for example, I can't see anything about the LPP). I suggest you break down the various concepts for which you want to provide a solution, create PRs containing at least the links to the specifications you are implementing, and cover all the JP components for each concept.
I'm not a specialist in this part, but I'm not entirely sure that the methods generated have been created in the right way. Could you explain how you went about generating these methods?

@binis2
Copy link
Author

binis2 commented Apr 2, 2024

I have no idea what those abrevations are JP (Java Parser?) LPP? Also I have no idea why the tests that are failing are failing. Those tests look really alien to me. I would appreciate any insights.

The approach of implementing some features instead of the full suit is that to give a ground for other contributors to jump in and polish stuff. The project is far behind in terms so I think we can get it up to date as a community. Unfotunately my time is very limited and I have no idea when I will have a spare weekend to work on this.

About the generated part... I reverse enginered it. Tests prove that the code is doing what it is expected to.

@johannescoetzee
Copy link
Contributor

johannescoetzee commented Apr 2, 2024

@binis2 I've started looking into adding java21 support and what you've done here looks great! The company I work for uses JavaParser, so I have a decent chunk of time to work on this now. I'd be happy to collaborate on this :)

My focus will be on the non-preview features (so switch pattern matching and record patterns), but I see you've already made progress on switch pattern matching.

Edit: regarding the LPP, I believe that's the LexicalPreservingPrinter

@binis2
Copy link
Author

binis2 commented Apr 2, 2024

@johannescoetzee Yeah I didn't have time to fully implement pattern matching. What's left will be easy to implement when Record Patterns (JEP440) is handled. I'm willing to implement it thou can't say when.

If someone can point me on the right direction what should I do for LPP to work it would be great.

@@ -708,7 +709,7 @@ TOKEN :
( // TODO: Could these escape sequences be extracted out?
"\\"
(
["s","n","t","b","r","f","\\","'","\""]
["s","n","t","b","r","f","\\","'","\"","{"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add this character here?

Copy link
Author

Choose a reason for hiding this comment

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

String Templates. They are escaped like this - STR."{someExpression()}"

Copy link
Collaborator

@jlerbsc jlerbsc Apr 3, 2024

Choose a reason for hiding this comment

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

For the information, I think this change was made in the definition of 'Escape Sequences'. I'm not sure that this is the most appropriate right for this change.
https://docs.oracle.com/javase/specs/jls/se21/html/jls-3.html#jls-3.10.7

Copy link
Author

Choose a reason for hiding this comment

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

String Templates is still preview feature

Copy link
Author

Choose a reason for hiding this comment

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

Pushed a revision with of String Template with it's own matcher

@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 2, 2024

About the generated part... I reverse enginered it. Tests prove that the code is doing what it is expected to.

Have you used the generators to generate code for visitors, nodes, etc? (https://javaparser.org/code-generation-and-maven-in-javaparser/)

@binis2
Copy link
Author

binis2 commented Apr 3, 2024

About the generated part... I reverse enginered it. Tests prove that the code is doing what it is expected to.

Have you used the generators to generate code for visitors, nodes, etc? (https://javaparser.org/code-generation-and-maven-in-javaparser/)

No. I wrote them manually. Didn't know there is fast way to do it 😁

@binis2
Copy link
Author

binis2 commented Apr 3, 2024

It seems that the JavaToken discrapancies was the issue with LPP so now all tests are passing.

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

3 participants