*: s/whitelist/allowlist/, s/blacklist/blocklist/#49946
*: s/whitelist/allowlist/, s/blacklist/blocklist/#49946craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Please also add a linter for the future |
|
Adding a reviewer for appdev and sql, since looks like we have approval from kv. |
solongordon
left a comment
There was a problem hiding this comment.
There are a few instances of "black list" and "white list" as well.
Reviewed 12 of 55 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)
pkg/cmd/roachtest/blocklist_test.go, line 31 at r1 (raw file):
func TestBlocklists(t *testing.T) { if _, ok := os.LookupEnv(runBlocklistEnv); !ok { t.Skipf("Blackist test is only run if %s is set", runBlocklistEnv)
Missed due to misspelling
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/blocklist_test.go, line 27 at r2 (raw file):
const githubAPITokenEnv = "GITHUB_API_TOKEN" const runBlocklistEnv = "RUN_BLOCKLIST_TEST"
TBH, "blocklist" is not a very clear term to me, at least for these ORM nightly tests. (I can't speak for every other test in the repo). in fact, i think the only meaning it has to me is because it sounds very similar to "blacklist."
would a term like "expectedFailList" be ok for the ORM tests -- that's exactly what is being tracked in the lists.
rafiss
left a comment
There was a problem hiding this comment.
thanks for thinking of this change!
is it missing a "Release note: none" ?
Reviewable status:
solongordon
left a comment
There was a problem hiding this comment.
Reviewed 17 of 55 files at r1.
Reviewable status:
pkg/cmd/roachtest/blocklist_test.go, line 27 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
TBH, "blocklist" is not a very clear term to me, at least for these ORM nightly tests. (I can't speak for every other test in the repo). in fact, i think the only meaning it has to me is because it sounds very similar to "blacklist."
would a term like "expectedFailList" be ok for the ORM tests -- that's exactly what is being tracked in the lists.
I'd say this is the perfect opportunity to choose more descriptive names like "expected fail list" on a case-by-case basis. I think "blocklist" is a reasonable starting place though. I agree it's vague but I would say no vaguer than "blacklist."
rafiss
left a comment
There was a problem hiding this comment.
that sounds fine. i can change the ORM related tests after this is merged.
Reviewable status:
Allow and block aren't hurtful terms, and they also happen to be clearer than white and black - they connote meaning instantly. c.f.: https://twitter.com/bradfitz/status/1269449722063773696?s=20 rails/rails#33677 https://gitlab.com/gitlab-org/gitlab/-/issues/7554 lagom/lagom#2532 https://bugs.chromium.org/p/chromium/issues/detail?id=981129 Release note: None
|
Good point, I didn't actually think about all of the cases here, just used search and replace. But you're right that blocklist doesn't really apply here. I also agree that you can do that in another PR - thanks for volunteering @rafiss :) Raphael, I think a linter is an interesting idea, but we also don't have linters for other phrases we avoid really, so why this one? I'd like to merge without a linter and see what happens. I'll do my part to remind colleagues to avoid blacklist/whitelist when it comes up, and you should too. I think there's a "When in Rome" argument to be made here - when I joined CockroachDB, I named the main TeamCity server the "teamcity leader" because I wanted to fit in with the leader/follower terminology we use here, even though it doesn't ... really make a ton of sense to call the main TeamCity server the leader in the same way a Raft leader is a leader. So, maybe I'm an optimist, but I don't think we need a linter. Can always revisit if it matters! I fixed the missing cases and added a release note, so going to merge. TFTRs all. bors r+ |
Build succeeded |
Allow and block aren't hurtful terms, and they also happen to be clearer
than white and black - they connote meaning instantly.
Even though blacklist and whitelist don't have racist intent, they nevertheless
propagate an unconscious "white = allow", "black = deny" meaning. There's no
need to keep these terms - they're just terms. This is just one tiny thing we can
do to improve our industry's inclusivity.
c.f.:
https://twitter.com/bradfitz/status/1269449722063773696?s=20
rails/rails#33677
https://gitlab.com/gitlab-org/gitlab/-/issues/7554
lagom/lagom#2532
https://bugs.chromium.org/p/chromium/issues/detail?id=981129
Release note: None