DRAFT: switch to spotless #1460

marko-bekhta · 2024-09-25T12:42:51Z

This one is only to evaluate the option and not actually to apply the change ...
I thought that it would be better to test this on the Validator as it uses all 3 plugins and has a smaller codebase.
Best to look by commit (if looked at all 😅).

Not all things that we are checking with checkstyle have an alternative (or at least I couldn't find one). I kept those rules in the checkstyle file so we can review them. I'll add a comment on that file so it's easier to find.
I couldn't split the license header check from formatting; hence, the files with different licenses are not formatted... (I suspect that having multiple executions would do the trick, but I didn't want to go that route as I suppose the idea was to simplify the build and have fewer executions....)
import sorting removes the comments. This means that the tags we have in the docs tests to include in the documentation file have to be excluded from spotless, resulting in those imports not being sorted (not a big deal, but a bit annoying 😃)
While trying to figure out the patterns for XML headers I've noticed that the plugin does not always fail when there's a problem in the config. I had a bad regex that was capturing the license header itself resulting in infinite update of the same file (at least that is what I've seen while debugging). This resulted in the build just passing successfully and the files not affected (I'd rather it failed with a build error .... )

(this work is not complete: the maven properties and builds should also be updated to make it all work and I haven't done that here yet...)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on licensing, please check here.

there were some files using spaces while others were using tabs ...

hibernate-github-bot · 2024-09-25T12:42:56Z

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HV-\d+
↳ Offending commits: [78d82bd, eb85e43, 27fe45e, 15478e2, f5f07ae, a412f6b, f1ef3d1, ad4fddc]

› This message was automatically generated.

marko-bekhta · 2024-09-25T12:44:16Z

build/build-config/src/main/resources/checkstyle.xml

@@ -14,69 +14,24 @@

 <module name="TreeWalker">

+ <!-- cannot do with spotless but then is it really needed ? -->
 <module name="RegexpSinglelineJava">
 <!-- do not allow a package declaration that contains ".target." or "target;" -->
 <property name="format" value="^package .*\.target[\.;]"/>
 <property name="message" value="Do not use &quot;target&quot; as package name element"/>
 </module>



And here's the checkstyle config ...

marko-bekhta · 2024-09-25T12:47:55Z

@yrodiere I'll let you have a look at this one (probably whenever you have nothing interesting to look into 😄 )

yrodiere · 2024-09-26T08:14:09Z

I couldn't split the license header check from formatting; hence, the files with different licenses are not formatted... (I suspect that having multiple executions would do the trick, but I didn't want to go that route as I suppose the idea was to simplify the build and have fewer executions....)
[...]

While trying to figure out the patterns for XML headers I've noticed that the plugin does not always fail when there's a problem in the config. I had a bad regex that was capturing the license header itself resulting in infinite update of the same file (at least that is what I've seen while debugging). This resulted in the build just passing successfully and the files not affected (I'd rather it failed with a build error .... )

Looks like we'd be better off keeping license header checks in checkstyle?

import sorting removes the comments. This means that the tags we have in the docs tests to include in the documentation file have to be excluded from spotless, resulting in those imports not being sorted (not a big deal, but a bit annoying 😃)

Annoying indeed... though it's quite rare that we include comments in docs code snippets, at least for Hibernate Search, so I suppose we can live with it.
Do you know if there's a bug report / feature request in Spotless regarding this?

Not all things that we are checking with checkstyle have an alternative (or at least I couldn't find one). I kept those rules in the checkstyle file so we can review them. I'll add a comment on that file so it's easier to find.

Will have a look, but if we need checkstyle for license header checks anyway, I suppose it's no big deal.

yrodiere · 2024-09-26T08:15:54Z

...t/java/org/hibernate/validator/referenceguide/chapter02/containerelement/custom/CarTest.java

+ * Hibernate Validator, declare and validate application constraints
+ *
+ * License: Apache License, Version 2.0
+ * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.


Can you create an issue to ultimately switch to SPDX headers, like in Search/ORM?

yrodiere · 2024-09-26T08:18:35Z

distribution/pom.xml

@@ -1,3 +1,4 @@
+<?xml version="1.0" encoding="utf-8" ?>


Oh, so it's not limited to code, it also gets applied to poms. Nice.

There's also this "pom" section that can be configured: https://github.com/diffplug/spotless/tree/main/plugin-maven#maven-pom where it can keep the pom "formatted" too, but I didn't want to try it as we have some cases where the profile has to go last (those reporting modules etc....) so I thought better leave it as is 😃

Ah, so this one was manual. Gotcha.

You're right, it's probably safer to skip poms, I didn't think of that.

yrodiere · 2024-09-26T08:20:11Z

pom.xml

- <executions>
- <execution>
- <id>import-sorting</id>
- <goals>
- <goal>${goal.impsort-maven-plugin}</goal>
- </goals>
- <phase>process-sources</phase>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <groupId>net.revelc.code.formatter</groupId>
- <artifactId>formatter-maven-plugin</artifactId>
- <version>${version.formatter-maven-plugin}</version>


So we can replace both of these plugins with just spotless? That's already a nice improvement, no?

yrodiere · 2024-09-26T08:22:12Z

pom.xml

+
+ <importOrder>
+ <!-- you can use an empty string for all the imports you didn't specify explicitly, '|' to join group without blank line, and '\#` prefix for static imports. -->
+ <order>\#,java,javax,jakarta,org.openjdk.jmh,org.hibernate,org.hibernate.testing,org.hibernate.test,org.junit,org.jboss,com,javafx.beans,</order>


Can this be imported from a file? People will need a file specifying import orders in order to set up formatting in their IDE.

Unless spotless gets executed automatically in the IDE, but I suspect it only is when compiling...

yes there's an option to pass a file:

<order>java|javax,org,com,com.diffplug,,\#com.diffplug,\#</order>

yrodiere · 2024-09-26T08:23:09Z

pom.xml

+ <!-- default one fails to process the classes -->
+ <engine>cleanthat-javaparser-unnecessaryimport</engine>
+ </removeUnusedImports>
+ <licenseHeader>


Does this apply the header automatically, on top of just checking?

it's like with those other plugins, if spotless:apply is executed then the headers are added, and if it's spotless:check then it fails the build if the header is missing...

yrodiere · 2024-09-26T08:23:43Z

pom.xml

- <artifactId>impsort-maven-plugin</artifactId>
- <version>${version.impsort-maven-plugin}</version>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>


We probably need two execution mode: one for dev (auto formats) and one for CI (checks only, no auto format)

marko-bekhta added 8 commits September 25, 2024 11:52

HV-??? Add missing xml tags to some of the project test XMLs

78d82bd

HV-??? Add missing xml license headers

eb85e43

HV-??? Use tabs in XMLs and Java files

27fe45e

there were some files using spaces while others were using tabs ...

HV-??? Add missing license headers

15478e2

HV-??? Clean up some extra blank lines

f5f07ae

HV-??? Add spotless config

a412f6b

HV-??? Remove checkstyle rules addressed by other plugins

f1ef3d1

HV-??? Remove the plugins that are supposed to be replaced by spotless

ad4fddc

marko-bekhta commented Sep 25, 2024

View reviewed changes

yrodiere reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: switch to spotless #1460

DRAFT: switch to spotless #1460

marko-bekhta commented Sep 25, 2024

hibernate-github-bot bot commented Sep 25, 2024

marko-bekhta Sep 25, 2024

marko-bekhta commented Sep 25, 2024

yrodiere commented Sep 26, 2024

yrodiere Sep 26, 2024

yrodiere Sep 26, 2024

marko-bekhta Sep 26, 2024

yrodiere Sep 26, 2024

yrodiere Sep 26, 2024

yrodiere Sep 26, 2024

marko-bekhta Sep 26, 2024

yrodiere Sep 26, 2024

marko-bekhta Sep 26, 2024

yrodiere Sep 26, 2024

DRAFT: switch to spotless #1460

Are you sure you want to change the base?

DRAFT: switch to spotless #1460

Conversation

marko-bekhta commented Sep 25, 2024

hibernate-github-bot bot commented Sep 25, 2024

Choose a reason for hiding this comment

marko-bekhta commented Sep 25, 2024

yrodiere commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment