jetty.project icon indicating copy to clipboard operation
jetty.project copied to clipboard

Add maven plugins to automatically reformat Java code

Open kohlschuetter opened this issue 2 years ago • 9 comments

Jetty version(s) 12

Enhancement Description Contributing to Jetty could be simplified by providing simple means to automatically reformat code from the command-line.

kohlschuetter avatar Feb 02 '23 22:02 kohlschuetter

@kohlschuetter you've seen the jetty-codestyle-eclipse-ide.xml and the jetty-codestyle-intellij.xml files in the build or build-resources (depending on which version of jetty you're dealing with) directory?

janbartel avatar Feb 02 '23 22:02 janbartel

@janbartel Yes, see my pull request https://github.com/eclipse/jetty.project/pull/9304, which uses jetty-codestyle-eclipse-ide.xml to reformat code from Maven.

kohlschuetter avatar Feb 02 '23 22:02 kohlschuetter

Snap - looks like your PR and my post crossed in the ether.

Not sure I'm keen on accepting commits of code that don't meet the coding standards and then having to have another set of commits to fix it up. We already provide the Intellij and Eclipse codestyle files to make it easier to integrate with IDEs, and then we run checkstyle to ensure the code meets that format both in the IDE and then as a check during build.

janbartel avatar Feb 02 '23 23:02 janbartel

I like the idea about being able to use a maven plugin to reformat some code and not being dependant on IDE (especially now we have configuration for ONLY 2 IDEs). Very more convenient to just run a command line rather than editing every file with the IDE (just because of a missing space :) ).

BUT the current changes in the PR just reformat everything which it shouldn’t and don't get pass checkstyle:check. I think we would be happy to consider such improvement if:

  • using the plugin still makes checkstyle:check passing
  • the reformat plugin should run before checkstyle:check during the build lifecycle.

And yes I'm sorry I feel your pain about so many tools to reformat/check code but none agree on single configuration format for all (lucky we are not using spotless 🤣)

olamy avatar Feb 03 '23 01:02 olamy

@olamy Yes, the reformatter in my pull request uses the Eclipse formatter with the Jetty formatting rules for Eclipse IDE, so there's no additional configuration necessary.

You mention that the formatted code doesn't pass a checkstyle:check. This is due to a misconfiguration in build/build-resources/jetty-codestyle-eclipse-ide.xml itself. You can verify this by using Jetty rules in the Eclipse IDE for the following file, for example: jetty-utils's SniX509ExtendedKeyManager.java.

The fact that the Maven script reformats everything (in less than 20 seconds on my Mac) is the point. Why shouldn't it? It found several files not adhering to Jetty's own formatting rules, and it even found inconsistencies between these rules and the checkstyle configuration.

kohlschuetter avatar Feb 03 '23 10:02 kohlschuetter

The checkstyle rules are sane, that is the set of golden formatting rules to adhere to.

We might need to update the Eclipse IDE formatter ruleset, but the last time we tried the Eclipse IDE couldn't handle it (this was back in mid 2019). It was far to simplistic to handle the rules we established. (Heck it is not even capable of adhering to the Google style guide, or the Java style guide, or the K&R style guide accurately, so I doesn't surprise me). There is only 1 committer left on the project using the Eclipse IDE (everyone else is on other IDEs with far more robust and capable formatters).

Personally, I wouldn't base anything off any Eclipse formatter ruleset, it's historically been incapable of doing things properly. Nearly everything you see committed in the Eclipse project is either formatted with other IDEs (or if the commit came from an Eclipse IDE user it is manually fixed)

joakime avatar Feb 03 '23 11:02 joakime

I'm not aware of any Maven plugin that supports IntelliJ formatting rules, and it looks like revelc's formatter isn't going to get it any time soon [1].

I'd say, yes, please fix the eclipse formatting rules for your project, because they're obviously broken.

I will also argue that the fact that most of your committers are "off" the Eclipse IDE shouldn't be a reason to not invest some time here. There are people, like me, who don't really care much about formatting rules as long as they can be enforced automatically, and who don't have the time to carefully configure their environment the way you would want a Webtide new hire spend their first weeks on.

For me, as an outside contributor not making any money from contributing, having that step automated is the difference between actually contributing a small change that I spend 10 minutes on rather than five hours or more (which probably would mean I won't be contributing that change since it's not worth it). It's the difference from being a collaborative open source project and a show kitchen.

I'm pretty sure the few formatting issues that arise from this automatic step can be fixed by adjusting the Eclipse formatting rules, or, by policy — just git checkout those conflicting changes. Nobody forces anyone to use this reformat profile (yet).

If there truly is barely anyone left in the Eclipse community who uses Eclipse IDE to format their code, there'd be something seriously wrong about that software stack, and, again, someone who gets paid for working on that software should spend a few hours getting that right.

[1] https://github.com/revelc/formatter-maven-plugin/issues/35

kohlschuetter avatar Feb 03 '23 18:02 kohlschuetter

@olamy @janbartel @joakime I've updated my PR #9304 to fix some inconsistencies with Jetty's Eclipse code formatter, but most importantly, I've added a shell script that uses this formatter to automatically reformat only the modified/newly added files (according to git diff).

This is still (deliberately) a manual step; no files are automatically reformatted upon build.

kohlschuetter avatar Feb 03 '23 21:02 kohlschuetter

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Feb 04 '24 00:02 github-actions[bot]

This issue has been closed due to it having no activity.

github-actions[bot] avatar Mar 05 '24 00:03 github-actions[bot]