I would like to start a discussion about organizing Java imports.
With merge requests a major source of change are the import sections. Depending on the used IDE and configured settings the imports are changing. Functionally this can be ignored, but it brings a lot of noise into the pull request reviews and commits, and is often causing merge conflicts.
I would like to harmonize this throughout the code base and make sure that we will not experience diverging changes in the future.
First, we need to agree on the import style:
Order of “normal” imports:
JDK imports (java.util etc.)
Operaton imports (org.operaton.*)
Other libraries (org.slf4j etc.)
Order of test imports
Just treat like regular imports?
Or have a section for “regular” imports and another on test libraries?
Where to place static imports?
Wildcard imports
Do we just wildcards or not?
If yes, Wildcard import from N imports of a package?
If yes, Wildcard import from M imports of static identifiers?
Next, how do we make sure that everyone uses the same settings? IMO we should place configuration files in ./.devenv/ide/<IDE> (“ide” T.B.D.) and update the contribution guide.
Ultimatively I am thinking about enforcement of these rules by automatic updates. I think we could leverage OpenRewrite to apply import guidelines on code contributions. Maybe we would set up a workflow that runs on PRs and fail (?) the build with instructions to update, or even commit on changes (when the PR is on the Operaton repo)?
I would agree with this order, have seen it in several projects and is a sensible approach. How about ordering inside of these groups? Alphabetical order, then we get hierarchichal order alongside.
My first impulse would be to treat them like normal imports. Separating test libraries into its own group could be quite hard to check automatically, and I’m not sure about the benefits.
I like static imports on the bottom, I think this helps readability.
In general I’m opposed to Wildcard imports. I think they can make life easier with static imports (at least in the testing scope), but the risk of collisions is still there. What’s your opinion on those?
For checking: We could add a Maven Checkstyle plugin, which has capabilities to check the Import Order: ImportOrder – checkstyle
Executing it in the normal build phase could be annoying, I could also imagine running it during PR checks in its own profile (which still allows people to run it locally if they prefer to check before pushing).
Thanks for for feedback. Then let’s get this rolling.
I am unsure about the Wildcard imports. Though I would usually not use them, they could improve readability when there are many imports from the same package. I tend to use them when having imports of constants, enum literals and testing libraries.
I would further like to have an automated approach in the build. I did first attempts with the OpenRewrite recipe, this makes the code style configurable in code (rewrite.yaml). However, I already ran into a small technical problem: There are some Operaton assertions that have name clashes with AssertJ: They define both “assertThat()” methods and it is important that the right one is picked. The recipe sorted them alphabetically which changed the order and lead to compile errors. I think we have just a few such cases and could solve that. I will investigate this further.
When you write “automated approach in the build” I’m not yet sure I understand what you mean. Do you want the pipeline to only check or to rewrite imports which are not according to standards?
It would be great if this could be somehow integrated in the pipeline. I would like to enforce some basic coding standards. For now, this is just an idea. I would be happy already when we have a clean code base and IDE settings for them. I expect that we have much less noise on PRs.