feat: Added OpenRewrite Maven plugin to auto-format java files #9422
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
…rator Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
|
It seems to apply a lot of changes to the code base. So it does not seem to be really following the checkstyle file as expected :-/. |
|
@scholzj Okay. I'll check once and see if I can fix it. |
|
@SaptarshiSarkar12 So, what is the right way to run it? |
|
Yes @scholzj |
|
It would be nice to get rid of the warnings: I seem to be also getting this weird exception: It also still seems to generate a huge number of changes (most seem to be related to various whitespaces etc.): |
|
@scholzj I have asked one of the maintainers of OpenRewrite to explain the cause of this problem. Once I know how to fix these problems, I'll make the requested changes. |
|
I do not think we want it to format any of those. I do not understand why is it even reading these files that have nothing to do with the Java project and are not part of it. I guess they should be ignored?
Well, we do not want to force this plugin on anyone or use this to automatically commit anything. @SaptarshiSarkar12 wanted to contribute this and we are fine with it as long as it is an optional tool that contributors might want to use when they want to. It should be optional for contributors who want to use it. But imagine that it is used by one of ten contributors and reformats all the code contributed by the 9 other contributors. That would create a huge mess in the PRs and commits and complicate reviews. So to make it work for us, it has to format only the things that are actually invalid by our Checkstyle definition which is what is enforced because that is the way to make sure it formats only the newly contributed code. If it reformats code that passes our checkstyle, it would be happening again and again every time it is used. I'm not sure if this is a general feature of the Maven plugin or if it is because of the configuration issue. Also, I do not necessarily say it is a bug -> maybe it just does not work the way we want it. |
|
@timtebeek Is there any way for Open Rewrite plugin to exclude helm files, @scholzj Once the formatting is applied, it will be consistent henceforth. Should we proceed with this? |
Exclusions should work; not sure what you've tried, but it'd be the first time I'm hearing that they don't just work. |
|
@timtebeek No, I have added something like this 👇 <exclusions>
<exclusion>.git/**</exclusion>
<exclusion>*.yaml</exclusion>
<exclusion>*.yml</exclusion>
</exclusions>but still, the plugin checks for files in |
That message does not look like anything OpenRewrite prints, based on this quick search: |
|
@timtebeek Okay. Then, it might be a part of the maven tool. Sorry for the misinterpretation. |
|
@scholzj I have added exclusions to get rid of those warnings. Can you please check if there is still any warnings? |
timtebeek
reviewed
Dec 12, 2023
@SaptarshiSarkar12 Sorry, but now, we cannot keep those changes. The original source code is fully conformant with our checkstyle definition. If the formatter starts changing it for things not enforced by the checkstyle, then it is also not about one-of change but about changes like that being introduced whenever some contributor uses it and reformats the changes done by others not using it. To be honest, this is what I was afraid of right from the beginning. But you said it will do only changes for things enforced by the checkstyle. PS: This seems to take a lot of effort and pull in other people etc. It seems a lot of the problems are caused by us wanting it to fit our workflow rather than adjusting our workflow to the tooling. So just to be clear, if this is deemed not worth the effort, we can just close it. (Just offering this as an option -> if you wanna try to make it work, that is also fine) |
scholzj
reviewed
Dec 12, 2023
timtebeek
reviewed
Dec 12, 2023
Co-authored-by: Tim te Beek <timtebeek@gmail.com> Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
Co-authored-by: Tim te Beek <timtebeek@gmail.com> Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
|
@scholzj I have made the necessary changes for redundant whitespace changes and now the plugin does not make unintended changes. Can you please try this plugin once again and let me know if it is okay now? |
|
@SaptarshiSarkar12 Sorry, I was busy with a release, so I got to this only now.
|
|
@scholzj It's okay. No problem.
|
The point is, that the tool is useful if it does exactly the changes the checkstyle requires. Not more and not less. But it looks like we cannot get it. So as I said, maybe the easiest way is to just close this because the way we do things and the checkstyle rules we use seem to be simply not compatible with the rewrite tool. |
|
@scholzj Yeah, I understand. Let's see if @timtebeek can provide some idea else we can close this. |
|
I don't have any immediate suggestions without potential further substantial time investment indeed. The variables in checkstyle config is something you could open an issue or PR for ; that might then cascade into better behavior here, but that's no guarantee. I agree with this earlier statement
While there might certainly be value in applying OpenRewrite recipes to the code base here, having to limit and bend it to only fix very specific checkstyle issues is likely not the best use of the tool. I'm working on similar efforts; perhaps when those are finished can we reevaluate the approach here. Perhaps best to close this effort until a later point in time due to incompatibility with the existing tooling as configured & desired. Thanks a lot for your efforts here so far @SaptarshiSarkar12 ! Always nice to see how folks apply the tool to OSS projects too. |
|
@timtebeek I see. Then, I'll open an issue for the variables in checkstyle - might be it is a good issue to look into 😄. |
|
@SaptarshiSarkar12 I guess it might be best to close it and we can reopen it later if needed. |
|
@scholzj Okay then I'm closing this for now. |
|
@timtebeek I have opened the issue openrewrite/rewrite-maven-plugin#699. I would love to work on that. |
|
Thanks for all your work on this @SaptarshiSarkar12 @timtebeek |
|
Most Welcome 😄 @scholzj. And, Thank you @timtebeek for the valuable suggestions 🙏! |
Draft
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Enhancement / new feature
Description
Fixes #9394
I have added OpenRewrite's Maven plugin to auto-format java files.
Checklist
Please go through this checklist and make sure all applicable tasks have been done