3.0: Remove deprecated Sniff::has_whitelist_comment() method and all references to it
#1908
Conversation
…erences to it
1. Remove the method which was deprecated in WPCS 2.0.0.
2. Remove all function calls to the method.
Includes:
* Removing the `SlowQuerySniff::process_token()` method.
This will now fall through automatically to the method in the parent class.
* Removing the `GlobalVariablesOverrideSniff::maybe_add_error() method.
This method no longer has a function.
3. Adjusting the unit tests.
* Where tests could be removed without having to renumber a lot, especially when the test was more about testing the whitelist functionality than testing the sniff, I've removed them.
* In all other cases, I've changed the previous deprecation warning in the lines lists to a "normal" error/warning and annotated the change in behaviour.
* Note: in the `PrefixAllGlobals` tests, there were two testcases which would previously throw a warning for the whitelist comment, but where the whitelist comment had no function anyway as the sniff had already been adjusted to no longer throw an error for those cases. For those, I've removed the whitelist comments.
Includes:
* Removing an exclusion for a whitelist comment deprecation notice from the WPCS native PHPCS ruleset.
* Updating the instructions regarding the old whitelist comments in the CONTRIBUTING file.
:question_mark: As an alternative, the reference to the old-style whitelist comments could be removed completely from the CONTRIBUTING file.
Fixes 1583
|
In some places, I've seen comments in tests like
Does this make sense? It's just with everything going on in the world atm, maybe we should change this?
I'm for this. The change will be written in the changelog so no information is lost. |
|
@dingo-d Good point, I fully agree. I created this commit way before that discussion got started (april 2020 - see commit date) and have to admit that I didn't review it with that in mind before pulling it. With this commit, the issue is largely gone from the sniff code, though the wording in For the test files, I could change the wording from something like I'll work on changing this and add that as a separate commit to this PR (which can be squashed on merge). 👉 IIRC, there are some more places where the whitelist/blacklist terminology is used - PrefixAllGlobals comes to mind. Maybe we should open a separate issue as a reminder to address those too ? And while we're at it, we may want to open an issue about renaming the |
|
Oh and we should probably also open an issue as a reminder to remove/update the wiki page about these type of "whitelist" comments to use "ignore comments" instead. That update can be done ASAP and would not need to wait till the release. |
... which explain what something is testing and/or what has changed.
…omments ... as support for those is now completely removed.
|
PR updated for language used - see the three new commits (please squash on merge). |
Argh, sorry! |
Includes:
SlowQuerySniff::process_token()method.This will now fall through automatically to the method in the parent class.
GlobalVariablesOverrideSniff::maybe_add_error()method.This method no longer has a function.
PrefixAllGlobalstests, there were two testcases which would previously throw a warning for the whitelist comment, but where the whitelist comment had no function anyway as the sniff had already been adjusted to no longer throw an error for those cases. For those, I've removed the whitelist comments.Includes:
👉 ❓ Question: As an alternative, the reference to the old-style whitelist comments could be removed completely from the CONTRIBUTING file. Should we ?
Fixes #1583