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

Filters/ExactMatch: deprecate getBlacklist() and getWhitelist() methods #198

Closed
2 tasks done
jrfnl opened this issue Dec 31, 2023 · 12 comments · Fixed by #203
Closed
2 tasks done

Filters/ExactMatch: deprecate getBlacklist() and getWhitelist() methods #198

jrfnl opened this issue Dec 31, 2023 · 12 comments · Fixed by #203

Comments

@jrfnl
Copy link
Member

jrfnl commented Dec 31, 2023

  • Deprecate two abstract methods which have needlessly cultural insensitive method names.
  • Introduce - temporarily non-abstract - replacement methods for both: getBlockedFiles() ++getDisallowedFiles()++ and getAllowedFiles().

This is a soft deprecation (changelog only) as throwing a deprecation notice for these methods would be unnecessarily disruptive for end-users.

In version 4.0, the deprecated methods will be removed and the replacement methods will be made abstract and therefore required.

Introducing the replacement methods at the same time as the deprecation will allow external standards which contain custom filters and other packages offering custom filters, to make the filters cross-version compatible with PHPCS 3.9.0+ as well as 4.x.

When both methods are available, I propose for the replacement methods to take precedence over the deprecated methods.

The impact of this deprecation is expected to be low.

@stronk7
Copy link
Contributor

stronk7 commented Jan 5, 2024

+1 for this, just suggesting that (non-native English here!) I find allow/deny more balanced than allow/block.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 5, 2024

+1 for this, just suggesting that (non-native English here!) I find allow/deny more balanced than allow/block.

@stronk7 Thanks for your input. I'm a non-native English speaker myself, so I'm not sure what would be best in this case. I lean towards "block" as I associate "deny" with lying/truth gathering, but that is probably just me.

I'd like to hear some more opinions. @GaryJones Care to pitch in ?

For context: this how the old names were being used:

  • getWhitelist(): retrieve a list of files which should be accepted for the scan.
  • getBlacklist(): retrieve a list of files which should not be accepted for the scan.

(with accept() being the name of the Iterator method these lists are fed into)


Note: depending on what is decided, PR #202 may need to be redone/may need a follow-up and open PR #203 may need to be updated.

@GaryJones
Copy link
Contributor

There's not too much in it from an English point of view. Any attempt to discern a difference in definition would be sublte at best.

Looking at 202 quickly, it seems like the property and methods could be more descriptive along the lines of getIncludedFilesPaths() and getExcludedFilesPaths(). Bit more wordy but a bit more descriptive?

@jrfnl
Copy link
Member Author

jrfnl commented Jan 6, 2024

Looking at 202 quickly, it seems like the property and methods could be more descriptive along the lines of getIncludedFilesPaths() and getExcludedFilesPaths(). Bit more wordy but a bit more descriptive?

@GaryJones Thanks for your input. I'm not sure those names would be right though as the accept() still has additional logic which uses the input from these method to determine whether a file should be included or excluded.

Darn... naming is hard ;-)

@jrfnl
Copy link
Member Author

jrfnl commented Jan 12, 2024

Been thinking this over some more - what about: getRejectedFiles() and getAllowedFiles() ? Would that work for you @stronk7 ?

@stronk7
Copy link
Contributor

stronk7 commented Jan 12, 2024

Yeah, in my (again, non-english) brain that couple sounds balanced enough.

Note I initially propose allow/deny, because that's an often/commonly used pair (permissions, web servers, IT in general).

+1 here!

@stronk7
Copy link
Contributor

stronk7 commented Jan 12, 2024

Or, LOL, sorry... if "accept" is the key word here... why not getAcceptedFiles() and getRejectedFiles(). They are, without a doubt, a perfect pair of antonyms (any lang).

@jrfnl
Copy link
Member Author

jrfnl commented Jan 12, 2024

Or, LOL, sorry... if "accept" is the key word here... why not getAcceptedFiles() and getRejectedFiles(). They are, without a doubt, a perfect pair of antonyms (any lang).

Because the accept() method still has some additional logic which it runs on those lists.

So how about these ?

  • getAcceptableFiles() + getRejectableFiles()
  • getAllowedFiles() + getDisallowedFiles()

@stronk7
Copy link
Contributor

stronk7 commented Jan 12, 2024

As far as they are internal implementation and the goal is to get rid of the "non-inclusive" terms, I'm happy with ANY (maybe the 1st sounds better, if they are not final and and subject to change by that accept() method).

Offtopic, in case it helps, just for curiosity I looked here. Also I've seen a bunch of places where your original suggestion (allowed/blocked) was shared as good replacement in general.

TLDR; : I'd suggest you to pick one, the one that works for you and the internals 🥇. I think all them are free from any inclusiveness trouble and I'm sure that any developer will understand them without a problem.

Yeah, naming... is... horrible! 😄

@jrfnl
Copy link
Member Author

jrfnl commented Jan 12, 2024

Okay, decision time - let's go for getAllowedFiles() + getDisallowedFiles().

I'll submit a follow-up PR to #202 in a moment and will update open PR #203 (and ticket #199).

@jrfnl
Copy link
Member Author

jrfnl commented Jan 12, 2024

See PR #258. Once that has been merged, I will update PR #203.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 22, 2024

Unless there are new comments/feedback on the PR (#203), I will merge it tomorrow.

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