CustomImportOrder should check that import groups are separated by one line only #3551

Closed
MEZk opened this Issue Nov 13, 2016 · 3 comments

Comments

Projects
None yet
4 participants
@MEZk
Contributor

MEZk commented Nov 13, 2016

Based on the discussion at #3519, CustomImportOrder check should validate the exact number of blank lines between groups if separateLineBetweenGroups option is set to true.

Google Style Guide (section 3.3.3) says that If there are both static and non-static imports, a single blank line separates the two blocks. There are no other blank lines between import statements. In addition, code formatters in popular IDEs remove double blank lines betwen groups (see discussion at #2143). Thus, lets restrict the number of blank lines between import groups to one.

The current implementation of CustomImportOrder allows to have more than one blank line between import groups and this leads to inability of covering the new Google Style Guide (section 3.3.3) rule with the check.

Current behavior:

$ cat TestClass.java
import static com.puppycrawl.tools.checkstyle.utils.AnnotationUtility.containsAnnotation;
import static com.puppycrawl.tools.checkstyle.utils.AnnotationUtility.getAnnotation;


import com.puppycrawl.tools.checkstyle.checks.design.FinalClassCheck;
import com.puppycrawl.tools.checkstyle.checks.design.ThrowsCountCheck;
import com.puppycrawl.tools.checkstyle.checks.design.VisibilityModifierCheck;
import com.sun.accessibility.internal.resources.*;
import java.util.Arrays;
import java.util.BitSet;
import java.util.Map;
import java.util.Map.Entry;
import java.util.NoSuchElementException;
import javax.accessibility.Accessible;
import org.apache.commons.beanutils.converters.ArrayConverter;

public class InputCustomImportOrderValid {
}

$ cat TestConfig.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="CustomImportOrder">
	    <property name="sortImportsInGroupAlphabetically" value="true"/>
	    <property name="separateLineBetweenGroups" value="true"/>
	    <property name="customImportOrderRules" value="STATIC###THIRD_PARTY_PACKAGE"/>
	</module>
    </module>
</module>

$ java -jar checkstyle-7.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

Expected: violation on line 4 since there are more than one line which separates the import groups.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@tcNickolas

This comment has been minimized.

Show comment
Hide comment
@tcNickolas

tcNickolas Nov 16, 2016

Contributor

If my understanding is correct, the fix for this is to modify hasEmptyLineBefore method to check that there is exactly one line before the token. If this is right, and nobody's working on it yet, can I try to do this?

Contributor

tcNickolas commented Nov 16, 2016

If my understanding is correct, the fix for this is to modify hasEmptyLineBefore method to check that there is exactly one line before the token. If this is right, and nobody's working on it yet, can I try to do this?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Nov 16, 2016

Contributor

@tcNickolas
Not exactly. We also need to control the number of lines which separate the imports inside the same group (should not be any lines). The method 'hasEmptyLineBefore' now is used just to control the number of lines between import groups. Please, pay attention to this fact.

Contributor

MEZk commented Nov 16, 2016

@tcNickolas
Not exactly. We also need to control the number of lines which separate the imports inside the same group (should not be any lines). The method 'hasEmptyLineBefore' now is used just to control the number of lines between import groups. Please, pay attention to this fact.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 12, 2016

Member

no actions for 1 month, issue is assigned to @kazachka .

Member

romani commented Dec 12, 2016

no actions for 1 month, issue is assigned to @kazachka .

kazachka added a commit to kazachka/checkstyle that referenced this issue Dec 26, 2016

kazachka added a commit to kazachka/checkstyle that referenced this issue Dec 28, 2016

kazachka added a commit to kazachka/checkstyle that referenced this issue Dec 28, 2016

romani added a commit that referenced this issue Dec 30, 2016

@romani romani added this to the 7.4 milestone Dec 30, 2016

@romani romani closed this Dec 30, 2016

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