Unify treatment of predefined and configurable rules

[codecholeric]

Summary

Back when I played around with the code to adjust the behavior of configurable rules, I started to get the impression, that the handling of preconfigured rules could be unified (and thus simplified).

Also the naming seemed slightly inconsistent now, for predefined rules, a <rule> is a single ArchRule, for configurable rules, <rule> is the class where the rules are contained and <check> is a single ArchRule. I know it would break the syntax, but I would still propose the following adjustments.

Unify the handling of predefined rule and configurable rule

If you look at <configurableRule> it has all the necessary infrastructure to execute predefined rules as well. I could imagine to just declare a class

public class PreconfiguredRules{
    static final ArchRule do_not_write_to_standard_streams = ...;
    static final ArchRule do_not_use_PowerMock = ...;
}

Then simply adjust the POM configuration

<rules>
    <preConfiguredRules>
        <rule>do_not_write_to_standard_streams</rule>
        <rule>do_not_use_PowerMock</rule>
    </preConfiguredRules>
</rules>

And the rest of the infrastructure is already there, because that's exactly how configurable rules are evaluated (would probably need to add some meta data for scope though, so a little more is needed). As a benefit you would not need any boiler plate like the extra interface all preconfigured rules have to implement at the moment, and future extensions would be limited to the very bare minimum, adding an ArchRule field to PreconfiguredRules. As far as I saw from comments on PRs, this would eliminate the confusion of how to import which path and how to test the execute(..) method.

Adjust the naming of configurable rule

To be consistent with <predefinedRule> I would suggest to rename the tag the specifies where the rules are declared to either <ruleLocation> or <declaringClass>.

Then the configurable rules could be defined like

<rules>
    <configurableRules>
        <ruleSet>
            <ruleLocation>com.mylibrary.MyCustomArchRules<ruleLocation>
            <rules>
                <rule>my_custom_rule_one</rule>
                <rule>my_custom_rule_two</rule>
            </rules>
        </ruleSet>
    </customRules>
</rules>

A little fuzzy about the extensive plurals 😉 Because in the end the child of <condigurableRules> is a plural again because it can house many <rules>.

What do you think? I would be willing to contribute / pitch in a PR if you're open to this, but I realize that it would be a major version upgrade since it would break the existing syntax of clients.

[FanJups]

Great, all in one ! I can't wait to see these changes 🙂

[vincent-fuchs]

thanks for the suggestion, I think I see where you're going, and yes, it makes sense.

But one thing though, because I have a doubt on one use case : with your approach, all predefined rules have to be in PreconfiguredRules in the "official" plugin distribution, right ? so people can't add their own private preconfigured rules in a jar and reference them in the config, right ?

[codecholeric]

Yes, people would not be able to reference their custom rule as a preconfigured rule, but would have to use a configurable rule 😉.
For me this seems like a natural behavior though, preconfigured rules are delivered with the plugin, if I want to plug in a custom rule, I use the configurable rule API.
I mean, you could add a rule location to preconfigured rules as well, which defaults to PreconfiguredRules, but then you kind of have configurable rule 😉

[vincent-fuchs]

yes, you're right, nobody would be totally blocked as such.

alright, let's see how this would go then ! We are very happy to get the inputs from various people, even if it means introducting a breaking change in the config. The project is still quite young, so hopefully not too many people are impacted ;-)

And if they are, maybe they can use CI-droid to replicate the changes in their many repositories !