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

test: change blacklist to blocklist #19227

Merged
merged 1 commit into from Jun 9, 2020
Merged

test: change blacklist to blocklist #19227

merged 1 commit into from Jun 9, 2020

Conversation

@TrentZ
Copy link
Contributor

@TrentZ TrentZ commented Jun 9, 2020

Let's use a more appropriate and clear word and discard the usage of the blacklist. Blocklist is clear and shall make everyone happy.

Let's use a more appropriate and clear word and discard the usage of the blacklist. Blocklist is clear. Happy for everyone.
@MarcoFalke MarcoFalke changed the title change blacklist to blocklist test: change blacklist to blocklist Jun 9, 2020
@amitiuttarwar
Copy link
Member

@amitiuttarwar amitiuttarwar commented Jun 9, 2020

ACK 6fc6416

@michaelfolkson
Copy link
Contributor

@michaelfolkson michaelfolkson commented Jun 9, 2020

Concept NACK. If there was consensus amongst existing black contributors in the ecosystem that they found this offensive I would reconsider. I think they would say they have better things to do than wasting their time on quibbling about terms that avoid colors (whitelist/blacklist). Perusing other projects there are black devs choosing to use whitelist/blacklist terminology.

@jonatack
Copy link
Member

@jonatack jonatack commented Jun 9, 2020

ACK 6fc6416 git grep shows these two lines to be the only uses of the word in the codebase other than for specifying colors for the GUI.

@JeremyRubin
Copy link
Contributor

@JeremyRubin JeremyRubin commented Jun 9, 2020

There are also a few uses of the term whitelist, one of which I'm responsible for (hence chiming in), that exist as external APIs.

I'm indifferent on this particular issue. I'm supportive of the underlying issue of avoiding uninclusive terms -- I just don't think that blacklist/whitelist bear the same connotations as terminology like master/slave. I am in favor of consistency; if we do decide to change this we should update all instances in a backwards-compatible way.

@sipsorcery
Copy link
Member

@sipsorcery sipsorcery commented Jun 9, 2020

ACK 6fc6416 due to easy change.

Getting rid of whitelist is a BIG change and a different matter.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 9, 2020

ACK and thanks for the change.

Looking at the 400 open pull requests, we are clearly lacking developers to review the code. Simply the possibility that one developer may directly or indirectly be offended/discriminated by code comments or symbol names is a risk that we can't afford to take.
Bitcoin Core is an international project and if there are two similar English words, we should always pick the word that is most likely understood by the majority of international English speakers. Code should be self-documenting and variable names should be self-explanatory.

This is a refactoring change in the tests to change the name of a variable, so it allows for infinite bike-shedding. There is no way this could break any functionality, so with the existing ACKs, this is ready to go in. Leaving this open any longer will in the long term only attract heated off-topic discussions, wasting our reviewers' time.

@MarcoFalke MarcoFalke merged commit 221873e into bitcoin:master Jun 9, 2020
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
x86_64 Linux [GOAL: install] [bionic] [Using ./ci/ system] Task Summary
Details
@jnewbery
Copy link
Member

@jnewbery jnewbery commented Jun 9, 2020

ACK 6fc6416

Copy link
Member

@promag promag left a comment

And maybe s/whitelist/include.

@@ -19,7 +19,7 @@
# Windows disallow control characters (0-31) and /\?%:|"<>
FILE_CHAR_START = 32 if os.name == 'nt' else 1
FILE_CHAR_END = 128
FILE_CHAR_BLACKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'
FILE_CHAR_BLOCKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'

This comment has been minimized.

@promag

promag Jun 11, 2020
Member

Suggestion FILE_CHAR_EXCLUDE.

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 11, 2020

Concept ACK

Blacklist/whitelist have never been race-related, so that's a distraction. But, "blacklist"'s primary definition is "a list of persons who are disapproved of or are to be punished or boycotted", and has caused some confusion previously given the nature of Bitcoin to be opposed to such censorship.

@BitcoinErrorLog
Copy link

@BitcoinErrorLog BitcoinErrorLog commented Jun 23, 2020

NACK any concept related to edits for racial/color concerns. "blackness" and "whiteness" are natural concepts and none of us should be wasting our time behaving like racists and attempting to cure it with passive-aggressive naming nonsense. Go do some real help if you're concerned.

@michaelfolkson
Copy link
Contributor

@michaelfolkson michaelfolkson commented Jun 23, 2020

If that is the case @luke-jr I would say blocklist is just as confusing. Blocked from what? Using the Bitcoin network? The fact that we have block(chain)s makes blocklist probably more confusing.

This has the potential to waste so much time. I note no one brought up in the Lightning dev mailing list that the use of the term "blackmail" isn't appropriate. We can't control what GitHub or other corporations do but we can control what is considered appropriate on this project and in this community. I really hope we can take a more sane and logical approach based on consensus rather than this jumpy "let's change it in case one person in the entire world finds this offensive."

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Jun 23, 2020

This change has been reviewed and ACKed by multiple regular contributors and was merged two weeks ago.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Jun 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.