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

Rename blacklist to banlist within internal modules and documentation #627

Closed
wants to merge 17 commits into from

Conversation

tonybaloney
Copy link
Contributor

This proposed change replaces the use of the term "blacklist" with a replacement, "banlist".

The change is mostly an internal refactoring task.

Some notes:

  • I have added a backward-compatibility check in the configuration loader that will patch all "blacklist" settings to "banlist".
  • If the user already has files that configure using the blacklist keyword, it will continue to work.
  • The documentation would use blocklist and not refer to blacklist at all

Could the core development team please comment on this proposal?

I would gladly make amendments, continue and extend the testing to have this change merged as I believe it would benefit the Python community not to use the term blacklist in such an important project as bandit.

@ericwb @lukehinds @ehooo @viraptor

@@ -74,6 +74,7 @@ def report(manager, fileobj, sev_level, conf_level, template=None):
'abspath': lambda issue: os.path.abspath(issue.fname),
'relpath': lambda issue: os.path.relpath(issue.fname),
'line': lambda issue: issue.lineno,
'col_offset': lambda issue: issue.col_offset,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. that was unrelated to this PR.

@ehooo
Copy link
Contributor

ehooo commented Jul 8, 2020

Please fix the test,
I see that convert_blacklist_to_banlist is pending to be implemented, however I think it is important for allows retrocompatibility.

@tonybaloney tonybaloney changed the title [WIP] Rename blacklist to banlist within internal modules and documentation Rename blacklist to banlist within internal modules and documentation Jul 9, 2020
@tonybaloney
Copy link
Contributor Author

@ehooo this is now ready for a proper review.

There are tests against legacy configs as well

@ehooo
Copy link
Contributor

ehooo commented Jul 9, 2020

For me is fine, however, I cannot merge it.
@ericwb @lukehinds @viraptor could you review?

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the extension model via the rename. Therefore it needs to go through a deprecation of sorts. It should still support the extension of 'blacklist' in addition to 'banlist' for a few releases before totally removing in order to give exploiters time to migrate off that extension name. And a warning message to 'blacklist' plugins would also be good.

bandit/banlists/calls.py Outdated Show resolved Hide resolved
bandit/banlists/calls.py Outdated Show resolved Hide resolved
bandit/banlists/imports.py Outdated Show resolved Hide resolved
bandit/banlists/imports.py Outdated Show resolved Hide resolved
bandit/core/extension_loader.py Outdated Show resolved Hide resolved
@tonybaloney
Copy link
Contributor Author

@ericwb I added some logic to load and merge the old namespace and raise a PendingDeprecationWarning if that namespace has values

https://github.com/PyCQA/bandit/pull/627/files#diff-84fa5c2b26f1476808885d6e44c73f40R74-R85

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Few more changes needed.

doc/source/banlists/banlist_calls.rst Outdated Show resolved Hide resolved
doc/source/banlists/banlist_calls.rst Outdated Show resolved Hide resolved
doc/source/banlists/banlist_imports.rst Outdated Show resolved Hide resolved
doc/source/banlists/banlist_imports.rst Outdated Show resolved Hide resolved
doc/source/banlists/index.rst Outdated Show resolved Hide resolved
bandit/core/extension_loader.py Outdated Show resolved Hide resolved
@tonybaloney
Copy link
Contributor Author

@ericwb the requested changes were made

@jwstein3400
Copy link

jwstein3400 commented Dec 2, 2020

Hi. I unknowingly opened a separate issue and pushed a change under #647
I would be happy to repush my proposed change with banlist instead of blocklist.

@tonybaloney
Copy link
Contributor Author

@jwstein3400 I don't mind what its called, as long as its changed from the current terminology. Happy to close this one or you can merge any changes from here into your branch. I'm just waiting for one of the maintainers to review and merge.

Copy link
Contributor

@KOLANICH KOLANICH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

banlist is not a very correct term. The things that are in blacklists are not technically banned, but just warned about by bandit.

IMHO it is the campaign of replacement of neutral terms with the words white and black racist (we in Russia have an idiom that can literaly be translated into English as "the thief has his hat burning", usually attributed to a folk story about an improvised guilty-knowledge test to detect thieves in a crowd at market and meaning the situation when someone socially inacceptable (i.e. the thief from the story) trying to play innocent (in the story it'd be to just ignore the scream, rather that touching the hat) but fails because he is a bad in acting), not the practice of using the words being replaced.

@za
Copy link

za commented Oct 26, 2021

IMHO it is the campaign of replacement of neutral terms with the words white and black racist

What about using allowlist and blocklist term to replace whitelist and blacklist? @tonybaloney

cc: @KOLANICH

@KOLANICH
Copy link
Contributor

As I have said, I think that the whole this campaign of renaming neutral things into "more neutral" is lot of useless fuzz out of nothing and is not so smart. I.e. they have renamed master branch into main only because one of the meanings of the word "master" is "slave-owner". To be consistent they now would have to rename Master of Sciences into "Main of Sciences" ;)

@tonybaloney
Copy link
Contributor Author

Closing PR. Have had no reply from maintainers of the project and the PR is now wildly out of date/conflicting with main branch

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

Successfully merging this pull request may close these issues.

6 participants