Fail the build on wrong import order

Development
1

Hi devs,

We have some rules about the right import order:

We also have IJ IDEA + Eclipse settings to follow that order:

The proposal here (asked originally by @MichaelHamann) is to break the build (using the existing checkstyle rule) if that order is not satisfied. The idea is to do a mechanistic full sweep of all violations and fix them in a few commits before enabling the check in the build.

The rationale is to avoid diffs in the order (for ex, if the order is not followed and your idea re-order automatically, you’ll get a diff). And avoid later corrections for both code contributions from some non-committers and for code written by coding agents.

WDYT?

Thanks

2

I assume you meant https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HImports.

+1 to add a check for import order in the checkstyle configuration

3

yes thx, corrected.

4

+1, sounds good to me. That’s what’s done by our eslint configuration for Typescript code.

5

Follow up on the checkstyle config to use:

Replace the currently-commented-out ImportOrder module in checkstyle.xml with:

<module name="ImportOrder">                                                                                                                                                                                    
  <property name="groups" value="java,javax,jakarta,org,com"/>                                                                                                                                                 
  <property name="ordered" value="true"/>                                                                                                                                                                      
  <property name="separated" value="true"/>                                                                                                                                                                    
  <property name="option" value="bottom"/>                                                                                                                                                                     
  <property name="sortStaticImportsAlphabetically" value="true"/>                                                                                                                                              
</module>

Why these properties:

  • groups: imports matching none of them land in an implicit trailing group, which is exactly the “any other imports” bucket of our rule.
  • separated=true: enforces the blank line between groups, and also rejects a blank line in the middle of a group.
  • option=bottom: static imports go last. Note that it also requires the blank line between the last non-static group and the static block.
  • sortStaticImportsAlphabetically=true: without it, the ordering inside the static block isn’t checked at all.
  • caseSensitive is left to its default (true), which is what matches our IDEA config: setting it to false gives 743 violations in commons alone, versus 37 with true.

One point to decide: checkstyle.xml is only applied to main sources, test sources go through checkstyle-test.xml. I’d add the same module to both, since a good half of the violations I found are in test code (and the whole point is to avoid IDE-generated diffs, which happens as much in tests).

WDYT? Ok for you to start introducing a checkstyle config for tests, starting with this import order?

Thx

6

While waiting for more opinions, I’ve fixed the order (since it’s already in our practices):

Thx

7

And Loading... and its PR are ready if the agreement goes through.

8

Sure.

9

Implemented in Loading...