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 doAfterVisit(UnnecessaryParentheses) to generated recipes #22

Merged
merged 4 commits into from
Aug 18, 2023

Conversation

timtebeek
Copy link
Contributor

What's your motivation?

Until we update the JavaTemplate to detect and insert parentheses around for instance binary expressions, we need a quick way to move along openrewrite/rewrite-migrate-java#271 ; With this change we're able to add parentheses to the templates defined there, which are then cleaned up through UnnecessaryParentheses.

Have you considered any alternatives or workarounds?

We could always add parentheses in the JavaTemplate Strings produced by TemplateProcessor here; that might be a bit excessive, but would prevent us from having to add those parentheses to rewrite-migrate-java.

Any additional context

@timtebeek timtebeek added bug Something isn't working enhancement New feature or request labels Aug 17, 2023
@timtebeek timtebeek self-assigned this Aug 17, 2023
@knutwannheden
Copy link
Contributor

Another alternative would be for us to determine whether expressions are required or not at "insertion time"; i.e. we could add some kind of maybeParenthesize(Expression, Cursor) utility method. I imagine this could be useful in other recipes as well. But generating a doAfterVisit() is simpler of course.

My only worry with doAfterVisit() is that if a lot of modifications are made, then for each such modification we schedule one of these visitors. I wonder if we should either add an overload or another method (like doOnceAfterVisit()) or add a marker interface to visitors. For maybeAddImport() and maybeRemoveImport() we hand-coded this logic.

Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

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

Left one small comment. After that I am happy.

@timtebeek timtebeek merged commit ad66be3 into main Aug 18, 2023
@timtebeek timtebeek deleted the do_after_visit_UnnecessaryParentheses branch August 18, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants