-
-
Notifications
You must be signed in to change notification settings - Fork 480
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.
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
Add Core and Extra sniffs #382
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,22 @@ | |
<ruleset name="WordPress Extra"> | ||
<description>Best practices beyond core WordPress Coding Standards</description> | ||
|
||
<rule ref="Generic.PHP.DeprecatedFunctions"/> | ||
<rule ref="Generic.PHP.ForbiddenFunctions"/> | ||
<rule ref="Generic.Functions.CallTimePassByReference"/> | ||
<rule ref="Generic.Formatting.DisallowMultipleStatements"/> | ||
<rule ref="Generic.Formatting.MultipleStatementAlignment"/> | ||
<rule ref="Generic.CodeAnalysis.EmptyStatement" /> | ||
<rule ref="Generic.CodeAnalysis.ForLoopShouldBeWhileLoop"/> | ||
<rule ref="Generic.CodeAnalysis.ForLoopWithTestFunctionCall"/> | ||
<rule ref="Generic.CodeAnalysis.JumbledIncrementer"/> | ||
<rule ref="Generic.CodeAnalysis.UnconditionalIfStatement"/> | ||
<rule ref="Generic.CodeAnalysis.UnnecessaryFinalModifier"/> | ||
<rule ref="Generic.CodeAnalysis.UnusedFunctionParameter"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of the way WordPress's filters/actions API works, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll remove it. Do we want a custom sniff that does a similar check, but doesn't report unused (e.g. 2nd-4th) variables if a later one (e.g. 5th) is used? Also, this sniff only gives warnings, not errors, so it may be permissible to keep this sniff in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My standard practice has been to just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened an upstream PR which addresses this: squizlabs/PHP_CodeSniffer#1318 |
||
<rule ref="Generic.CodeAnalysis.UselessOverridingMethod"/> | ||
<rule ref="Generic.Classes.DuplicateClassName"/> | ||
<rule ref="Generic.Strings.UnnecessaryStringConcat"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will probably disable |
||
|
||
<rule ref="WordPress-Core"/> | ||
|
||
<rule ref="WordPress.XSS.EscapeOutput"/> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think we should include
MultipleStatementAlignment
. It's really annoying, for instance:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it previously: 9f6854d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree,
MultipleStatementAlignment
is a good practice within reason, but the sniff isn't refined enough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I'd optionally put
$redirect_vars_pairs
above line 129, and align the=
out.In other cases, just adding a clear line above 134 would do - that the separates the predefined data in
$redirect_vars
, and the variable that's only populated from theforeach
.It is part of the standard.
However, I'm open to removing it if you both still insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda. It says “spaces can be used mid-line for alignment”. It doesn't say that that alignment is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have in the
README
a list of other sniffs that users may like to opt-in to, such as this, but which are perhaps even more opinionated than Extra already is.