CustomImportOrder property 'customImportOrderRules' should be String[] about checkstyle HOT 12 CLOSED

Seeing it is the setter on the check used for our properties and is set via reflection via bean properties, I wouldn't think this would be a breaking change. Users can't normally access this.

Also, I am not sure how bean properties behaves if we have 2 setter methods for one property, especially when the 2 methods are so similar. I think one would always take priority over the other. In this case it may be the alone String since it is the default with no extra conversion needed. We need to do a conversion to get String....
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java#L156

I am liking this route more than the custom override and special javadoc tag.

from checkstyle.

@rnveach good thinking, we can take advantage of varargs parameter to make backwards compatible permanent change and just do something like:

    public final void setCustomImportOrderRules(String... rules) {
        if (rules.length == 1 && !DEFAULT_CUSTOM_IMPORT_ORDER_RULES.equals(rules[0]) && rules[0].contains("###")) {
            for (String currentState : GROUP_SEPARATOR_PATTERN.split(rules[0])) {
                addRulesToList(currentState);
            }
            customOrderRules.add(NON_GROUP_RULE_GROUP);
        } else {
            Arrays.stream(rules).forEach(this::addRulesToList);
            customOrderRules.add(NON_GROUP_RULE_GROUP);
        }
    }

from checkstyle.

Please keep in mind of https://checkstyle.sourceforge.io/checks/imports/customimportorder.html#Example12-config
value="STATIC###SAME_PACKAGE(3)###THIRD_PARTY_PACKAGE###STANDARD_JAVA_PACKAGE"/>

I hope it still can be a list. I don't remember what forced us to avoid list usage as a type. Most likely to allow future extensions to reuse , in () to define list of packages inside or whatever is required.

Now I do not see big future of this Check, but unfortunately this Check is part of Google style, breaking changes in it will be big pain for many users.

Are we sure to expose this pain or migration/updates to public? Instead of eating it in our internal web site generation class.

from checkstyle.

This issue has nothing to do with website generation. ### delimited string is weird, and not consistent with other properties. Conceptually, this property is a list. There is no reason that I can find that we should not just have a comma separated list of rules. See #14216 (comment) for a fully backwards compatible way to fix this.

from checkstyle.

Now I do not see big future of this Check, but unfortunately this Check is part of Google style, breaking changes in it will be big pain for many users.

IMO, either the check is part of our repo or its not. If we don't want this check, or think it is not worth examining things or doing proper fixes, then we should just remove it and be done with it. Otherwise, we should do our best to move it forward in a positive manner and ensure we don't make our user experience worse for it either with bugs or such. I am fine with breaking backwards compatibility to fix past mistakes as long as we don't make it a constant habit.

===

As for web site generation, other issue has atleast 2-3 other alternatives for moving that forward if this issue is not the way.

from checkstyle.

and ensure we don't make our user experience worse for it either with bugs or such.

Check works fine, but update to commas with require all all people in wold who use Google style or style based on it to update configuration.

from checkstyle.

and ensure we don't make our user experience worse for it either with bugs or such.

Check works fine, but update to commas with require all all people in wold who use Google style or style based on it to update configuration.

No, this is not correct.

from checkstyle.

@nrmancuso 's solution is backwards compatible, as seen by him not updating existing tests in his PR.

from checkstyle.

I see no problems with this issue right now.

from checkstyle.

I am ok too but how we will document this? Whole word configs are ### based. People need abilities to modify what they see. Or ?

from checkstyle.

Change is backwards compatible. Our whole repo is comma separated. We can just add some verbiage to documentation we also still support deprecated ### comma-style separation.

from checkstyle.

@nrmancuso, issue is approved please update doc somehow to let people knows what to do is they have ###. But we can advocate to have list by default in all/most examples and in google_style.xml .

from checkstyle.