Include WordPress-Extra rules in PHPCS configuration and fix resulting problems#695
Conversation
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 A few points of feedback here, but mostly looks good.
joemcgill
left a comment
There was a problem hiding this comment.
This is on the right track. Thanks @mukeshpanchal27! Besides the translation fix that Felix already noted, the only other thing I would like to change is the way we're handling lines that we want to ignore.
First, you should always use phpcs:ignore instead of phpcs:disable to ignore a specific line, because phpcs:disable will disable that sniff for the rest of the file, unless paired with a phpcs:enable rule later in the file.
See: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file
The other thing I'd like is for us to always include an inline comment explaining why something is being ignored, as @felixarntz suggested with the one nonce example. This makes it very clear why the line was added and helps determine if/when it can be removed in the future.
For readability, it would be easier to put these phpcs:ignore statements on the line above the code it is ignoring with the explanation right above. Here is an example from WP Core that shows what this could look like
|
Thanks, @felixarntz and @joemcgill for the feedback. I have addressed that feedback, and PR is ready for review. Thanks! |
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @mukeshpanchal27! LGTM, just a tiny not-blocking note.
Can you please open a follow up issue to address the relevant problems in the SQLite module so that we don't forget about that?
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
joemcgill
left a comment
There was a problem hiding this comment.
This is much better. I have a bit more feedback based on your changes that I have left inline. I still think that the in-line comments explaining why we are ignoring some PHPCS sniffs would be more readable if the ignore statement was right below the in-line comment on the line above the line being ignored, like this:
// PHPCS ignore reason: explanation of why this is ignored
// phpcs:ignore WordPress.PHP.SniffRule.Reason
$var = example()
|
Thanks @joemcgill for the review. In 1aab8fa i have address all the feedback. PR is ready for another round of feedback. |
Summary
Fixes #691
The PR add Include
WordPress-Extrarules in PHPCS configuration and fix resulting problems.For now i have exclude
sqlitemodule files as it shows many files to change.Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.