-
Notifications
You must be signed in to change notification settings - Fork 170
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
Adopts blocklist/allowlist as suitable alternatives for the harmful terms blacklist/allowlist #312
base: master
Are you sure you want to change the base?
Adopts blocklist/allowlist as suitable alternatives for the harmful terms blacklist/allowlist #312
Conversation
1434c0a
to
6eabce0
Compare
…erms blacklist/whitelist Closes instacart#310
6eabce0
to
d7fec97
Compare
I expect that maintainers (and users) will want this correction to be made through deprecations as was done for other harmful metaphors. I'll spend some time today on this. |
…_duration config and Makara::Errors classes
@@ -38,25 +38,25 @@ def _makara_shard_id | |||
@config[:shard_id] | |||
end | |||
|
|||
# has this node been blacklisted? | |||
def _makara_blacklisted? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to provide some kind of backward compatible support for these old methods? I would have assumed "yes" because they are public methods, but the _
prefixes in the method names indicate to me that it is not intended for users to call or override these methods themselves.
@mathieuripert, hi, I hope you are well. I'm tagging you here because I recognize your name from contributions to hostolab/covidliste and I thought that maybe you could help unlock some progress here. This MR has been around for a little while now -- I'm wondering if these changes are something that Instacart's development team would be interested in merging. I see the originally reported issue (#310) has received some down thumbs, but I'm assuming (hoping) those aren't coming from maintainers of this project. I'm happy to do more work on this if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block/allow are nice and clear.
Do you have a plan for backward compatibility? Deprecating the error class names is the tough one IMO. Need to emit a deprecation warning when those are referenced, like Rails does.
Agree the underscore-prefixed methods are "library-private" public methods. Unless they state that they're meant to be overridden in the class they're included in, or subclassed, then it's fair game to change.
Out of an abundance of caution, the library could feature-detect whether it responds_to? the old methods, suggesting something was expecting to override them. IMO it's fine not to, though, and rely on the app doing the override to catch the regression with its own suite.
It's clear that this project has taken steps to correct usages of harmful terminology (as made evident by the deprecated usage of the "master/slave" metaphor).
This change would continue that effort by replacing the "blacklist/whitelist" metaphor with the more technically descriptive terminology of "blocklist/allowlist".
Quoting Terminology, Power and Offensive Language, a document from the Internet Engineering Task Force (IETF) which discusses both harmful metaphors: