Repository: checkstyle/checkstyle

Problem statement:
CustomImportOrderCheck.customImportOrderRules does not support the default value
https://checkstyle.org/config_imports.html#CustomImportOrder

In this example:
```
<property name="customImportOrderRules" 
value="${checkstyle.customimportorder.customimportorderrules}" default="{}"/>
```
I am trying to allow users to specify their own values, as recommended in [the documentation [1]](https://checkstyle.org/config.html#Properties). This approach requires a default value, and per [the documentation [2]](https://checkstyle.org/config_imports.html#CustomImportOrder_Properties), `{}` is the default.

Expected behavior: `{}` should behave the same as not setting the value at all.

Actual behavior: [checkstyle does not support `{}` [3]](https://github.com/checkstyle/checkstyle/blob/394030d57239730a5cb622baff934a12be3d0693/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java#L489)

-------

Links
1. https://checkstyle.org/config.html#Properties
2. https://checkstyle.org/config_imports.html
3. https://github.com/checkstyle/checkstyle/blob/394030d57239730a5cb622baff934a12be3d0693/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java#L488


Hints:
https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java#L1095
`{}` is used to represent an empty list, and not necessarily the value for the property. `customImportOrderRules` is converted to a list of strings from the single string you provide it.

How else should we signify an empty list?
The property is not a list. It is a `###`-delimited string. See [link #3](https://github.com/checkstyle/checkstyle/blob/394030d57239730a5cb622baff934a12be3d0693/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java#L488) that I posted earlier.

Specifying any value results in the code performing `"###".split(input)`, which can never return an empty list. If you set the value to "", then the `"###".split("")` will return `String[]{""}`, which causes the code to throw an `IllegalStateException` in `addRulesToList()` because `""` is not a valid rule.

Adding a special case handler for `"{}"` or `""` would make the code behavior match the documentation.

------

The proposed code would look something like this:

```java
    public final void setCustomImportOrderRules(final String inputCustomImportOrder) {
        if (inputCustomImportOrder == null || "".equals(inputCustomImportOrder) || "{}".equals(inputCustomImportOrder)) {
            // Default value, do nothing
            return;
        }
        for (String currentState : GROUP_SEPARATOR_PATTERN.split(inputCustomImportOrder)) {
            addRulesToList(currentState);
        }
        customImportOrderRules.add(NON_GROUP_RULE_GROUP);
    }
```
> The property is not a list. It is a ###-delimited string

The internal field of the property is a list and this is how we determine what is put on the xdocs and determine the default value to display.
https://github.com/checkstyle/checkstyle/blob/394030d57239730a5cb622baff934a12be3d0693/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java#L408

@romani Can answer about what to allow for a default value.
@scottanderson , sorry for delay in reply.

fact that `{}` is placed in our doc is mistake, there should be `""`. Setter need to be updated to expect empty string and keep empty list for `customImportOrderRules` class field.

Please be welcome to send PullRequest with fix.
I am on it

Patch:
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheck.java
@@ -135,8 +135,8 @@
  * </p>
  * <ul>
  * <li>
- * Property {@code customImportOrderRules} - Specify list of order declaration customizing by user.
- * Default value is {@code {}}.
+ * Property {@code customImportOrderRules} - Specify format of order declaration
+ * customizing by user. Default value is {@code ""}.
  * </li>
  * <li>
  * Property {@code standardPackageRegExp} - Specify RegExp for STANDARD_JAVA_PACKAGE group imports.
@@ -410,12 +410,15 @@ public class CustomImportOrderCheck extends AbstractCheck {
     /** Pattern used to separate groups of imports. */
     private static final Pattern GROUP_SEPARATOR_PATTERN = Pattern.compile("\\s*###\\s*");
 
-    /** Specify list of order declaration customizing by user. */
-    private final List<String> customImportOrderRules = new ArrayList<>();
+    /** Processed list of import order rules. */
+    private final List<String> customOrderRules = new ArrayList<>();
 
     /** Contains objects with import attributes. */
     private final List<ImportDetails> importToGroupList = new ArrayList<>();
 
+    /** Specify format of order declaration customizing by user. */
+    private String customImportOrderRules = "";
+
     /** Specify RegExp for SAME_PACKAGE group imports. */
     private String samePackageDomainsRegExp = "";
 
@@ -487,15 +490,18 @@ public final void setSortImportsInGroupAlphabetically(boolean value) {
     }
 
     /**
-     * Setter to specify list of order declaration customizing by user.
+     * Setter to specify format of order declaration customizing by user.
      * @param inputCustomImportOrder
      *        user value.
      */
     public final void setCustomImportOrderRules(final String inputCustomImportOrder) {
-        for (String currentState : GROUP_SEPARATOR_PATTERN.split(inputCustomImportOrder)) {
-            addRulesToList(currentState);
+        if (!customImportOrderRules.equals(inputCustomImportOrder)) {
+            for (String currentState : GROUP_SEPARATOR_PATTERN.split(inputCustomImportOrder)) {
+                addRulesToList(currentState);
+            }
+            customOrderRules.add(NON_GROUP_RULE_GROUP);
         }
-        customImportOrderRules.add(NON_GROUP_RULE_GROUP);
+        customImportOrderRules = inputCustomImportOrder;
     }
 
     @Override
@@ -549,7 +555,7 @@ public void finishTree(DetailAST rootAST) {
     /** Examine the order of all the imports and log any violations. */
     private void finishImportList() {
         String currentGroup = getFirstGroup();
-        int currentGroupNumber = customImportOrderRules.indexOf(currentGroup);
+        int currentGroupNumber = customOrderRules.indexOf(currentGroup);
         ImportDetails previousImportObjectFromCurrentGroup = null;
         String previousImportFromCurrentGroup = null;
 
@@ -571,13 +577,13 @@ private void finishImportList() {
             }
             else {
                 // not the last group, last one is always NON_GROUP
-                if (customImportOrderRules.size() > currentGroupNumber + 1) {
+                if (customOrderRules.size() > currentGroupNumber + 1) {
                     final String nextGroup = getNextImportGroup(currentGroupNumber + 1);
                     if (importGroup.equals(nextGroup)) {
                         validateMissedEmptyLine(previousImportObjectFromCurrentGroup,
                                 importObject, fullImportIdent);
                         currentGroup = nextGroup;
-                        currentGroupNumber = customImportOrderRules.indexOf(nextGroup);
+                        currentGroupNumber = customOrderRules.indexOf(nextGroup);
                         previousImportFromCurrentGroup = fullImportIdent;
                     }
                     else {
@@ -719,13 +725,13 @@ else if (NON_GROUP_RULE_GROUP.equals(currentGroupNumber)) {
     private String getNextImportGroup(int currentGroupNumber) {
         int nextGroupNumber = currentGroupNumber;
 
-        while (customImportOrderRules.size() > nextGroupNumber + 1) {
-            if (hasAnyImportInCurrentGroup(customImportOrderRules.get(nextGroupNumber))) {
+        while (customOrderRules.size() > nextGroupNumber + 1) {
+            if (hasAnyImportInCurrentGroup(customOrderRules.get(nextGroupNumber))) {
                 break;
             }
             nextGroupNumber++;
         }
-        return customImportOrderRules.get(nextGroupNumber);
+        return customOrderRules.get(nextGroupNumber);
     }
 
     /**
@@ -756,11 +762,11 @@ private boolean hasAnyImportInCurrentGroup(String currentGroup) {
      */
     private String getImportGroup(boolean isStatic, String importPath) {
         RuleMatchForImport bestMatch = new RuleMatchForImport(NON_GROUP_RULE_GROUP, 0, 0);
-        if (isStatic && customImportOrderRules.contains(STATIC_RULE_GROUP)) {
+        if (isStatic && customOrderRules.contains(STATIC_RULE_GROUP)) {
             bestMatch.group = STATIC_RULE_GROUP;
             bestMatch.matchLength = importPath.length();
         }
-        else if (customImportOrderRules.contains(SAME_PACKAGE_RULE_GROUP)) {
+        else if (customOrderRules.contains(SAME_PACKAGE_RULE_GROUP)) {
             final String importPathTrimmedToSamePackageDepth =
                     getFirstDomainsFromIdent(samePackageMatchingDepth, importPath);
             if (samePackageDomainsRegExp.equals(importPathTrimmedToSamePackageDepth)) {
@@ -769,7 +775,7 @@ else if (customImportOrderRules.contains(SAME_PACKAGE_RULE_GROUP)) {
             }
         }
         if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)) {
-            for (String group : customImportOrderRules) {
+            for (String group : customOrderRules) {
                 if (STANDARD_JAVA_PACKAGE_RULE_GROUP.equals(group)) {
                     bestMatch = findBetterPatternMatch(importPath,
                             STANDARD_JAVA_PACKAGE_RULE_GROUP, standardPackageRegExp, bestMatch);
@@ -781,7 +787,7 @@ else if (customImportOrderRules.contains(SAME_PACKAGE_RULE_GROUP)) {
             }
         }
         if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)
-                && customImportOrderRules.contains(THIRD_PARTY_PACKAGE_RULE_GROUP)
+                && customOrderRules.contains(THIRD_PARTY_PACKAGE_RULE_GROUP)
                 && thirdPartyPackageRegExp.matcher(importPath).find()) {
             bestMatch.group = THIRD_PARTY_PACKAGE_RULE_GROUP;
         }
@@ -895,7 +901,7 @@ private void addRulesToList(String ruleStr) {
                 || THIRD_PARTY_PACKAGE_RULE_GROUP.equals(ruleStr)
                 || STANDARD_JAVA_PACKAGE_RULE_GROUP.equals(ruleStr)
                 || SPECIAL_IMPORTS_RULE_GROUP.equals(ruleStr)) {
-            customImportOrderRules.add(ruleStr);
+            customOrderRules.add(ruleStr);
         }
         else if (ruleStr.startsWith(SAME_PACKAGE_RULE_GROUP)) {
             final String rule = ruleStr.substring(ruleStr.indexOf('(') + 1,
@@ -905,7 +911,7 @@ else if (ruleStr.startsWith(SAME_PACKAGE_RULE_GROUP)) {
                 throw new IllegalArgumentException(
                         "SAME_PACKAGE rule parameter should be positive integer: " + ruleStr);
             }
-            customImportOrderRules.add(SAME_PACKAGE_RULE_GROUP);
+            customOrderRules.add(SAME_PACKAGE_RULE_GROUP);
         }
         else {
             throw new IllegalStateException("Unexpected rule: " + ruleStr);


Test patch:
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheckTest.java
--- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheckTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheckTest.java
@@ -157,6 +157,18 @@ public void testNonSpecifiedImports() throws Exception {
         verify(checkConfig, getPath("InputCustomImportOrderDefault.java"), expected);
     }
 
+    @Test
+    public void testOrderRuleEmpty() throws Exception {
+        final DefaultConfiguration checkConfig =
+                createModuleConfig(CustomImportOrderCheck.class);
+        checkConfig.addAttribute("customImportOrderRules", "");
+        final String[] expected = {
+            "5: " + getCheckMessage(MSG_SEPARATED_IN_GROUP, "java.util.List"),
+        };
+
+        verify(checkConfig, getPath("InputCustomImportOrderEmptyRule.java"), expected);
+    }
+
     @Test
     public void testOrderRuleWithOneGroup() throws Exception {
         final DefaultConfiguration checkConfig =
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java
--- a/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java
@@ -937,11 +937,6 @@ private static String getModulePropertyExpectedTypeName(String sectionName, Clas
             // dynamic custom expression
             result = "Regular Expression";
         }
-        else if ("CustomImportOrder".equals(sectionName)
-                && "customImportOrderRules".equals(propertyName)) {
-            // specially separated list
-            result = "String";
-        }
         else if (fieldClass == boolean.class) {
             result = "Boolean";
         }
diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/imports/customimportorder/InputCustomImportOrderEmptyRule.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/imports/customimportorder/InputCustomImportOrderEmptyRule.java
new file mode 100644
--- /dev/null
+++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/imports/customimportorder/InputCustomImportOrderEmptyRule.java
@@ -0,0 +1,15 @@
+package com.puppycrawl.tools.checkstyle.checks.imports.customimportorder;
+
+import java.util.Map;
+
+import java.util.List; // violation, extra separation in import group
+
+public class InputCustomImportOrderEmptyRule {
+}
+/*
+ * test: testOrderRuleEmpty()
+ *
+ * Config = default
+ * customImportOrderRules = ""
+ *
+ */