rewrite-maven-plugin

rewrite-maven-plugin copied to clipboard

What's changed?

Fixes #699

Anyone you would like to review specifically?

@timtebeek

Checklist

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

@timtebeek I have created a blank test class. Can you please tell how I should test for the working of the checkstyle config file variables?

Hi @SaptarshiSarkar12 ! You'll want to create matching files in https://github.com/openrewrite/rewrite-maven-plugin/tree/main/src/test/resources-its/org/openrewrite/maven as you can see there for other unit tests too. All those tests use https://khmarbaise.github.io/maven-it-extension/itf-documentation/usersguide/usersguide.html, which might help to read up on as well. Thanks for helping out here!

Hi @timtebeek :wave:! Thank you for pointing to the proper resources. I was thinking that this project was using basic Junit Test structure. So, I couldn't understand what IT meant and how did the test classes get executed. But, now I know about the Junit extension that this project is using. Thank you for that. If I face any issue, I'll post those problems here :smile:.

@timtebeek As a side question, is there a general rule of this project to assign the PRs to the contributors? I have seen projects assign the issues to the contributors. If this is a silly question, please do not mind :upside_down_face:. I asked this out of curiosity :smile:.

As a side question, is there a general rule of this project to assign the PRs to the contributors? I have seen projects assign the issues to the contributors. If this is a silly question, please do not mind 🙃. I asked this out of curiosity 😄.

We typically assign the issue or pull request to whoever is working on it, also if they are community contributors. That way it's easy to see at a glance who (if anyone) is working on something from our project board. Sometimes when folks then later indicate they do not have the time or expertise to finish something, we can unassign them to indicate someone else needs to step up. Not a silly question at all; hope my answer helps you understand!

Yeah, I understood. Thank you for the explanation @timtebeek :grin:!

@timtebeek Which recipe should I use in the plugin configuration for tests - the custom configuration that I made in https://github.com/strimzi/strimzi-kafka-operator/pull/9422 or the one described in the docs of open-rewrite?

@timtebeek Which recipe should I use in the plugin configuration for tests - the custom configuration that I made in strimzi/strimzi-kafka-operator#9422 or the one described in the docs of open-rewrite?

org.openrewrite.staticanalysis.CodeCleanup should work well enough I suppose. :)

Yeah, it should :smile:.

@timtebeek I have added a small test project in the resource-its folder as well as I have added a method in the test directory. Can you please check if those changes are right? Can you please tell me what are the next steps to do?

@timtebeek I have added a small test project in the resource-its folder as well as I have added a method in the test directory. Can you please check if those changes are right? Can you please tell me what are the next steps to do?

@timtebeek Have you checked?

@timtebeek Please tell me what changes are required.

Hi @SaptarshiSarkar12 ; Appreciate your enthusiasm to get this going! I've not had time yet to dive into what would be needed next; I did push two small changes to hopefully get the test going, after seeing the tests previously failed with

[ERROR] org.openrewrite.maven.CheckstylePropertiesTest.checkstyleProperties(MavenExecutionResult)  Time elapsed: 0.035 s  <<< ERROR!
com.soebes.itf.jupiter.extension.exceptions.PathUtilException: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:51)
	at com.soebes.itf.jupiter.extension.PathUtils.copyDirectoryRecursively(PathUtils.java:109)
	at com.soebes.itf.jupiter.extension.MavenITExtension.beforeEach(MavenITExtension.java:133)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixCopyFile.copy(UnixCopyFile.java:548)
	at java.base/sun.nio.fs.UnixFileSystemProvider.copy(UnixFileSystemProvider.java:257)
	at java.base/java.nio.file.Files.copy(Files.java:1305)
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:49)
	... 4 more

Sadly now there's a different puzzling error.

Now finally we fail with

,  [STDOUT] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.1:check (validate) on project checkstyle_properties: Failed during checkstyle execution: Failed during checkstyle configuration: unable to parse configuration stream: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Property ${checkstyle.suppressions.file} has not been set -> [Help 1]

Time to work in this earlier suggestion https://github.com/openrewrite/rewrite-maven-plugin/issues/699#issuecomment-1872536382 ; Would you want to try your hand at that @SaptarshiSarkar12 ?

Hi @SaptarshiSarkar12 ; Appreciate your enthusiasm to get this going! I've not had time yet to dive into what would be needed next; I did push two small changes to hopefully get the test going, after seeing the tests previously failed with

[ERROR] org.openrewrite.maven.CheckstylePropertiesTest.checkstyleProperties(MavenExecutionResult)  Time elapsed: 0.035 s  <<< ERROR!
com.soebes.itf.jupiter.extension.exceptions.PathUtilException: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:51)
	at com.soebes.itf.jupiter.extension.PathUtils.copyDirectoryRecursively(PathUtils.java:109)
	at com.soebes.itf.jupiter.extension.MavenITExtension.beforeEach(MavenITExtension.java:133)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixCopyFile.copy(UnixCopyFile.java:548)
	at java.base/sun.nio.fs.UnixFileSystemProvider.copy(UnixFileSystemProvider.java:257)
	at java.base/java.nio.file.Files.copy(Files.java:1305)
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:49)
	... 4 more

Sadly now there's a different puzzling error.

@timtebeek Thank you for spending your valuable time! Sorry, I was very busy the last three days. So, I couldn't reply.

Now finally we fail with

,  [STDOUT] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.1:check (validate) on project checkstyle_properties: Failed during checkstyle execution: Failed during checkstyle configuration: unable to parse configuration stream: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Property ${checkstyle.suppressions.file} has not been set -> [Help 1]

Time to work in this earlier suggestion #699 (comment) ; Would you want to try your hand at that @SaptarshiSarkar12 ?

I think I have not provided any checkstyle suppressions file. So, that's why it is showing error. I'll try to remove that line and re-run the tests. @timtebeek What do you think?

Sounds good! indeed best to make the test example be something that ought to work (no missing config or files), and then work out what we need to do to make it work.

Yeah, right. Will do it soon :smile:. Thank you @timtebeek :grin:!

@timtebeek I have added all the missing files. We are now ready to test our ideas :grin:.

@timtebeek Please tell me if any further changes in the missing files are required 😃. If not, then can you please guide me how to change the way the checkstyle properties are handled by open rewrite Maven plugin?

@timtebeek Please reply 😃.

Hi yes sorry I'm swamped with work and other reviews at the moment. This PR seems like it would take more effort on my side to help guide you through, which I'm having trouble fitting in on the short term.

There's were some earlier hints here and here, but I don't know if that's enough for you to try your hand at changing the actual code too in addition to the test you've already added.

@timtebeek I was busy in some other works the last month. I went through some parts of the ConfigurableRewriteMojo java class and learnt how we can retrieve the required data from the pom file. I added a <CheckstyleProperties/> tag in pom file and a method to retrieve the same. But, got some error as shown below :point_down:

Do you know how to fix that? Secondly, can you please check if the way to replace the checkstyle properties with the actual value is correct? https://github.com/openrewrite/rewrite-maven-plugin/blob/11bca1e6e1c14adc8004e4dd4f5952a189259795/src/main/java/org/openrewrite/maven/ConfigurableRewriteMojo.java#L265-L276

@timtebeek Can you please help me to fix the above problem?

Based on the screenshot it looks like you need to rename your folder from CheckstylePropertiesTest to CheckstylePropertiesIT.

@knutwannheden Thank you for the help :smile:! It worked. But, there's another error coming up. It says about any itf-repo folder which I don't think exist in the project. Do you know how to fix this error?

I've renamed the test resources to line up with the test class name, as required by the integration test framework.

Unfortunately it seems there's now some conflicts with the main branch, after merging this recent support for inline config:

• https://github.com/openrewrite/rewrite-maven-plugin/pull/768

I've renamed the test resources to line up with the test class name, as required by the integration test framework.

Thank you :smile:. But, there's another error coming up as posted in this comment. Can you help me fix that error?

Unfortunately it seems there's now some conflicts with the main branch, after merging this recent support for inline config:

• Parse inline checkstyle config from maven-checkstyle-plugin #768

I have resolved the conflicts and merged the changes successfully. Thank you for informing @timtebeek.

@timtebeek Can you please tell me how do I fix that error?

I've pushed up a change to fix the test for now @SaptarshiSarkar12 ; hope that helps you on your way!