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

Recipe ReplaceAWTGetPeerMethod in Java 11 Migration #524

Merged
merged 24 commits into from
Aug 9, 2024

Conversation

ranuradh
Copy link
Contributor

@ranuradh ranuradh commented Aug 5, 2024

What's changed?

This PR contains recipe - org.openrewrite.java.migrate.ReplaceAWTGetPeerMethod
Rule:

image

What's your motivation?

This custom recipe replaces the use of getPeer() method in the java.awt.Component, java.awt.Font, and java.awt.MenuComponent classes and direct known subclasses. The occurrence of (component.getPeer() != null) { .. } is replaced with (component.isDisplayable()) and the occurrence of (component.getPeer() instanceof LightweightPeer) is replaced with (component.isLightweight()).

Anyone you would like to review specifically?

@timtebeek
@cjobinabo

Any additional context

Since I couldn't test the recipe I am attaching the rewrite.patch along with the Test that tests using similar invocations
rewrite.patch

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@ranuradh ranuradh added the recipe Recipe requested label Aug 5, 2024
@ranuradh ranuradh self-assigned this Aug 5, 2024
@ranuradh ranuradh requested review from timtebeek and cjobinabo August 5, 2024 20:37
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Hi @ranuradh ! Thanks for yet another recipe. I'm working my way through and I'm wondering what IDE settings you're using. You had checked the box above that you applied the formatter, but when I do the same locally I still got quite a bit of changes, as seen in deb96c3 .

Additionally, I see quite a bit of warnings in my IDE about nullability handling & casts for instance:
image

Ideally we'd have none of those issues on recipes that we add to our catalog, such that any new issues in the future stand out when we maintain these recipes. Would you mind sharing and perhaps adjusting your settings? We use IntelliJ defaults where possible.

@timtebeek timtebeek marked this pull request as draft August 6, 2024 09:38
@ranuradh
Copy link
Contributor Author

ranuradh commented Aug 6, 2024

Hi @ranuradh ! Thanks for yet another recipe. I'm working my way through and I'm wondering what IDE settings you're using. You had checked the box above that you applied the formatter, but when I do the same locally I still got quite a bit of changes, as seen in deb96c3 .

Additionally, I see quite a bit of warnings in my IDE about nullability handling & casts for instance: image

Ideally we'd have none of those issues on recipes that we add to our catalog, such that any new issues in the future stand out when we maintain these recipes. Would you mind sharing and perhaps adjusting your settings? We use IntelliJ defaults where possible.

I am not sure what is going on probably have to update my IntelliJ - I use the following -
image
I will recheck next time

@ranuradh
Copy link
Contributor Author

ranuradh commented Aug 6, 2024

Hi @timtebeek am taking a look at ur suggestions and will update the PR

@ranuradh
Copy link
Contributor Author

ranuradh commented Aug 6, 2024

Hi @timtebeek would like some clarification here:
For the cases where we don't have ControlParentheses. Here is the code snippet for before and after

            """
              class Test {
                  void foo() { 
                      Component1 y = new Component1(); 
                      boolean instance = y.getPeer() instanceof TestDummy;
                      if (instance){
                      }
                      boolean instance1 = y.getPeer() != null;
                      if (instance1){
                      }
                  }
              }
              """,
            """
              class Test {
                  void foo() {             
                      Component1 y = new Component1(); 
                      boolean instance = y.isLightweight();
                      if (instance){
                      }
                      boolean instance1 = isDisplayable();
                      if (instance1){
                      }
                  }
              }
              """
          )
  

In the above case neither visitBinary() nor visitInstantOf is called instead we need to override visitMethodDeclaration.
Also for changes when we find c.getPeer() != null not sure we want our recipe to work on when null != c.getPeer() this gives an LST error as well Here is the link to the - https://docs.oracle.com/en/java/javase/11/migrate/index.html#GUID-0C350BAB-F2C8-409E-AD3E-63831C684A55

@ranuradh ranuradh changed the title Recipe DetectAWTGetPeerMethod in Java 11 Migration Recipe ReplaceAWTGetPeerMethod in Java 11 Migration Aug 7, 2024
@timtebeek
Copy link
Contributor

Hi @ranuradh ; I've pushed a change that will have caused your overridden methods not to be called; now if you set a breakpoint in those other methods they will actually be triggered. As a reminder you are in complete control of the tree traversal, which includes calling super.visitXyz to visit nested elements.

@ranuradh
Copy link
Contributor Author

ranuradh commented Aug 8, 2024

Thanks @timtebeek taking a look now

@timtebeek timtebeek marked this pull request as ready for review August 9, 2024 08:32
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks again @ranuradh ; I've applied a last few finishing touches to expand what's matched, and reduce the code to maintain.

@ranuradh
Copy link
Contributor Author

ranuradh commented Aug 9, 2024

@timtebeek love how you have made the recipe more generic and removed duplications will remember for the next recipe. This recipe looks great and we should be able to merge it post Chuka taking a look There are still build errors though.

@timtebeek
Copy link
Contributor

Thanks for the kind words! Indeed incremental improvements is what we're going for; glad you're still learning from my feedback.

The build issue should be fixed once this change rolls out to a new release.

Copy link
Contributor

@cjobinabo cjobinabo left a comment

Choose a reason for hiding this comment

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

Thanks @timtebeek for working so closely with Anu on this.

@timtebeek timtebeek merged commit cdba5a2 into openrewrite:main Aug 9, 2024
1 of 2 checks passed
@ranuradh ranuradh deleted the recipe_detectAWTGetPeerMethod branch August 9, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants