KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter #16097

gongxuanzhang · 2024-05-28T03:12:41Z

add a import-order rule. It includes the following parts

checkstyle import-order
suppress all module
in Intellj IDEA auto format coding configuration

checkstyle import-order

In order to uniform coding specifications in Kafka. This is the first step.
Referred to #10428 import order .
We specify the import order as "kafka, org.apache.kafka, com, net, org, java, javax and static imports."

suppress all module

If the detection function is enabled,A lot of code needs to be changed.
In order to pass CI and minimal modification. suppress all module checkstyle import-order.
We can open the module step by step.

in Intellj IDEA auto format coding configuration

If we want to uniform all code. The manual approach is cumbersome and error-prone.
And we need everyone to follow the rules in the future. So we need auto format coding tools.
In this PR I add a IDEA configuration and update gitignore file

chia7712

@gongxuanzhang could you please include the auto-formater in this PR? That can save our life :)

chia7712 · 2024-05-28T08:46:32Z

.gitignore

@@ -13,6 +13,7 @@ project/boot/
 project/plugins/project/
 patch-process/*
 .idea
+!/.idea/codeStyles


the whole idea folder is not in version control, so do we need this line?

the whole idea folder is not in version control, so do we need this line?

It has to be, and it's very important.

This file will allow you to import your idea automatically into the project's code format

It seems to me using build tool to address that is more acceptable. We all use gradle to build kafka but there are many IDE.

We can add IDE supports in the future but could you please keep this PR simple for now?

gongxuanzhang · 2024-05-30T03:39:06Z

I revised this PR.It add spotless gradle plugin and mini configuration.
It contains two part.

checkstyle import order rule. But suppress all module. you can change checkstyle/suppressions.xml open module check If you want
auto format import order. Again, I did not open in any module. If you want open the auto format. Just add a module name into build.gradle #spotlessApplyModules like def spotlessApplyModules = ['core'] .then run ./gradlew spotlessApply you will get formated code module

chia7712

@gongxuanzhang nice patch. BTW, could you add docs to README for spotlessApply?

build.gradle

chia7712 · 2024-05-30T19:52:48Z

build.gradle

@@ -1007,7 +1026,7 @@ project(':core') {
    testImplementation libs.junitJupiter
    testImplementation libs.slf4jlog4j
    testImplementation libs.caffeine
-


please revert this change

chia7712 · 2024-05-30T19:53:00Z

checkstyle/suppressions.xml

@@ -361,4 +361,7 @@
    <suppress checks="(ClassDataAbstractionCoupling|ClassFanOutComplexity)"
              files="(ReplicaFetcherThreadBenchmark).java"/>

+    <!--import order -->
+    <suppress checks="ImportOrder" files="^*$"/>


why we need this change?

I think checkstyle should consistent with auto format . If you open the A module auto format, we should open the module check rule.
ImportOrder rule can't custom in each module(It's going to take a lot of changes,maybe should add build.gradle every module).
So i add this line in order that open rule some module in future,This is what I think is a more convenient way to modify by module

Making the consistency is extra effort. Also, those changes will complicate checkstyle files. Hence, I prefer to remove them if we do NOT use it actually.

chia7712 · 2024-05-30T20:04:01Z

build.gradle

@@ -787,6 +800,12 @@ subprojects {
    skipProjects = [ ":jmh-benchmarks", ":trogdor" ]
    skipConfigurations = [ "zinc" ]
  }
+
+  afterEvaluate {


Maybe we can set spotless directly. For example:

if (project.name in spotlessApplyModules) { apply plugin: 'com.diffplug.spotless' spotless { java { importOrder('kafka', 'org.apache.kafka', 'com', 'net', 'org', 'java', 'javax', '', '\\#') removeUnusedImports() } } }

I change the PR,please review it @chia7712

chia7712

@gongxuanzhang thanks for updated PR.

chia7712 · 2024-05-31T12:48:03Z

README.md

@@ -232,6 +232,12 @@ You can run checkstyle using:
 The checkstyle warnings will be found in `reports/checkstyle/reports/main.html` and `reports/checkstyle/reports/test.html` files in the
 subproject build directories. They are also printed to the console. The build will fail if Checkstyle fails.

+#### Spotless ####
+This plugin can review code by rules,it is disabled by default. You can enable it by setting `build.gradle` add a module name into the `spotlessApplyModules` variable like 
+`def spotlessApplyModules = ['core']`,then you can run spotless using:


spotlessApplyModules is a temporary limit, and we don't need to expose it.

chia7712 · 2024-05-31T12:50:33Z

gradle.properties

@@ -28,6 +28,8 @@ scalaVersion=2.13.14
 # Adding swaggerVersion in gradle.properties to have a single version in place for swagger
 # New version of Swagger 2.2.14 requires minimum JDK 11.
 swaggerVersion=2.2.8
+# 6.14.0 and newer are need for java 11
+spotlessVersion=6.13.0


spotlessVersion is used by only one-line, hence we can inline it.

chia7712 · 2024-05-31T12:53:35Z

checkstyle/suppressions.xml

@@ -361,4 +361,7 @@
    <suppress checks="(ClassDataAbstractionCoupling|ClassFanOutComplexity)"
              files="(ReplicaFetcherThreadBenchmark).java"/>

+    <!--import order -->
+    <suppress checks="ImportOrder" files="^*$"/>


Making the consistency is extra effort. Also, those changes will complicate checkstyle files. Hence, I prefer to remove them if we do NOT use it actually.

gongxuanzhang · 2024-06-01T15:18:04Z

I update this PR,plz review it

chia7712

@gongxuanzhang thanks for updated PR

chia7712 · 2024-06-01T16:40:20Z

build.gradle

@@ -47,7 +47,8 @@ plugins {
  // Updating the shadow plugin version to 8.1.1 causes issue with signing and publishing the shadowed
  // artifacts - see https://github.com/johnrengelman/shadow/issues/901
  id 'com.github.johnrengelman.shadow' version '8.1.0' apply false
-  id 'com.diffplug.spotless' version '6.14.0' apply false // 6.14.1 and newer require Java 11 at compile time, so we can't upgrade until AK 4.0
+  //  6.14.0 and newer are need for java 11


how about: the minimum required JRE of 6.14.0+ is 11

I refer to spotless github: https://github.com/diffplug/spotless/tree/main/plugin-gradle#requirements

chia7712 · 2024-06-01T16:41:52Z

checkstyle/checkstyle.xml

@@ -82,6 +82,10 @@
      <property name="file" value="${config_loc}/${importControlFile}"/>
    </module>

+    <!--   import order rule not config in the checkstyle.xml.


how about:

DON'T define any import order here! Import order check/format is addressed by spotless.

I update that @chia7712

chia7712 · 2024-06-02T12:38:07Z

README.md


 The checkstyle warnings will be found in `reports/checkstyle/reports/main.html` and `reports/checkstyle/reports/test.html` files in the
 subproject build directories. They are also printed to the console. The build will fail if Checkstyle fails.

+#### Spotless ####
+This plugin can review code by rules and can also help you check the code , it is disabled by default. Some of our code reviews use `spotless` instead of `checkstyle`:


The import order is a part of static check. please call spotlessApply to optimize the imports of Java codes before filing pull request.

chia7712

LGTM!

…matic formatter (apache#16097) Reviewers: Chia-Ping Tsai <[email protected]>

gongxuanzhang and others added 6 commits May 27, 2024 18:15

update hide file

227a957

add Project.xml

7596c2d

Merge branch 'apache:trunk' into import-order

7e56b6a

upload Project.xml

00ad230

update suppressions.xml

2d6b289

Merge branch 'apache:trunk' into import-order

77f46e1

chia7712 reviewed May 28, 2024

View reviewed changes

gongxuanzhang and others added 6 commits May 29, 2024 22:13

Delete .idea directory

e9db586

Merge branch 'apache:trunk' into import-order

f2ef540

revert gitignore

37cc473

Merge branch 'apache:trunk' into import-order

40aabc0

add spotless

2960a41

update build.gradle

7cde3f8

Merge branch 'apache:trunk' into import-order

8e171fb

chia7712 reviewed May 30, 2024

View reviewed changes

gongxuanzhang and others added 4 commits May 31, 2024 10:43

revert blank

82de188

change the build.gradle

5623a3b

update README

3cb353d

Merge branch 'apache:trunk' into import-order

1ee4e62

chia7712 reviewed May 31, 2024

View reviewed changes

gongxuanzhang and others added 5 commits June 1, 2024 19:39

Merge branch 'apache:trunk' into import-order

076b467

update checkstyle

c3abfc2

update readme

462625d

update build.gradle

6f25e4d

update build.gradle

7d99f05

chia7712 reviewed Jun 1, 2024

View reviewed changes

update checkstyle.xml

d956ba3

gongxuanzhang and others added 4 commits June 2, 2024 16:00

update build.gradle

9f316a9

Merge branch 'apache:trunk' into import-order

bfa5077

revert build.gradle

5e08003

lowercase

3702bb1

chia7712 reviewed Jun 2, 2024

View reviewed changes

gongxuanzhang added 2 commits June 2, 2024 21:22

Update README.md

54277f3

Merge branch 'apache:trunk' into import-order

89999a8

chia7712 approved these changes Jun 2, 2024

View reviewed changes

chia7712 merged commit 342e691 into apache:trunk Jun 2, 2024
1 check failed

This was referenced Jun 3, 2024

KAFKA-10787: Apply spotless to transaction-coordinator and server-common #16172

Merged

KAFKA-10787: Update spotless version and remove support JDK8 #16176

Merged

wernerdv pushed a commit to wernerdv/kafka that referenced this pull request Jun 3, 2024

KAFKA-10787 Add import ordering checkstyle rule and configure an auto…

3359be1

…matic formatter (apache#16097) Reviewers: Chia-Ping Tsai <[email protected]>

TaiJuWu pushed a commit to TaiJuWu/kafka that referenced this pull request Jun 8, 2024

KAFKA-10787 Add import ordering checkstyle rule and configure an auto…

5475d1b

…matic formatter (apache#16097) Reviewers: Chia-Ping Tsai <[email protected]>

gongxuanzhang added a commit to gongxuanzhang/kafka that referenced this pull request Jun 12, 2024

KAFKA-10787 Add import ordering checkstyle rule and configure an auto…

e828c68

…matic formatter (apache#16097) Reviewers: Chia-Ping Tsai <[email protected]>

gongxuanzhang mentioned this pull request Jun 27, 2024

KAFKA-10787: Apply spotless to all module #16467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter #16097

KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter #16097

gongxuanzhang commented May 28, 2024 •

edited

Loading

chia7712 left a comment

chia7712 May 28, 2024

gongxuanzhang May 28, 2024

chia7712 May 28, 2024

gongxuanzhang commented May 30, 2024

chia7712 left a comment

chia7712 May 30, 2024

chia7712 May 30, 2024

gongxuanzhang May 31, 2024 •

edited

Loading

chia7712 May 31, 2024

chia7712 May 30, 2024

gongxuanzhang May 31, 2024

chia7712 left a comment

chia7712 May 31, 2024

chia7712 May 31, 2024

chia7712 May 31, 2024

gongxuanzhang commented Jun 1, 2024

chia7712 left a comment

chia7712 Jun 1, 2024

gongxuanzhang Jun 2, 2024

chia7712 Jun 1, 2024

gongxuanzhang Jun 2, 2024

chia7712 Jun 2, 2024 •

edited

Loading

chia7712 left a comment

KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter #16097

KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter #16097

Conversation

gongxuanzhang commented May 28, 2024 • edited Loading

checkstyle import-order

suppress all module

in Intellj IDEA auto format coding configuration

chia7712 left a 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

gongxuanzhang commented May 30, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongxuanzhang May 31, 2024 • edited Loading

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

chia7712 left a 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

gongxuanzhang commented Jun 1, 2024

chia7712 left a 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

chia7712 Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

gongxuanzhang commented May 28, 2024 •

edited

Loading

gongxuanzhang May 31, 2024 •

edited

Loading

chia7712 Jun 2, 2024 •

edited

Loading