Enabled checkstyle rules for imports

[dmatej]

Reason

• Makes merges easier
• Consistent rules in the repository (except old ant tests which don't have any rules)
• See https://checkstyle.sourceforge.io/checks/imports/index.html
• From my experience
  • wildchar imports hide unused dependencies and can bring unexpected conflicts (ie. isEmpty method)
  • when making uncompilable code compilable you have to guess or investigate what is missing.
  • "This should not be here effect" - easier to notice poisonous dependency
  • ...? (I mightl remember later for more)

Required additional changes

• DeploymentEventListenerImpl was unused, so I deleted it including related I18N properties.
• Comments inside imports probably as a consequence of some old corrupted RCS/CVS merge were moved or deleted in dependency on content (javadocs moved, code history deleted). Example: com.sun.ejte.ccl.reporter.Reporter
• appserver/tests/embedded/web/web-api/pom.xml failed to pass the check because Eclipse IDE could not resolve dependencies. I added them to the pom.xml file.

Controversies

• static imports before/after/forbidden - maybe 2 years ago I asked somewhere who prefers what (twitter/linkedin?); winner was static imports AFTER common imports, the result was like 3:5:1. Since then I am asking nearly everyone I meet for opinion :D . I wrote somewhere even pros and cons and honestly TOP and BOTTOM both have advantages and disadvantages, but one is not better than the other. I use BOTTOM for some 15 years and it seems to me it is more preferred.
• import groups - have to be defined explicitly in the editor, however it is quite short list. I know it is quite simple in Eclipse IDE (I use it) and in IDEA (one of my recent teams uses it). I am not sure about Netbeans, VSCode etc. In general it makes easier to see the number of groups, the size of groups, ... and now we are talking about coupling, cohesion etc.

The Configuration

        <!-- Checks for imports                              -->
        <!-- See https://checkstyle.sourceforge.io/checks/imports/index.html -->
        <module name="AvoidStarImport"/>
        <!-- defaults to sun.* packages -->
        <module name="IllegalImport" />
        <module name="ImportOrder">
            <property name="groups" value="com,io,jakarta,java,javax,org" />
            <property name="option" value="bottom"/>
            <property name="ordered" value="true" />
            <property name="separated" value="true" />
            <property name="separatedStaticGroups" value="true" />
            <property name="sortStaticImportsAlphabetically" value="true" />
        </module>
        <module name="RedundantImport" />
        <module name="UnusedImports" />

[pzygielo]

Consider https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view once ready.

[pzygielo]

I see changes other than imports 😢

[dmatej]

I see changes other than imports 😢

I am yet going through the diff manually (and not using GitHub -- too many files changed). I will also yet update the description - in short, sometimes I had to move/delete comments inside imports - usually commented out code, sometimes wrong merges so javadoc ended between imports, probably at the time of RCS/CVS. I had to fix them manually.

One class with completely commented out body was deleted together with I18N found by fulltext search. I will add that to the description.

[dmatej]

I asked for the review all committers who recently contributed something. Also @OndroMih is significant contributor, so I am asking you too ;-)

It is not so important to see every file in the PR, rather read those rules and say why you don't like them - if you don't like them. My experience may be different, your editors can have different settings or you see something as an issue which could cause that we would have to change the configuration again.

[avpinchuk]

I use the same rules ;)

[OndroMih]

I don't care about the rules as long as they are the same for everybody. Checkstyle ensures that. I'll adjust my IDE if needed to comply.

All looks OK to me.

When I filtered all the lines with imports and comments from the diff, there are only a few things to review and they all look OK to me.

I used the following grep command to filter it (in Linux): cat 25083.diff | grep -v '^diff|^index|^---|^+++|^@@|^ |^[+-]import|^[+-]$|^[+-](//| *|/*| ?*/)|^[+-]#' | less

The output is:

-ejb.BaseProcessor.cannotdroptables=JDO76607: Cannot drop tables for application {0}. The expected DDL file {1} is not available.
-ejb.BaseProcessor.nodropfile=JDO76608: Cannot drop tables for deployment. The expected DDL file {0} is not available.
-ejb.PersistenceProcessor.nondefaultprovider=JDO76616: The java2db feature is not supported for the persistence provider ''{0}'' that you specified. Hence the tables associated to the entities of the PU named ''{1}'' would not be created and/or dropped from the database.
deleted file mode 100644
-package com.sun.jdo.spi.persistence.support.ejb.ejbc;
-public class DeploymentEventListenerImpl {
-} //DeploymentEventListenerImpl
-    // Reference and force the initialization of the DeploymentEventListener
-    // implementation.
-    private static final Class listener =
-        forceInit("com.sun.jdo.spi.persistence.support.ejb.ejbc.DeploymentEventListenerImpl");
-**/
-   @Description : Main class used for Uniform reporting of results
-   @Author : Ramesh Mandava
-   @Last Modified : Initial Creation by Ramesh Mandava after taking input from
-        Jeanfrancois and other Team members
-   @Last Modified : By Ramesh on 1/20/2002 , Added code to use new testIdVector and
-                testCaseIdVector for  preserving order of entry of them and now <tests>
-                element is added around multiple tests
-   @Last Modified : By Ramesh on 4/5/2002, Taken care of machine name unavailability
-                under J2EE. And allowed having . in the path of result file
-          <groupId>org.hamcrest</groupId>
-          <artifactId>hamcrest</artifactId>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.glassfish.main.web</groupId>
+            <artifactId>web-embed-api</artifactId>
+            <version>${project.version}</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>jakarta.servlet</groupId>
+            <artifactId>jakarta.servlet-api</artifactId>
+            <scope>provided</scope>
-        Context context = (Context) embedded.createContext(root);
+        Context context = embedded.createContext(root);
- import javax.transaction.xa.*;
+ import javax.transaction.xa.XAException;
- /* An implementation of org.omg.CosTransactions.Resource to support
-    public static enum LogMode {
+    public enum LogMode {
-    private Class<T> typedModuleClass = null; // typically set by postConstruct of a subclass invoking setTypeClass
+    private final Class<T> typedModuleClass = null; // typically set by postConstruct of a subclass invoking setTypeClass
-    private Map<BaseAuditModule, String> moduleToNameMap = new HashMap<BaseAuditModule, String>();
-    private Map<String, BaseAuditModule> nameToModuleMap = new HashMap<String, BaseAuditModule>();
+    private final Map<BaseAuditModule, String> moduleToNameMap = new HashMap<>();
+    private final Map<String, BaseAuditModule> nameToModuleMap = new HashMap<>();
-        final List<U> list = new ArrayList<U>();
+        final List<U> list = new ArrayList<>();
-        final List<U> list = new ArrayList<U>();
+        final List<U> list = new ArrayList<>();
-        final List<T> result = new ArrayList<T>();
+        final List<T> result = new ArrayList<>();
-        <!-- See http://checkstyle.sf.net/config_import.html -->
-        <module name="IllegalImport" /> <!-- defaults to sun.* packages -->
+        <!-- See https://checkstyle.sourceforge.io/checks/imports/index.html -->
+        <module name="AvoidStarImport"/>
+        <!-- defaults to sun.* packages -->
+        <module name="IllegalImport" />
+        <module name="ImportOrder">
+            <property name="groups" value="com,io,jakarta,java,javax,org" />
+            <property name="option" value="bottom"/>
+            <property name="ordered" value="true" />
+            <property name="separated" value="true" />
+            <property name="separatedStaticGroups" value="true" />
+            <property name="sortStaticImportsAlphabetically" value="true" />
+        </module>

[dmatej]

Ok, inspired by @OndroMih , I create my own monster :D
git diff --patch eclipse/8.0..HEAD | grep -E -v '^(\+import|\-import| import| package|index|\+\+\+|\-\-\-|\@\@).*$' | grep -E -v '^[+-]$' | grep -E '^(diff|\+|\-).+$'

[arjantijms]

Example for Eclipse settings: