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.
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
modules/database/sqlite/wp-includes/sqlite/class-perflab-sqlite-pdo-engine.php
Outdated
Show resolved
Hide resolved
|
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 <[email protected]>
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.