openrewrite / rewrite-migrate-java

OpenRewrite recipes for migrating to newer versions of Java.
Apache License 2.0
111 stars 75 forks source link

Recipe to replace PropertyChangeListener methods to ConfigurationListener Methods #526

Closed BhavanaPidapa closed 2 months ago

BhavanaPidapa commented 3 months ago

What's changed?

image Replacing the addPropertyListener with addConfigurationListener and removePropertyListener with removeConfigurationListener.

What's your motivation?

I used org.openrewrite.java.ChangeMethodName for the recipe RemovedLogManagerPropertyChangeListenerMethods.

Anyone you would like to review specifically?

@timtebeek @cjobinabo

Have you considered any alternatives or workarounds?

Since it was not possible to test the recipe. I created sample files, created a local plugin using ./gradlew publishToMavenLocal then ran it in the Java8 Sample App using mvn rewrite:dryRun

Attaching the rewrite.patch file rewrite.patch

Checklist

timtebeek commented 3 months ago

Hi! Thanks for getting this started; I'm looking over the APIs and wondering if this changes is correct & complete.

Java 8

https://docs.oracle.com/javase/8/docs/api/java/util/logging/LogManager.html#addPropertyChangeListener-java.beans.PropertyChangeListener-

public void addPropertyChangeListener(PropertyChangeListener l) throws SecurityException public void removePropertyChangeListener(PropertyChangeListener l) throws SecurityException

Java 21

https://docs.oracle.com/en/java/javase/21/docs/api/java.logging/java/util/logging/LogManager.html#addConfigurationListener(java.lang.Runnable)

public LogManager addConfigurationListener(Runnable listener) public void removeConfigurationListener(Runnable listener)

Notice how the argument types are different; where one takes an argument that you could write as event -> { ... }, whereas the other takes a runnable () -> { ... }. The changes made here up to now are just changing the method name, but leaving the arguments as they are, which would then break compilation.

Could you explore what more is needed for a complete migration? We likely can't use the event in any capacity inside { ... }, since the replacement method does not have that as input to work with.

BhavanaPidapa commented 3 months ago

Hey @timtebeek, I will investigate more on this issue and I will get back to you.

BhavanaPidapa commented 2 months ago

We don't plan to tackle this using a recipe.