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

PSR1/SideEffects: improve recognition of disable/enable annotations #3772

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Mar 2, 2023

As it was, the sniff would respect PHPCS disable annotations, but only when either unqualified or qualified up to a sniff name. It would not respect a disable annotation using a sniffname with errorcode, i.e. PSR1.Files.SideEffects.FoundWithSymbols.

While the FoundWithSymbols errorcode is the only error code for this sniff and using // phpcs:disable PSR1.Files.SideEffects.FoundWithSymbols is effectively the same as using // phpcs:disable PSR1.Files.SideEffects, I do believe end-users should not need to be aware of whether or not a sniff has multiple error codes when using disable annotations.

This updates the sniff to also respect disable annotations which include the error code.

Includes unit test.

Fixes #3386

As it was, the sniff would respect PHPCS disable annotations, but only when either unqualified or qualified up to a sniff name.
It would not respect a disable annotation using a sniffname with errorcode, i.e. `PSR1.Files.SideEffects.FoundWithSymbols`.

While the `FoundWithSymbols` errorcode is the only error code for this sniff and using `// phpcs:disable PSR1.Files.SideEffects.FoundWithSymbols` is effectively the same as using `// phpcs:disable PSR1.Files.SideEffects`, I do believe end-users should not need to be aware of whether or not a sniff has multiple error codes when using disable annotations.

This updates the sniff to also respect disable annotations which include the error code.

Includes unit test.

Fixes 3386
@nwehrhan
Copy link

Hmm seems like its not happy on 8.2 :( would love to see this get working, but I haven't contributed before... so not sure how to attempt.

@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 13, 2023

Hmm seems like its not happy on 8.2 :( would love to see this get working, but I haven't contributed before... so not sure how to attempt.

The 8.2 issue is unrelated to this PR, so should not be a blocker for this PR to get merged.

@fredden
Copy link
Contributor

fredden commented Mar 14, 2023

The test failure on 8.2 should be fixed when #3773 gets merged in.

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 2, 2023

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#54

@jrfnl jrfnl closed this Dec 2, 2023
@jrfnl jrfnl deleted the feature/3386-psr1-sideffects-allow-for-ignore-with-errorcode branch December 2, 2023 02:15
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 8, 2023

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).

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.

PSR1.Files.SideEffects.FoundWithSymbols not being ignored
3 participants