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

ImportOrder: adjacent static import groups ones became impossible in 8.3 #5176

Closed
ssp opened this Issue Oct 8, 2017 · 16 comments

Comments

Projects
None yet
7 participants
@ssp

ssp commented Oct 8, 2017

In Version 8.3 checkstyle start to enforce no blank lines within what it considers an import group. ImportOrder

In a project, we have a setup where static imports from a group are considered a group of its own, separated by a blank line. In terms of a an Eclipse-Importorder file it looks like this:

2=javax
1=\#java
0=java

As far as I understand the documentation and code in checkstyle, this means I cannot make checkstyle accept this kind of import order anymore. Previously 8.3 will only let me have static imports without a blank line separating them from their non-static counterparts or all of them together in a group at the very top or bottom.

Example import order that used to be OK but is not anymore:

➜  cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="ImportOrderCheck">
            <property name="groups" value="java, javax" />
            <property name="separated" value="true" />
            <property name="ordered" value="true" />
            <property name="option" value="bottom" />
         </module>
    </module>
</module>


➜  cat InputImportOrderGapBetweenStaticGroups.java
import static java.lang.Math.abs;
import static java.lang.Math.cos; // Attention: gap below

import static javax.xml.transform.TransformerFactory.newInstance;

public class InputImportOrderGapBetweenStaticGroups { }

# Error about extra separation between import groups
➜  java -jar checkstyle-8.3-all.jar -c config.xml InputImportOrderGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderGapBetweenStaticGroups.java:4: Extra separation in import group before 'javax.xml.transform.TransformerFactory.newInstance' [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

# compare with Checkstyle 8.2: No error with separation
➜  java -jar checkstyle-8.2-all.jar -c config.xml InputImportOrderGapBetweenStaticGroups.java
Starting audit...
Audit done.

After removing separation: ERROR about missing separation.

➜  cat InputImportOrderNoGapBetweenStaticGroups.java
import static java.lang.Math.abs;
import static java.lang.Math.cos; // no gap below
import static javax.xml.transform.TransformerFactory.newInstance;

public class InputImportOrderNoGapBetweenStaticGroups { }

➜  java -jar checkstyle-8.3-all.jar -c config.xml InputImportOrderNoGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderNoGapBetweenStaticGroups.java:3: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

➜  java -jar checkstyle-8.2-all.jar -c config.xml InputImportOrderNoGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderNoGapBetweenStaticGroups.java:3: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

I am not able to have two static import groups following each other without non-static imports between them in 8.3.

This used to work up to 8.2 using the same configuration.

This behaviour seems to have been introduced by a736d19, the fix for #5065. The suggested test above runs OK without it: https://github.com/ssp/checkstyle/commits/static-imports-between

My impression is that checkstyle unintentionally allowed this kind of import grouping because the of issue fixed by that patch and now cannot be configured anymore to allow it. Ideally we’d have both the fix of that patch and the interspersed static import groups.

@ssp ssp changed the title from static import groups between non-static ones became impossible in 8.3 to adjacent static import groups ones became impossible in 8.3 Oct 12, 2017

@checkstyle checkstyle deleted a comment from rnveach Oct 12, 2017

@checkstyle checkstyle deleted a comment from ssp Oct 12, 2017

@checkstyle checkstyle deleted a comment from ssp Oct 12, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 12, 2017

Member

option "bottom" means - "All static imports are at the bottom."

So from first looks this is invalid issue, as Checkstyle do exactly what is required.
We need to recheck that Eclipse actually can support sorting like @ssp is provided.

If that is possible - we probably need to make new options top_separated, bottom_separated to work like "All static imports are at the top/bottom but separated by local groups."

Member

romani commented Oct 12, 2017

option "bottom" means - "All static imports are at the bottom."

So from first looks this is invalid issue, as Checkstyle do exactly what is required.
We need to recheck that Eclipse actually can support sorting like @ssp is provided.

If that is possible - we probably need to make new options top_separated, bottom_separated to work like "All static imports are at the top/bottom but separated by local groups."

@djydewang

This comment has been minimized.

Show comment
Hide comment
@djydewang

djydewang Oct 13, 2017

Contributor

@romani @ssp

➜ cat InputImportOrderNoGapBetweenStaticGroups.java
import static java.lang.Math.abs;
import static java.lang.Math.cos; // no gap below
import static javax.xml.transform.TransformerFactory.newInstance;

public class InputImportOrderNoGapBetweenStaticGroups { }

➜ java -jar checkstyle-8.3-all.jar -c config.xml InputImportOrderNoGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderNoGapBetweenStaticGroups.java:3: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

It's casued by

|| !beforeFirstImport && isAlphabeticallySortableStaticImport(isStatic)) {

the default value of sortStaticImportsAlphabetically is false,So program go to

and throw exception.
It can be resolved by adding <property name="sortStaticImportsAlphabetically" value="true"/> to config file temporarily.

I have fixed it.
Please add "approved" label.
Thanks.

Contributor

djydewang commented Oct 13, 2017

@romani @ssp

➜ cat InputImportOrderNoGapBetweenStaticGroups.java
import static java.lang.Math.abs;
import static java.lang.Math.cos; // no gap below
import static javax.xml.transform.TransformerFactory.newInstance;

public class InputImportOrderNoGapBetweenStaticGroups { }

➜ java -jar checkstyle-8.3-all.jar -c config.xml InputImportOrderNoGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderNoGapBetweenStaticGroups.java:3: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

It's casued by

|| !beforeFirstImport && isAlphabeticallySortableStaticImport(isStatic)) {

the default value of sortStaticImportsAlphabetically is false,So program go to

and throw exception.
It can be resolved by adding <property name="sortStaticImportsAlphabetically" value="true"/> to config file temporarily.

I have fixed it.
Please add "approved" label.
Thanks.

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 13, 2017

@djydewang

This comment has been minimized.

Show comment
Hide comment
@djydewang

djydewang Oct 27, 2017

Contributor

@romani
It's a bug since with config file:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="ImportOrderCheck">
            <property name="groups" value="java, javax" />
            <property name="separated" value="true" />
            <property name="ordered" value="true" />
            <property name="option" value="bottom" />
         </module>
    </module>
</module>
 cat InputImportOrderNoGapBetweenStaticGroups.java
import static java.lang.Math.abs;
import static java.lang.Math.cos; // no gap below
import static javax.xml.transform.TransformerFactory.newInstance;

public class InputImportOrderNoGapBetweenStaticGroups { }

➜  java -jar checkstyle-8.3-all.jar -c config.xml InputImportOrderNoGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderNoGapBetweenStaticGroups.java:3: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

The error is not excepted.
And I have explained for this bug in last comment.
Please add "approved" label.

Contributor

djydewang commented Oct 27, 2017

@romani
It's a bug since with config file:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="ImportOrderCheck">
            <property name="groups" value="java, javax" />
            <property name="separated" value="true" />
            <property name="ordered" value="true" />
            <property name="option" value="bottom" />
         </module>
    </module>
</module>
 cat InputImportOrderNoGapBetweenStaticGroups.java
import static java.lang.Math.abs;
import static java.lang.Math.cos; // no gap below
import static javax.xml.transform.TransformerFactory.newInstance;

public class InputImportOrderNoGapBetweenStaticGroups { }

➜  java -jar checkstyle-8.3-all.jar -c config.xml InputImportOrderNoGapBetweenStaticGroups.java
Starting audit...
[ERROR] /var/tmp/InputImportOrderNoGapBetweenStaticGroups.java:3: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

The error is not excepted.
And I have explained for this bug in last comment.
Please add "approved" label.

@djydewang

This comment has been minimized.

Show comment
Hide comment
@djydewang

djydewang Oct 29, 2017

Contributor

@romani
Please reply my last comment.
Thanks!:)

Contributor

djydewang commented Oct 29, 2017

@romani
Please reply my last comment.
Thanks!:)

@cesar1000

This comment has been minimized.

Show comment
Hide comment
@cesar1000

cesar1000 Oct 31, 2017

We have a similar problem: 8.2 would allow us to declare groups of static imports, so that our imports would look like this:

import static com.twitter.test.mockito.TwitterArgumentMatchers.anyNullable;

import static org.mockito.Matchers.anyInt;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Checkstyle 8.3/8.4 now flags this as an error (which is not right), and this is blocking the upgrade. It would be awesome if the rule was updated to allow these groups.

cesar1000 commented Oct 31, 2017

We have a similar problem: 8.2 would allow us to declare groups of static imports, so that our imports would look like this:

import static com.twitter.test.mockito.TwitterArgumentMatchers.anyNullable;

import static org.mockito.Matchers.anyInt;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Checkstyle 8.3/8.4 now flags this as an error (which is not right), and this is blocking the upgrade. It would be awesome if the rule was updated to allow these groups.

@yasserg

This comment has been minimized.

Show comment
Hide comment
@yasserg

yasserg Nov 2, 2017

We have the same issue. With separated=true checkstyle 8.4 shows error if we separate groups in static imports or not separate them.

yasserg commented Nov 2, 2017

We have the same issue. With separated=true checkstyle 8.4 shows error if we separate groups in static imports or not separate them.

@ssp

This comment has been minimized.

Show comment
Hide comment
@ssp

ssp Nov 3, 2017

still seeing the issue in 8.4 as well here

ssp commented Nov 3, 2017

still seeing the issue in 8.4 as well here

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 11, 2017

Member

@ssp , @djydewang , bug is approved, sorry for delay.

@djydewang , please do PR. We will need regression report , I will guide you in PR on how to make it.

Member

romani commented Nov 11, 2017

@ssp , @djydewang , bug is approved, sorry for delay.

@djydewang , please do PR. We will need regression report , I will guide you in PR on how to make it.

@romani romani added the approved label Nov 11, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 11, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 11, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 14, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 16, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 16, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 18, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 18, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 18, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 18, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 18, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 19, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 19, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 19, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 23, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 23, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 23, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 23, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Nov 24, 2017

rnveach added a commit that referenced this issue Nov 24, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 24, 2017

Member

Fix was merged

Member

rnveach commented Nov 24, 2017

Fix was merged

@rnveach rnveach closed this Nov 24, 2017

@rnveach rnveach added this to the 8.5 milestone Nov 24, 2017

@ssp

This comment has been minimized.

Show comment
Hide comment
@ssp

ssp Nov 25, 2017

@djydewang thanks a lot for your effort!

I just built a current repo version (8441fca) which contains your commits. For my test case it does indeed not fail for both test files, but only for one of them. However, the behaviour now seems to differ from that seen in Checkstyle 8.2.

I’m pasting my output below so you can judge whether that’s intended or not.

➜  checkstyle-5176 cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
        <module name="ImportOrderCheck">
            <property name="groups" value="java, javax" />
            <property name="separated" value="true" />
            <property name="ordered" value="true" />
            <property name="option" value="bottom" />
         </module>
    </module>
</module>

➜  checkstyle-5176 cat InputImportOrderStaticGroupsBetween.java
package com.puppycrawl.tools.checkstyle.checks.imports.importorder;

import static java.lang.Math.abs;
import static java.lang.Math.cos;

import static javax.xml.transform.TransformerFactory.newInstance;

/*
 * ^^^^^^
 * Gap between two directly adjacent static import groups.
 */
public class InputImportOrderStaticGroupsBetween {
    void method() {
    }
}

➜  checkstyle-5176 cat InputImportOrderStaticGroupsBetween2.java
package com.puppycrawl.tools.checkstyle.checks.imports.importorder;

import static java.lang.Math.abs;
import static java.lang.Math.cos;
import static javax.xml.transform.TransformerFactory.newInstance;

/*
 * ^^^^^^
 * No gap between two directly adjacent static import groups.
 */
public class InputImportOrderStaticGroupsBetween2 {
    void method() {
    }
}

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.2-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween.java
Starting audit...
Audit done.

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.5-SNAPSHOT-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween.java
Starting audit...
[ERROR] InputImportOrderStaticGroupsBetween.java:6: Extra separation in import group before 'javax.xml.transform.TransformerFactory.newInstance' [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.2-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween2.java
Starting audit...
[ERROR] InputImportOrderStaticGroupsBetween2.java:5: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.5-SNAPSHOT-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween2.java
Starting audit...
Audit done.

ssp commented Nov 25, 2017

@djydewang thanks a lot for your effort!

I just built a current repo version (8441fca) which contains your commits. For my test case it does indeed not fail for both test files, but only for one of them. However, the behaviour now seems to differ from that seen in Checkstyle 8.2.

I’m pasting my output below so you can judge whether that’s intended or not.

➜  checkstyle-5176 cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
        <module name="ImportOrderCheck">
            <property name="groups" value="java, javax" />
            <property name="separated" value="true" />
            <property name="ordered" value="true" />
            <property name="option" value="bottom" />
         </module>
    </module>
</module>

➜  checkstyle-5176 cat InputImportOrderStaticGroupsBetween.java
package com.puppycrawl.tools.checkstyle.checks.imports.importorder;

import static java.lang.Math.abs;
import static java.lang.Math.cos;

import static javax.xml.transform.TransformerFactory.newInstance;

/*
 * ^^^^^^
 * Gap between two directly adjacent static import groups.
 */
public class InputImportOrderStaticGroupsBetween {
    void method() {
    }
}

➜  checkstyle-5176 cat InputImportOrderStaticGroupsBetween2.java
package com.puppycrawl.tools.checkstyle.checks.imports.importorder;

import static java.lang.Math.abs;
import static java.lang.Math.cos;
import static javax.xml.transform.TransformerFactory.newInstance;

/*
 * ^^^^^^
 * No gap between two directly adjacent static import groups.
 */
public class InputImportOrderStaticGroupsBetween2 {
    void method() {
    }
}

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.2-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween.java
Starting audit...
Audit done.

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.5-SNAPSHOT-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween.java
Starting audit...
[ERROR] InputImportOrderStaticGroupsBetween.java:6: Extra separation in import group before 'javax.xml.transform.TransformerFactory.newInstance' [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.2-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween2.java
Starting audit...
[ERROR] InputImportOrderStaticGroupsBetween2.java:5: 'javax.xml.transform.TransformerFactory.newInstance' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 1 errors.

➜  checkstyle-5176 java \
-Duser.language=en \
-Duser.country=US \
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.5-SNAPSHOT-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween2.java
Starting audit...
Audit done.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 25, 2017

Member
-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.5-SNAPSHOT-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween.java
Starting audit...
[ERROR] .....:6: Extra separation in import group before 'javax.xml.transform.TransformerFactory.newInstance' [ImportOrder]

we updated xdoc to make it clear that grouping does not work in static, when sortStaticImportsAlphabetically is on - acfe3c0#diff-86e367c71f2d80def0ab6755375bf88a.
But in this case we use only separated that can follow grouping even with bottom mode.

@djydewang , please confirm and if so , lets create new issue on this.

Member

romani commented Nov 25, 2017

-jar /Users/ssp/Downloads/checkstyle-5176/checkstyle-8.5-SNAPSHOT-all.jar \
-c config.xml \
InputImportOrderStaticGroupsBetween.java
Starting audit...
[ERROR] .....:6: Extra separation in import group before 'javax.xml.transform.TransformerFactory.newInstance' [ImportOrder]

we updated xdoc to make it clear that grouping does not work in static, when sortStaticImportsAlphabetically is on - acfe3c0#diff-86e367c71f2d80def0ab6755375bf88a.
But in this case we use only separated that can follow grouping even with bottom mode.

@djydewang , please confirm and if so , lets create new issue on this.

@djydewang

This comment has been minimized.

Show comment
Hide comment
@djydewang

djydewang Nov 26, 2017

Contributor

@ssp It's intended now. Thanks!

@romani

If that is possible - we probably need to make new options top_separated, bottom_separated to work like "All static imports are at the top/bottom but separated by local groups."

I have created the Issue #5279 about it. Because top/bottom means "All static imports are at the top/bottom as one group" now.

But in this case we use only separated that can follow grouping even with bottom mode.

If we agree with this behaviour, then behaviour of sortStaticImportsAlphabetically is complicated since it has two funtion:

  1. Applyed for all static imports as one group.
  2. Sorting them alphabetically.

Please confirm our requirement.
Thanks!

Contributor

djydewang commented Nov 26, 2017

@ssp It's intended now. Thanks!

@romani

If that is possible - we probably need to make new options top_separated, bottom_separated to work like "All static imports are at the top/bottom but separated by local groups."

I have created the Issue #5279 about it. Because top/bottom means "All static imports are at the top/bottom as one group" now.

But in this case we use only separated that can follow grouping even with bottom mode.

If we agree with this behaviour, then behaviour of sortStaticImportsAlphabetically is complicated since it has two funtion:

  1. Applyed for all static imports as one group.
  2. Sorting them alphabetically.

Please confirm our requirement.
Thanks!

@ssp

This comment has been minimized.

Show comment
Hide comment
@ssp

ssp Dec 1, 2017

@djydewang

unfortunately it’s not really clear to me know what I need to do to upgrade to Checkstyle with this behaviour. The updated documentation mentioned by @romani does not seem to relate to the situation I am stuck with (alternating non-static and static import groups which should be separated), so it’s not clear to me how I need to configure Checkstyle > 8.2 to work with the existing code. Any hints?

ssp commented Dec 1, 2017

@djydewang

unfortunately it’s not really clear to me know what I need to do to upgrade to Checkstyle with this behaviour. The updated documentation mentioned by @romani does not seem to relate to the situation I am stuck with (alternating non-static and static import groups which should be separated), so it’s not clear to me how I need to configure Checkstyle > 8.2 to work with the existing code. Any hints?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 2, 2017

Member

@ssp , looks like @djydewang know how to reconcile all requests , you should be able to upgrade as soon #5279 is fixed.

Member

romani commented Dec 2, 2017

@ssp , looks like @djydewang know how to reconcile all requests , you should be able to upgrade as soon #5279 is fixed.

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

@eric-milles

This comment has been minimized.

Show comment
Hide comment
@eric-milles

eric-milles Jan 3, 2018

I'm not sure I can follow the current status of this issue. Using Eclipse's import ordering feature, both normal and static imports may be separated into groups. For example, all java imports then all javax imports then all java static imports then all javax static imports.

This used to be achievable in Checkstyle until the definition of "top" and "bottom" was changed without notice.

Checkstyle 8.1 and previous has been used successfully using this config:

        <module name="ImportOrder">
            <message key="import.groups.separated.internally"
              value="Blank line before import ''{0}'' should be removed" />
            <message key="import.ordering" value="Import ''{0}'' is out of order" />
            <message key="import.separation" value="Import ''{0}'' should be separated from previous import group by a blank line" />
            <property name="groups" default="java,javax,groovy,groovyx"
              value="java,javax,groovy,groovyx,/^(?!com\.(xxxxxxxxx|xxxx?|tr(gr)?))\w+/,/^com\.(xxxxxxxxx|xxxx?|tr(gr(?!\.cobalt\.(\w+\.)?${cobalt.package}))?)\./,/^com\.trgr\.cobalt\.(\w+\.)?${cobalt.package}/"
            /><!--   ^language/runtime,        ^third-party,                            ^first-party (but not this module),                                        ^module                                      -->
            <property name="option" value="top" /><!-- Place static imports above regular imports. -->
            <property name="ordered" value="false" /><!-- Eclipse's organize imports will fix order. -->
            <property name="separated" default="true" value="${cobalt.imports.separated}" />
            <property name="id" value="importOrder" />
        </module>

This is the corresponding Eclipse config:
import-order

eric-milles commented Jan 3, 2018

I'm not sure I can follow the current status of this issue. Using Eclipse's import ordering feature, both normal and static imports may be separated into groups. For example, all java imports then all javax imports then all java static imports then all javax static imports.

This used to be achievable in Checkstyle until the definition of "top" and "bottom" was changed without notice.

Checkstyle 8.1 and previous has been used successfully using this config:

        <module name="ImportOrder">
            <message key="import.groups.separated.internally"
              value="Blank line before import ''{0}'' should be removed" />
            <message key="import.ordering" value="Import ''{0}'' is out of order" />
            <message key="import.separation" value="Import ''{0}'' should be separated from previous import group by a blank line" />
            <property name="groups" default="java,javax,groovy,groovyx"
              value="java,javax,groovy,groovyx,/^(?!com\.(xxxxxxxxx|xxxx?|tr(gr)?))\w+/,/^com\.(xxxxxxxxx|xxxx?|tr(gr(?!\.cobalt\.(\w+\.)?${cobalt.package}))?)\./,/^com\.trgr\.cobalt\.(\w+\.)?${cobalt.package}/"
            /><!--   ^language/runtime,        ^third-party,                            ^first-party (but not this module),                                        ^module                                      -->
            <property name="option" value="top" /><!-- Place static imports above regular imports. -->
            <property name="ordered" value="false" /><!-- Eclipse's organize imports will fix order. -->
            <property name="separated" default="true" value="${cobalt.imports.separated}" />
            <property name="id" value="importOrder" />
        </module>

This is the corresponding Eclipse config:
import-order

@ssp

This comment has been minimized.

Show comment
Hide comment
@ssp

ssp Jan 5, 2018

@djydewang, @romani , @timurt 👍

Thanks for all your efforts, the latest checkstyle version works for our existing import orders again.

ssp commented Jan 5, 2018

@djydewang, @romani , @timurt 👍

Thanks for all your efforts, the latest checkstyle version works for our existing import orders again.

@romani romani changed the title from adjacent static import groups ones became impossible in 8.3 to ImportOrder: adjacent static import groups ones became impossible in 8.3 Mar 18, 2018

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