phpmd / phpmd

PHPMD is a spin-off project of PHP Depend and aims to be a PHP equivalent of the well known Java tool PMD. PHPMD can be seen as an user friendly frontend application for the raw metrics stream measured by PHP Depend.
https://phpmd.org
BSD 3-Clause "New" or "Revised" License
2.34k stars 347 forks source link

Feature req: add exception white-lists to all rules #440

Open adamcameron opened 7 years ago

adamcameron commented 7 years ago

Some rules don't support exceptions. A one-off fix for UnusedLocalVariable is awaiting merging (https://github.com/phpmd/phpmd/pull/329), but I think rather than dealing on a case-by-case basis, the facility ought to be applied to all rules.

A case in point is that we need to whitelist $_ as an UnusedLocalVariable too, for the reasons cited in the original (closed) issue (https://github.com/phpmd/phpmd/issues/326), but $_ also trips the CamelCaseParameterName rule too, and that doesn't allow exceptions either. This means we need to clutter our code with @SuppressWarnings annotations, which kinda runs counter to the intent of phpmd in the first place.

I hasten to add I think this is a great tool, and it's really helped our code quality. And this is just a minor irk :-)

Cheers!

ravage84 commented 7 years ago

Hi @adamcameron

Thank you for your feature request.

I'm not against it, though we have to think this through, first.

Could you do me a favor? Could you add a table of all rules here with three infos: Name, whether the rule already supports an execption white-list and in what way it does or in what way it should do so.

Example:

Rule Name Supports Exceptions How
ShortVariable Yes Full variable name gets ignored.

This way we can get an overview and do not rush in. 😼

adamcameron commented 7 years ago

Sure thing. I'm on the clock slightly today, and then out of circulation for a coupla days, but will get onto it as soon as I can.

ravage84 commented 7 years ago

No problem. Take your time. Thanks!

ravage84 commented 7 years ago

@adamcameron could you give me hand on this?

adamcameron commented 7 years ago

Ooh bugger... completely forgot about this. On the case. Sorry about the delay.

adamcameron commented 7 years ago
Rule Name Supports Exceptions How Note
BooleanArgumentFlag No - *
ElseExpression No - *
IfStatementAssignment No - *
StaticAccess Yes List of classes to ignore
CyclomaticComplexity No - *
NPathComplexity No - *
ExcessiveMethodLength No - *
ExcessiveClassLength No - *
ExcessiveParameterList No - *
ExcessivePublicCount No - *
TooManyFields No - *
TooManyMethods No ignorepattern covers same functionality
TooManyPublicMethods No ignorepattern covers same functionality
ExcessiveClassComplexity No - *
Superglobals No Possibly same as with static access? **
CamelCaseClassName No - *
CamelCasePropertyName No List of identifiers to ignore **
CamelCaseMethodName No List of identifiers to ignore **
CamelCaseParameterName No List of identifiers to ignore **
CamelCaseVariableName No List of identifiers to ignore **
ExitExpression No - *
EvalExpression No - *
GotoStatement No - *
NumberOfChildren No - *
DepthOfInheritance No - *
CouplingBetweenObjects No - Should be coupling between classes, btw
DevelopmentCodeFragment No - unwanted-functions covers equivalent functionality
EmptyCatchBlock No - *
ShortVariable Yes List of variable names to ignore
LongVariable No List of identifiers to ignore **
ShortMethodName Yes List of method names to ignore
ConstructorWithNameAsEnclosingClass No - *
ConstantNamingConventions No List of identifiers to ignore (very edge-case with this one though)**
BooleanGetMethodName No List of identifiers to ignore (but so edgy perhaps not worth a config rule?)**
UnusedPrivateField No - *
UnusedLocalVariable No - *
UnusedPrivateMethod No - *
UnusedFormalParameter No List of identifiers to ignore (EG: the use of _ in other languages to indicate an argument that won't be used in a callback's signature)**

* = no sensible way of having an exception list ** = there might be a business case for breaking naming standards

ravage84 commented 7 years ago

Many thanks, @adamcameron !

So I'm cutting down your list to the relevant ones.

At first all the rules that support some kind of exceptions or vice versa, grouped by how the exceptions work.

Rule Name How
DevelopmentCodeFragment unwanted functions
TooManyMethods ignore pattern
TooManyPublicMethods ignore pattern
ShortVariable List of variable names to ignore
ShortMethodName List of method names to ignore
StaticAccess List of classes to ignore

I see three ways of exceptions:

ravage84 commented 7 years ago

Now to the list of rules, which could benefit from having some kind of exceptions.

Some exceptions would only make sense, if you could configure them in a case to case situation (read class/method/funcition). Just like envisioned by https://github.com/phpmd/phpmd/issues/30.

Grouped by how the exceptions work.

Rule Name How
Superglobals Unwanted list (current list = new default)
ExcessivePublicCount Case by case ignore list or pattern only
TooManyFields Case by case ignore list or pattern only
UnusedLocalVariable Case by case ignore list only
CamelCasePropertyName Ignore list or pattern
CamelCaseMethodName Ignore list or pattern
CamelCaseParameterName Ignore list or pattern
CamelCaseVariableName Ignore list or pattern
LongVariable Ignore list
ConstantNamingConventions Ignore list
BooleanGetMethodName Ignore list
UnusedFormalParameter Ignore list

But I'm not really convinced whether all of them are really a good idea. I'd love to hear @manuelpichler's opinion about it.