koppor / jabref

Collection of simple for JabRef issues. Please submit PRs to https://github.com/jabRef/jabref/.
https://github.com/jabRef/jabref/
MIT License
8 stars 14 forks source link

Open rewrite #641

Closed Siedlerchr closed 9 months ago

Siedlerchr commented 1 year ago
### Compulsory checks
- [ ] Change in `CHANGELOG.md` described in a way that is understandable for the average user (if applicable)
- [ ] Tests created for changes (if applicable)
- [ ] Manually tested changed features in running JabRef (always required)
- [ ] Screenshots added in PR description (for UI changes)
- [ ] [Checked developer's documentation](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ ] [Checked documentation](https://docs.jabref.org/): Is the information available and up to date? If not, I created an issue at <https://github.com/JabRef/user-documentation/issues> or, even better, I submitted a pull request to the documentation repository.
koppor commented 1 year ago

Interestingly, they claim that their autoformat feature formats according to IntelliJ's default style.

Source: https://docs.openrewrite.org/running-recipes/getting-started#step-3-activate-a-recipe

koppor commented 1 year ago

Currently, this PR enables https://docs.openrewrite.org/reference/recipes/java/cleanup

koppor commented 1 year ago

We should add

Oh, wow, it can also update code to Java 20: https://docs.openrewrite.org/reference/recipes/java/migrate/upgradetojava20

Wholy moly: javax vs. jakarata: https://docs.openrewrite.org/reference/recipes/java/migrate/jakarta

Siedlerchr commented 1 year ago

We should add

* https://docs.openrewrite.org/reference/recipes/java/logging/slf4j/slf4jbestpractices (which includes https://docs.openrewrite.org/reference/recipes/java/logging/slf4j/parameterizedlogging (I thinkk https://docs.openrewrite.org/reference/recipes/java/logging/parameterizedlogging has the cool explanation, but is not for SLF4J))

* https://docs.openrewrite.org/reference/recipes/java/security/javasecuritybestpractices

Oh, wow, it can also update code to Java 20: https://docs.openrewrite.org/reference/recipes/java/migrate/upgradetojava20

Wholy moly: javax vs. jakarata: https://docs.openrewrite.org/reference/recipes/java/migrate/jakarta

We use https://docs.openrewrite.org/running-recipes/popular-recipe-guides/automatically-fix-checkstyle-violations

Siedlerchr commented 1 year ago

I need some manual fixing, for example it removed stuff from the module-info and added some final to variables that would lead to compile errors. Just try it: run ./gradlew fixCheckstyle

koppor commented 1 year ago

I would even hook the rewrite directly before compile. I know that some developers find it strange if their code is changed by a tool. (We had some discussions when applying google java format: https://github.com/sherter/google-java-format-gradle-plugin)

Siedlerchr commented 1 year ago

I did a re-run and separated the commits to show you that it mangles the module-info file!

Siedlerchr commented 1 year ago
koppor commented 1 year ago

I think, this magic does more than "just" fixing configured checkstyle rules. - It seems, all things of https://docs.openrewrite.org/reference/recipes/java/cleanup are done.

Private constructor rewrite seems to be https://docs.openrewrite.org/reference/recipes/java/cleanup/hideutilityclassconstructor

koppor commented 1 year ago

Final variables is https://docs.openrewrite.org/reference/recipes/java/cleanup/finalizelocalvariables

koppor commented 1 year ago

The doc even says to use "Cleanup" plugin for fixing checkstyle violations: https://docs.openrewrite.org/running-recipes/popular-recipe-guides/automatically-fix-checkstyle-violations

I created a list of checkers manually and enabled each checker one by one. There is now one commit per enabled checker (to directly see the differences).

Still 50 to go, maybe I'll finish this week.

Had to raise following issues:

timtebeek commented 1 year ago

Figured chime in here briefly as an OpenRewrite developer; thanks for reporting those issues! I imagine it took you some time to generate all of those changesets. I'll see if I can hook you into https://public.moderne.io/ ; that way you can (optionally) apply changes in seconds, and either commit or start pull requests directly from the interface. And if any issues pop up, you can report those more easily with all the required context. Hope that helps!

koppor commented 1 year ago

@timtebeek That would be great! I completely watched Interstellar - and the 3h movie was finished earlier than me with openrewrite 😅. OK, i watched with 1.25 speed ^^.

timtebeek commented 1 year ago

@timtebeek That would be great! I completely watched Interstellar - and the 3h movie was finished earlier than me with openrewrite sweat_smile. OK, i watched with 1.25 speed ^^.

What gettings are you using in terms of memory limits for running recipes? As my crude first attempts to run locally fail with:

org.gradle.launcher.daemon.server.api.DaemonStoppedException: Gradle build daemon has been stopped: since the JVM garbage collector is thrashing

Siedlerchr commented 1 year ago

Default gradle memory is too low, for pipeline etc we configured org.gradle.jvmargs=-Xmx4096M

koppor commented 1 year ago

Default gradle memory is too low, for pipeline etc we configured org.gradle.jvmargs=-Xmx4096M

https://github.com/JabRef/jabref/blob/main/gradle.properties

org.gradle.vs.watch=true
org.gradle.jvmargs=-Xmx4096M
koppor commented 1 year ago

Now, openRewrite hangs at completion. Think, I will create minimal config applying maybe 5 rules and then go for jabref main. Then, we can test the rules one-by-one.

Making use of the web platform would also be good. Our goal is also to enable newcomers to execute fixCheckstyle so that they concentrate on the content and not on checkstyle violations. Thus, it would be really good if openRewrite worked in our JabRef config.

koppor commented 1 year ago

Refs https://github.com/openrewrite/rewrite-gradle-plugin/issues/199

koppor commented 9 months ago

We included it in JabREf. This PR is absolete.