Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add import order in checkstyle rules #556

Closed
brenuart opened this issue Jul 6, 2021 · 3 comments · Fixed by #561
Closed

Add import order in checkstyle rules #556

brenuart opened this issue Jul 6, 2021 · 3 comments · Fixed by #561
Milestone

Comments

@brenuart
Copy link
Collaborator

brenuart commented Jul 6, 2021

It would be nice to have checkstyle enforce the ordering of import statements as well. People are using different IDEs with different defaults. We we don't impose a particular order, imports will be constantly reorder with every commit...

Here is a first proposition:

  1. static imports first
  2. -blank line-
  3. java.*
  4. -blank line-
  5. javax.*
  6. -blank line-
  7. net.logstash.*
  8. -blank line-
  9. org.*
  10. -blank line-
  11. anything else

Each group is sorted alphabetically.

Example:

import static java.util.Objects.requireNonNull;

import java.util.ArrayList;
import java.util.List;

import net.logstash.logback.appender.listener.AppenderListener;
import net.logstash.logback.status.LevelFilteringStatusListener;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.lmax.disruptor.EventFactory;
import com.lmax.disruptor.EventHandler;

import ch.qos.logback.access.spi.IAccessEvent;
import ch.qos.logback.classic.AsyncAppender;

Corresponding checkstyle rule:

        <module name="ImportOrder">
            <property name="groups" value="/^java\./,javax,net.logstash,org,com"/>
            <property name="ordered" value="true"/>
            <property name="separated" value="true"/>
            <property name="option" value="above"/>
            <property name="sortStaticImportsAlphabetically" value="true"/>
        </module>
@philsttr
Copy link
Collaborator

philsttr commented Jul 7, 2021

I personally like less groups.

static imports first
-blank line-
java.*
-blank line-
anything else

However, running that config means 179 checkstyle errors in 92 files.

Running with your proposal is 114 errors in 78 files, which means less code churn. So, I'm good with your proposal, although (minor comment) I think all the groups can use prefixes instead of regexes value="java.,javax.,net.logstash,org,com"

@brenuart
Copy link
Collaborator Author

brenuart commented Jul 7, 2021

No matter how many files are affected - more than 50% will have to be changed anyway...
I'm also in favour of less groups but like the idea of having a separate group with net.logstash packages.

What about this:

static imports first
-blank line-
java.*
-blank line-
net.logstash.*
-blank line-
anything else

I think all the groups can use prefixes instead of regexes value="java.,javax.,net.logstash,org,com"

Indeed...

@philsttr
Copy link
Collaborator

philsttr commented Jul 7, 2021

more than 50% will have to be changed anyway

Good point.

What about this:

Sounds good to me. Let's go with that.

brenuart added a commit to brenuart/logstash-logback-encoder that referenced this issue Jul 7, 2021
brenuart added a commit to brenuart/logstash-logback-encoder that referenced this issue Jul 7, 2021
@philsttr philsttr linked a pull request Jul 7, 2021 that will close this issue
@philsttr philsttr added this to the 7.0 milestone Jul 7, 2021
brenuart added a commit to brenuart/logstash-logback-encoder that referenced this issue Jul 7, 2021
philsttr pushed a commit that referenced this issue Jul 10, 2021
* Add checkstyle rule for java import statements
* Fix import ordering + ban unused imports
- static imports go on top in their own separate group (not mixed with other groups)
- ban unused imports
* Limit the check for a valid license header to .java source files only

Related issue: #556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants