Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PMD exclude cleanup #2777

Closed
wants to merge 15 commits into from
Closed

PMD exclude cleanup #2777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
46def23
Chrome Developer Tools console snippet for PMD violations
TWiStErRob Dec 31, 2017
7174c4c
PMD.AvoidCatchingThrowable/AvoidCatchingGenericException: configure r…
TWiStErRob Dec 31, 2017
c31a39b
PMD.EmptyMethodInAbstractClassShouldBeAbstract: allow if commented
TWiStErRob Dec 31, 2017
1f53eed
PMD.ShortClassName/ShortMethodName: fix violations, and re-configure …
TWiStErRob Dec 31, 2017
c373720
PMD.ShortVariable: quick win towards enabling it (cb -> callback)
TWiStErRob Dec 31, 2017
9f02496
PMD.ShortVariable: quick win towards enabling it (suppress common occ…
TWiStErRob Dec 31, 2017
f571861
PMD.ShortVariable: fix the rest of them
TWiStErRob Dec 31, 2017
30b2fb0
PMD.ExcessiveMethodLength: Downsampler.calculateScaling
TWiStErRob Dec 31, 2017
2f51b52
PMD.ExcessiveMethodLength: Downsampler.decodeFromWrappedStreams
TWiStErRob Dec 31, 2017
971bf29
PMD.ExcessiveMethodLength: RequestBuilder.buildThumbnailRequestRecursive
TWiStErRob Dec 31, 2017
72a48f5
PMD.Excessive* is now enabled
TWiStErRob Dec 31, 2017
a6292ac
PMD.CouplingBetweenObjects: cut Glide class in half
TWiStErRob Jan 1, 2018
e767fe7
PMD.TooManyStaticImports: qualify ImageType
TWiStErRob Jan 1, 2018
520deda
PMD.AvoidBranchingStatementAsLastInLoop: move suppression to local so…
TWiStErRob Jan 1, 2018
a871189
PMD.CallSuperInConstructor: fix violations, ignore extends Object
TWiStErRob Jan 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,13 @@ public void onLoadFailed(@Nullable Drawable errorDrawable) {
}

@Override
public void getSize(@NonNull SizeReadyCallback cb) {
target.getSize(cb);
public void getSize(@NonNull SizeReadyCallback callback) {
target.getSize(callback);
}

@Override
public void removeCallback(@NonNull SizeReadyCallback cb) {
target.removeCallback(cb);
public void removeCallback(@NonNull SizeReadyCallback callback) {
target.removeCallback(callback);
}

@Override
Expand Down Expand Up @@ -218,13 +218,13 @@ public void onLoadFailed(@Nullable Drawable errorDrawable) {
}

@Override
public void getSize(@NonNull SizeReadyCallback cb) {
target.getSize(cb);
public void getSize(@NonNull SizeReadyCallback callback) {
target.getSize(callback);
}

@Override
public void removeCallback(@NonNull SizeReadyCallback cb) {
target.removeCallback(cb);
public void removeCallback(@NonNull SizeReadyCallback callback) {
target.removeCallback(callback);
}

@Override
Expand Down
87 changes: 68 additions & 19 deletions library/pmd-ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,15 @@
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">

<description>Check for flaws in Glide's codebase.</description>
<!-- Useful tool for getting a violation count per rule in the PMD HTML report (Chrome Console):
var violations = {};
$$('table tr td:last-child a').forEach(a => {
var key = a.attributes['href'].value;
violations[key] = (violations[key] || 0) + 1;
}); violations
-->

<rule ref="category/java/errorprone.xml">
<exclude name="AvoidBranchingStatementAsLastInLoop"/>
<!-- Not using beans. -->
<exclude name="BeanMembersShouldSerialize" />
<!-- wat -->
Expand All @@ -21,8 +27,6 @@
<exclude name="AvoidFieldNameMatchingMethodName" />
<!-- There are enough cases where this makes sense (typically related to logic around the number of items in a collection) that a blanket ban doesn't seem like a good idea. -->
<exclude name="AvoidLiteralsInIfCondition" />
<!-- It's clear that this is bad, but we have a number of cases where it makes sense and a blanket ban is irritating. -->
<exclude name="AvoidCatchingThrowable" />
</rule>
<rule ref="category/java/errorprone.xml/AvoidDuplicateLiterals">
<properties>
Expand All @@ -37,8 +41,6 @@
<!-- Don't need to annotate package private methods. -->
<exclude name="DefaultPackage" />
<exclude name="CommentDefaultAccessModifier" />
<!-- Optionally implemented default empty methods are fine. -->
<exclude name="EmptyMethodInAbstractClassShouldBeAbstract" />
<!-- Why make generics less clear by using shorter names? -->
<exclude name="GenericsNaming" />
<!-- No need to enforce final if it's not necessary. -->
Expand All @@ -48,24 +50,71 @@
<exclude name="OnlyOneReturn" />
<!-- Obfuscated code is best code? -->
<exclude name="LongVariable" />
<!-- This is not always true. -->
<exclude name="ShortClassName" />
<!-- A good idea but we have tons of violations. FIXME. -->
<exclude name="ShortMethodName" />
<exclude name="ShortVariable" />
<!-- We don't use in and out to mean modified or not modified by the method, it's useful to match framework methods. -->
<exclude name="AvoidPrefixingMethodParameters" />
<!-- No idea what this is supposed to accomplish. -->
<exclude name="AvoidFinalLocalVariable" />
<!-- These are often useful for clarity and explicitly suggested by Google's code style. -->
<exclude name="UselessParentheses" />
<!-- Theoretically this might be reasonable but the number of imports probably varies from class to class and this doesn't seem worth the overhead to maintain. -->
<exclude name="TooManyStaticImports" />
<!-- Lots of existing violations, not clear that the overhead is worthwhile though there are some cases where we definitely need to call super. FIXME. -->
<exclude name="CallSuperInConstructor" />
<!-- This is a reasonable idea, but in practice often the != null case is the expected case and it makes sense for it to come first. -->
<exclude name="ConfusingTernary" />
</rule>
<rule ref="category/java/codestyle.xml/CallSuperInConstructor">
<properties>
<!-- Ignore classes implicitly extending Object.
TODO report why is this not default? see definition: `count (ExtendsList/*) > 0` -->
<property name="violationSuppressXPath" value="ancestor::ClassOrInterfaceDeclaration[not(child::ExtendsList)]" />
</properties>
</rule>
<rule ref="category/java/codestyle.xml/EmptyMethodInAbstractClassShouldBeAbstract">
<properties>
<!-- Allow if commented -->
<property name="violationSuppressXPath" value="Block[@containsComment='true']" />
</properties>
</rule>
<rule ref="category/java/codestyle.xml/ShortVariable">
<properties>
<!-- Allow some short variable names which are frequently used. -->
<property name="violationSuppressXPath" value=".[
(
@Image='sb' and typeof('java.lang.StringBuilder')
and ancestor::LocalVariableDeclaration
) or (
@Image='ID' and typeof('java.lang.String')
and ancestor::FieldDeclaration[@Static='true' and @Final='true']
) or (
@Image='o' and typeof('java.lang.Object')
and ancestor::FormalParameter
and ancestor::MethodDeclarator[@Image='equals' or @Image='isEquivalentTo']
and ancestor::MethodDeclaration[@Public='true' and ResultType/Type/PrimitiveType[@Image='boolean']]
) or (
(@Image='t' or @Image='e')
and ancestor::FormalParameter
and (
typeof('java.lang.Exception')
or typeof('java.lang.Throwable')
or ../Type//ClassOrInterfaceType[@Image='GlideException']
)
) or (
@Image='is' and typeof('java.io.InputStream')
) or (
@Image='os' and typeof('java.io.OutputStream')
)
]" />
</properties>
</rule>
<rule ref="category/java/codestyle.xml/ShortClassName">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be inclined to leave this off entirely, I haven't seen an example where this has caught a class that should actually be renamed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

решай

<properties>
<!-- Consider a few short class names valid. -->
<property name="violationSuppressRegex" value="^.*like \b(Util)\b.*$"/>
<!-- Globally allow `static class Key implements Poolable` as a convention. -->
<property name="violationSuppressXPath"
value=".[@Image = 'Key']
[@Nested = 'true' and @Static = 'true']
[ImplementsList/ClassOrInterfaceType[@Image='Poolable']]
" />
</properties>
</rule>
<rule ref="category/java/performance.xml" >
<!-- Android may not behave the same as java VMs, using short can be clearer when working with binary data. -->
<exclude name="AvoidUsingShortType" />
Expand Down Expand Up @@ -120,7 +169,6 @@
<exclude name="GodClass" />
<!-- No idea how you reasonably define this. -->
<exclude name="ExcessiveImports" />
<exclude name="CouplingBetweenObjects" />
<exclude name="TooManyMethods" />
<exclude name="LawOfDemeter" />
<exclude name="NcssCount" />
Expand All @@ -134,11 +182,12 @@
<!-- TODO: explore these further. -->
<exclude name="CyclomaticComplexity" />
<exclude name="NPathComplexity" />
<exclude name="ExcessiveMethodLength" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather leave these off for now. If we want to refactor, we're better off doing so in areas that we think are important, not just because we've hit an arbitrary limit.

I think the complexity checks are somewhat more interesting.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

да

<exclude name="ExcessiveClassLength" />
<exclude name="ExcessivePublicCount" />
</rule>
<rule ref="category/java/design.xml/AvoidCatchingGenericException">
<properties>
<!-- This is redundant, also caught with AvoidCatchingNPEs. -->
<exclude name="AvoidCatchingGenericException" />
<property name="violationSuppressXPath" value=".[@Image='NullPointerException']"/>
</properties>
</rule>

<rule ref="category/java/multithreading.xml">
Expand Down
Loading