Skip to content

Conversation

@mukeshpanchal27
Copy link
Member

Summary

Fixes #691

The PR add Include WordPress-Extra rules in PHPCS configuration and fix resulting problems.

For now i have exclude sqlite module files as it shows many files to change.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure Needs Review labels Apr 3, 2023
@mukeshpanchal27 mukeshpanchal27 added this to the 2.2.0 milestone Apr 3, 2023
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 A few points of feedback here, but mostly looks good.

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

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

@mukeshpanchal27
Copy link
Member Author

Thanks, @felixarntz and @joemcgill for the feedback. I have addressed that feedback, and PR is ready for review. Thanks!

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

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]>
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

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()

@mukeshpanchal27
Copy link
Member Author

Thanks @joemcgill for the review. In 1aab8fa i have address all the feedback. PR is ready for another round of feedback.

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 mukeshpanchal27 merged commit 1112cec into trunk Apr 5, 2023
@mukeshpanchal27 mukeshpanchal27 deleted the fix/691-add-wordpress-extra-rules branch April 5, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include WordPress-Extra rules in PHPCS configuration and fix resulting problems

4 participants