Skip to content

optimize: Apply Spotless with Palantir Java Format #7223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 12, 2025
Merged

optimize: Apply Spotless with Palantir Java Format #7223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
32a5e7e
init spotless
YongGoose Mar 15, 2025
de6cf2e
Merge branch '2.x' into feature/apply-spotless
YongGoose Mar 15, 2025
73b865c
Merge branch '2.x' into feature/apply-spotless
YongGoose Mar 16, 2025
c294714
Merge branch '2.x' into feature/apply-spotless
xingfudeshi Mar 18, 2025
32b55f7
Add ratchetFrom
YongGoose Mar 20, 2025
661d18a
Add changes
YongGoose Mar 20, 2025
f7a0266
change ratchetFrom to HEAD
YongGoose Mar 20, 2025
fc7770f
Merge branch '2.x' into feature/apply-spotless
xingfudeshi Mar 21, 2025
3d5b4eb
Merge branch '2.x' into feature/apply-spotless
funky-eyes Mar 27, 2025
5c30142
Merge branch '2.x' into feature/apply-spotless
YongGoose Mar 31, 2025
3bee817
change ratchetFrom to origin/2.x
YongGoose Mar 31, 2025
a31ddf8
revert changes
YongGoose Mar 31, 2025
4f07580
change ratchetFrom to origin/2.x
YongGoose Mar 31, 2025
0e71c8d
change ratchetFrom to 2.x
YongGoose Mar 31, 2025
ffa856b
change ratchetFrom to refs/2.x
YongGoose Mar 31, 2025
0b3d1e4
revert changes
YongGoose Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions build/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@
<license-maven-plugin.version>4.0</license-maven-plugin.version>
<mojo-license-maven-plugin.version>1.20</mojo-license-maven-plugin.version>
<maven-checkstyle-plugin.version>3.1.1</maven-checkstyle-plugin.version>
<spotless-maven-plugin.version>2.44.3</spotless-maven-plugin.version>
<palantirJavaFormat.version>2.38.0</palantirJavaFormat.version>
<maven-enforcer-plugin.version>3.0.0-M3</maven-enforcer-plugin.version>
<dependency-check-maven.version>12.1.0</dependency-check-maven.version>
<!-- Test -->
Expand Down Expand Up @@ -174,6 +176,11 @@
<artifactId>maven-checkstyle-plugin</artifactId>
<version>${maven-checkstyle-plugin.version}</version>
</plugin>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>${spotless-maven-plugin.version}</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
Expand Down
1 change: 1 addition & 0 deletions changes/en-us/2.x.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Add changes here for all PR submitted to the 2.x branch.
- [[#7182](https://github.com/apache/incubator-seata/pull/7182)] use the ip of the peerId as the host of the raft node
- [[#7181](https://github.com/apache/incubator-seata/pull/7181)] raft implements domain name resolution and selects peerId
- [[#7213](https://github.com/apache/incubator-seata/pull/7213)] support kingbase xa mode
- [[#7223](https://github.com/apache/incubator-seata/pull/7223)] apply Spotless with Palantir java format


### bugfix:
Expand Down
1 change: 1 addition & 0 deletions changes/zh-cn/2.x.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
- [[#7182](https://github.com/apache/incubator-seata/pull/7182)] 采用peerId的ip作为raft节点的host
- [[#7181](https://github.com/apache/incubator-seata/pull/7181)] raft实现域名解析并选择peerId
- [[#7213](https://github.com/apache/incubator-seata/pull/7213)] support kingbase xa mode
- [[#7223](https://github.com/apache/incubator-seata/pull/7223)] 使用 Palantir java 格式应用 Spotless

### bugfix:

Expand Down
72 changes: 72 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,78 @@
</plugins>
</build>
</profile>

<!-- profile: spotless -->
<profile>
<id>jdk9-jdk11-spotless</id>
<activation>
<jdk>[1.8, 11)</jdk>
</activation>
<properties>
<palantirJavaFormat.version>1.1.0</palantirJavaFormat.version>
</properties>
</profile>

<profile>
<id>jdk11-jdk21-spotless</id>
<activation>
<jdk>[11, 21)</jdk>
</activation>
<properties>
<palantirJavaFormat.version>2.28.0</palantirJavaFormat.version>
</properties>
</profile>

<profile>
<id>jdk21-spotless</id>
<activation>
<jdk>[21,)</jdk>
</activation>
<properties>
<palantirJavaFormat.version>2.39.0</palantirJavaFormat.version>
</properties>
</profile>
<profile>
<id>java11+</id>
<activation>
<jdk>[11,)</jdk>
</activation>
<build>
<plugins>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>${spotless-maven-plugin.version}</version>
<configuration>
<java>
<excludes>
<exclude>**/script/**</exclude>
<exclude>**/generated/**</exclude>
<exclude>**/antlr/mysql/parser/*.*</exclude>
<exclude>**/antlr/mysql/antlr/*.*</exclude>
<exclude>**/antlr/mysql/stream/ANTLRNoCaseStringStream.java</exclude>
</excludes>
<ratchetFrom>HEAD</ratchetFrom>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slievrly @funky-eyes @xingfudeshi

I configured <ratchetFrom>HEAD</ratchetFrom> so that only the modified files are formatted by comparing them with the currently checked-out commit (HEAD).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slievrly @funky-eyes @xingfudeshi

I configured <ratchetFrom>HEAD</ratchetFrom> so that only the modified files are formatted by comparing them with the currently checked-out commit (HEAD).

Good job! I will pull this PR to my local machine for related testing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@funky-eyes

Currently, our baseline is set to HEAD, which means that only files changed since the last commit are considered. This creates a potential issue: if code with incorrect formatting gets committed, checkstyle will catch it, but Spotless won’t apply its fixes, and we lose the intended benefit.

To address this, we tried switching the baseline from HEAD to origin/2.x. However, the branch wasn’t recognized. 🥹

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@funky-eyes

Currently, our baseline is set to HEAD, which means that only files changed since the last commit are considered. This creates a potential issue: if code with incorrect formatting gets committed, checkstyle will catch it, but Spotless won’t apply its fixes, and we lose the intended benefit.

To address this, we tried switching the baseline from HEAD to origin/2.x. However, the branch wasn’t recognized. 🥹

What do you think?

Hi, I pulled your code and followed the steps. Seems to work!

1. git checkout feature/apply-spotless
2. Change the value of ratchetFrom from HEAD to feature/apply-spotless
3. Add some spaces in the method of Java class, like this
image 
4. mvn spotless:check
5. Then I got the outputs like this
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@funky-eyes

Currently, our baseline is set to HEAD, which means that only files changed since the last commit are considered. This creates a potential issue: if code with incorrect formatting gets committed, checkstyle will catch it, but Spotless won’t apply its fixes, and we lose the intended benefit.

To address this, we tried switching the baseline from HEAD to origin/2.x. However, the branch wasn’t recognized. 🥹

What do you think?

I think it can be done this way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, unless we format and commit all the code at once, we will not be able to prompt the user to run mvn spotless:apply. What if we change the HEAD to 2.x?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we change the HEAD to 2.x?

I think this approach is a good one.

However, when I tried running it locally, the behavior was different from what I expected, so I'm reconsidering it.
To explain in more detail, switching to origin/2.x should apply changes only to the files that have been modified from 2.x.

Even though there were no changes in my local branch, some recently committed files in 2.x were also modified.
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, I think setting it to 2.x is a better approach than setting it to HEAD.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this commit 4f07580 i changed ratchet from to origin/2.x

Then below error occured.

Error:  Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.44.3:apply (default) on project seata-parent: Execution default of goal com.diffplug.spotless:spotless-maven-plugin:2.44.3:apply failed: No such reference 'origin/2.x' -> [Help 1]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that after setting it to 2.x, the number of changed files is quite large, unless we are willing to accept changes to all files. Otherwise, I think we can initially set it to HEAD, merge this PR, and then create an issue to discuss the remaining problems.
@slievrly What do you think?

<palantirJavaFormat>
<version>${palantirJavaFormat.version}</version>
</palantirJavaFormat>
<removeUnusedImports/>
<trimTrailingWhitespace/>
<endWithNewline/>
</java>
</configuration>
<executions>
<execution>
<goals>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How it works in github action workflow, I didn't see this profile id included in workflow.In addition to checking if the format is as expected, is it possible to automatically format changes in the workflow and commit them to PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work in GitHub Actions.

Automatically formatting and committing changes seems more suitable for a git hook, which would require users to modify their local .git folder.

For now, in this PR, I’ve applied Spotless.
I’m planning to explore additional improvements in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

<goal>apply</goal>
</goals>
<phase>process-sources</phase>
</execution>
</executions>
</plugin>
</plugins>
</build>
</profile>
</profiles>

<build>
Expand Down
Loading