Respect all sniffs when reviewing PHP_CodeSniffer itself

fredden commented 8 months ago

Description

While reviewing https://github.com/squizlabs/PHP_CodeSniffer/pull/3912, I was surprised that Squiz.PHP.NonExecutableCode did not seem to complain about anything when running this over the main branch (before applying the changes from that pull request). Upon further investigation, I found that the coding standard included a parameter of -n, which ignores all warnings by default. Removing this allowed me to properly review #3912.

This pull request removes the -n parameter and makes the necessary changes to the code-base (including the rule-set) so that no warnings are omitted. This means that when we add Squiz.PHP.NonExecutableCode to the rule-set for PHP_CodeSniffer itself, the warnings it produces won't be ignored.

Suggested changelog entry

Respect warnings as well as errors from sniffs within the coding standard for PHP_CodeSniffer itself.

Types of changes

Bug fix (non-breaking change which fixes an issue)

PR checklist

[x] I have checked there is no other PR open for the same change.
[x] I have read the Contribution Guidelines.
[x] I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
[x] I have verified that the code complies with the projects coding standards.

fredden commented 8 months ago

I presume the n was added for a reason, so I'm not keen on removing it

From what I can tell, this parameter was included in the initial commit and hasn't been changed since. Perhaps @gsherwood can share some information regarding why this was included, but 8.5 years ago is a long time to remember this level of detail.

the "fixes" being made are IMO wrong. The @link tag is the correct tag to use for the links

The sniff Squiz.Commenting.VariableComment says that @link is not allowed in this context. This sniff is specifically included in the rule-set in https://github.com/squizlabs/PHP_CodeSniffer/commit/350511c45cc5d56cfb890290e528a6a7bd399ce4.

The documentation says that The @see tag indicates a reference and The @link tag indicates a custom relation, which both sound like they're suitable in this case. Given the specifically-included sniff forbids the use of @link here, I elected to replace the tag with @see, which seemed to hold a similar meaning.

Can you share some more information to help me understand why @see is wrong here?

changing the TODO to to-do, while the tag name is @todo feels wrong too.

Yes, this change I was less sure about. Would you prefer to see these two files excluded from the sniff as false positives?

fredden commented 7 months ago

Superseded by https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/122

squizlabs / PHP_CodeSniffer