PMD ruleset #5
PMD ruleset #5
Conversation
mikebell90
requested review from
SegFaultAX,
stevenschlansker,
adamzr,
rushalias and
pennello
| <exclude name="JUnit4TestShouldUseTestAnnotation" /> | ||
| <exclude name="UnusedFormalParameter"/> |
| <exclude name="JUnit4TestShouldUseTestAnnotation" /> | ||
| <exclude name="UnusedFormalParameter"/> | ||
| <!-- added --> | ||
| <exclude name="MissingOverride" /> |
There was a problem hiding this comment.
I don't actually see this rule in their docs, but why exclude it. I like @Override annotations.
There was a problem hiding this comment.
The question isn't "Do you like it" my friend ;) It's
should every persons build break if they miss an @OverRide
| <exclude name="UnusedFormalParameter"/> | ||
| <!-- added --> | ||
| <exclude name="MissingOverride" /> | ||
| <exclude name="LooseCoupling" /> |
There was a problem hiding this comment.
same reason. I'm open to discussion of course. But the balance is "Break because this is really bad" vs being more lenient
| <exclude name="MissingOverride" /> | ||
| <exclude name="LooseCoupling" /> | ||
| <!-- A prod best left to the ide --> | ||
| <exclude name="ForLoopCanBeForeach" /> |
| </rule> | ||
| <rule ref="category/java/codestyle.xml"> | ||
| <exclude name="ConfusingTernary" /> | ||
| <exclude name="FieldDeclarationsShouldBeAtStartOfClass" /> |
| <!-- nice idea, bad idea --> | ||
| <exclude name="PrematureDeclaration" /> | ||
| <exclude name="ShortMethodName" /> | ||
| <exclude name="MethodNamingConventions" /> |
There was a problem hiding this comment.
I think method names should start with lowercase, I don't have a problem with underscores.
| <exclude name="UncommentedEmptyConstructor" /> | ||
| <exclude name="UncommentedEmptyMethodBody" /> | ||
| <!-- Added for PMD 6+ Complains if constructor/class is not commented --> | ||
| <exclude name="CommentRequired" /> |
There was a problem hiding this comment.
I think all public methods and fields should have comments, but I agree it can be burdensome for obvious stuff.
| <exclude name="UselessParentheses" /> | ||
| <rule ref="category/java/errorprone.xml"> | ||
| <exclude name="CompareObjectsWithEquals" /> | ||
| <exclude name="InvalidSlf4jMessageFormat" /> |
There was a problem hiding this comment.
This should be included. I've caught this before and it's easily missed. Perfect case for PMD.
| <exclude name="DoNotUseThreads" /> | ||
| </rule> | ||
| <rule ref="category/java/performance.xml"> | ||
| <exclude name="OptimizableToArrayCall" /> |
| </rule> | ||
| <rule ref="category/java/performance.xml"> | ||
| <exclude name="OptimizableToArrayCall" /> | ||
| <exclude name="ConsecutiveAppendsShouldReuse" /> |
There was a problem hiding this comment.
Why does this exist? Can the compiler not optimize that?
|
Adam and I talked. The issue is simple - the scope of this PR is to largely keep things "mostly" as they are but ported to the new ruleset/engine. While I myself like a lot of the ones adam has mentioned, currently we've got this configured for breaking builds. There's an option to warn - based on prio - but AFAICT that means reviewing the prios of everything. That seems a huge task, and one best done at a later date. Do I summarize more or less correctly @adamzr |
(My methodology has been going through abtest, disco, and frontdoor - all complex apps and making judgings as to what is sane and fair and useful)
|
The justification is that |
Note this includes two of the 3 (TooManyFields, ClearerApi) that chris and david expressed rservartions on
…le - use charAt(0) instead. But of course fails with a arrayoutofbounds if blindly applied and once you add the check for empty, what's the point?
|
@pennello ready for re-review |
|
@pennello Agreed. |
Note: This requires the new PMD (which itself is not yet merged into parent). The new PMD provides J9 and Java 10 support, but changed the structurign.
Please feel free to make comments. Also if you want to test against your projects for fine tuning
<dep.pmd.version>6.5.0</dep.pmd.version>
<dep.plugin.pmd.version>3.10.0</dep.plugin.pmd.version>
<dep.plugin.pmd.ruleset.version>1.2.1-SNAPSHOT</dep.plugin.pmd.ruleset.version>
Will do the trick (mvn clean install this branch to get the snapshot)