Skip to content
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

Respect all sniffs when reviewing PHP_CodeSniffer itself #3914

Closed
wants to merge 5 commits into from

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Oct 29, 2023

Description

While reviewing #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

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • 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).
  • I have verified that the code complies with the projects coding standards.

This was previously ignored due to the use of '-n' to ignore all warnings.
There are too many violations of this sniff to warrant fixing these here.
Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

@fredden I see where you are coming from, but I'm not keen on accepting this.

On the one hand, I presume the n was added for a reason, so I'm not keen on removing it (though I run locally with w to show warnings anyway).

On the other hand, the "fixes" being made are IMO wrong. The @link tag is the correct tag to use for the links and changing the TODO to to-do, while the tag name is @todo feels wrong too.

I think the better approach would be for me to update PR #3912 to change the warning from that sniff to an error.

@fredden
Copy link
Contributor Author

fredden commented Oct 30, 2023

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 350511c.

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
Copy link
Contributor Author

fredden commented Dec 4, 2023

Superseded by PHPCSStandards/PHP_CodeSniffer#122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants