Fix #140: If an abstract rule is changed, run all rules #141
Conversation
UncleOwen
changed the title
| # matches Java-based rule implementations | ||
| match_data = %r{.+/src/main/java/.+/lang/([^/]+)/rule/([^/]+)/([^/]+)Rule.java}.match(filename) | ||
| unless match_data.nil? | ||
| unless match_data.nil? || match_data[3].start_with?('Abstract') |
There was a problem hiding this comment.
We also have https://github.com/pmd/pmd/blob/main/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodRule.java , which is a rule about abstract classses, but the class impl itself is not abstract.
match_data[2] would contain the rule's category (bestpractices, codestyle, ...). We have fixed set of valid categories, so we can exclude the case in #141:
| unless match_data.nil? || match_data[3].start_with?('Abstract') | |
| unless match_data.nil? || match_data[2] == 'internal' |
This would need to be generalized to something like "match_data[2] not in ('bestpractices', 'codestyle', 'documentation' ...)"
But that would not fix the example in #124 , where the abstract class is in the same package as a valid category name: net.sourceforge.pmd.lang.java.rule.codestyle.AbstractNamingConventionRule - the class name looks like a rule (because it ends with "Rule"), but it isn't a rule... But that case should maybe simply be solved by only naming concrete rules with the suffix "Rule" - which means, we rename AbstractNamingConventionRule -> AbstractNamingConvention in pmd...
There was a problem hiding this comment.
Yes, if all the AbstractFooRules were in an internal package, we could use that to filter. And that would be the better solution.
But: We can't move or rename them, without leaving a @deprecated version behind, at least until 8.0.0. And that version would have the same problem, wouldn't it? And I'd like to solve this now, not whenever 8.0.0 comes around.
And yes, if one were to change AbstractClassWithoutAbstractMethodRule.java, with this version this would trigger a full comparison instead of only comparing the output of this rule. But it wouldn't break completely!
Since I ran into this twice in short succession, I figured I might just as well fix this.
Applies a simple heuristic: If the name of a rule starts with 'Abstract', it's an abstract class. This could change any number of rules, so let's bail and just compare all rules.
My knowledge of ruby is very basic. I basically copied one of the other "compare all rules" test cases.
Closes #140