Skip to content

KAFKA-10787: Introduce an import order in Java sources#8404

Closed
dongjinleekr wants to merge 9 commits intoapache:trunkfrom
dongjinleekr:feature/checkstyle-import-order
Closed

KAFKA-10787: Introduce an import order in Java sources#8404
dongjinleekr wants to merge 9 commits intoapache:trunkfrom
dongjinleekr:feature/checkstyle-import-order

Conversation

@dongjinleekr
Copy link
Copy Markdown
Contributor

Until now, Kafka has not enforced any import order policy. However, the lack of import order policy has been introduced many meaningless import order changes in the commits.

This PR adds an import order policy in checkstyle.xml that has been most generally accepted - that is, it organizes the imports to the following 4 groups in alphabetical order:

  1. org.apach.kafka.*
  2. javax.*
  3. java.*
  4. *

As an example, I reorganized 5 files in org.apache.kafka.streams.state.internals package here. Please pay attention to that all java package imports are grouped together and the imports on org.apache.kafka always go to the top of the file.

(This PR is WIP yet; As soon as this change is accpeted, I will update the PR with reorganizing all imports.)

@hachikuji
Copy link
Copy Markdown
Contributor

Thanks, I think this makes sense. I also get a bit annoyed with the constant import shuffling. I'd like to hear some feedback from others though before we consider merging this. @ijuma any thoughts?

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 7, 2020

I think this would be good. A couple of things:

  1. This will generate a massive diff.
  2. It would be good to have ways to format correctly automatically. It will be really annoying to do this manually. Ideally, an IntelliJ config and a Gradle plugin.
  3. We need to discuss the desired order with a few committers.

@dongjinleekr
Copy link
Copy Markdown
Contributor Author

@ijuma

Ideally, an IntelliJ config and a Gradle plugin.

I also inspected this issue a little bit. In the case of IntelliJ, checkstyle-idea plugin works like a charm - I used this plugin.

In the case of Gradle, It seems like diffplug/spotless plugin is the best option as of present. It has lots of features we need and is also actively developed and maintained.

We need to discuss the desired order with a few committers.

Great. I will follow the discussion result.

@dongjinleekr dongjinleekr force-pushed the feature/checkstyle-import-order branch from 354f90c to 0daddf0 Compare August 13, 2020 07:16
@dongjinleekr dongjinleekr force-pushed the feature/checkstyle-import-order branch from 0daddf0 to 4eba935 Compare October 15, 2020 02:49
@dongjinleekr dongjinleekr force-pushed the feature/checkstyle-import-order branch from 4eba935 to c11f9d4 Compare December 1, 2020 06:10
@dongjinleekr dongjinleekr changed the title MINOR: Add ImportOrder settings to checkstyle/checkstyle.xml KAFKA-10787: Introduce an import order in Java sources Dec 1, 2020
@dongjinleekr
Copy link
Copy Markdown
Contributor Author

Here is the update.

  • Adapted the three-group import ordering as discussed in the mailing thread.
  • Add documentation for the IDE plugins.
  • Reorder the existing files and rebased onto the latest trunk.

cc/ @mjsax @cadonna @ableegoldman @vvcephei

Copy link
Copy Markdown
Member

@chia7712 chia7712 Dec 3, 2020

Choose a reason for hiding this comment

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

According to description, the import order is

  1. org.apach.kafka.*
  2. javax.*
  3. java.*
  4. *

Is org.slf4j.Logger in wrong order?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chia7712 Oh, the description I had at first on 2nd April is outdated; After the discussion, we concluded that the following three-group ordering would be better:

  • kafka, org.apache.kafka
  • com, net, org
  • java, javax

@dongjinleekr dongjinleekr force-pushed the feature/checkstyle-import-order branch from c11f9d4 to 315fb41 Compare December 4, 2020 07:26
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 4, 2020

@dongjinleekr The PR shows merge conflict. Can you rebase it?

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 4, 2020

About #8404 (comment)

As "spotless" can reformat code, could we use it to re-order imports? This might be the best solution.

@dongjinleekr dongjinleekr force-pushed the feature/checkstyle-import-order branch from 315fb41 to eebfa46 Compare December 5, 2020 06:41
@dongjinleekr
Copy link
Copy Markdown
Contributor Author

@mjsax Rebased onto the latest trunk. Since this PR touches so many files, this PR will be broken every time the latest trunk updates. I will follow up on the latest trunk periodically as far as I can do until it is merged.

@dongjinleekr
Copy link
Copy Markdown
Contributor Author

@mjsax And here are some explanation of why I did not adopt any auto-import reorganization or auto-formatting.

At first, I tried to add some configuration to diff/spotless plugin to reformat the code automatically. However, I found that to enable this diff/spotless requires to adopt any code formatter, like Google Java Format or Eclipse JDT Formatter, etc.

Since the current Kafka code uses its own checkstyle format with 4 spaces, any one of the formatters above will reformat the whole codebase. So I concluded that it would be better to leave it unchanged for now.

If most of the community prefers to adopt a new formatting rule taking advantage of this chance, I will also adapt the auto-reformat plugin. It is totally up to the community's decision.

How do you think? Would it much better to open a discussion thread on dev mailing list?

cc/ @cadonna @ableegoldman @vvcephei @chia7712

@chia7712
Copy link
Copy Markdown
Member

Personally, the import rule is good to avoid import conflicts even though there is no auto-optimization tool.

@dongjinleekr dongjinleekr force-pushed the feature/checkstyle-import-order branch from eb26157 to 59c7d3d Compare January 18, 2021 07:15
@dongjinleekr
Copy link
Copy Markdown
Contributor Author

Rebased onto the latest trunk.

@dongjinleekr
Copy link
Copy Markdown
Contributor Author

Closes this PR in preference of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants