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

Reduce use of blacklist/whitelist terminology #3961

Merged
merged 8 commits into from
Mar 9, 2021

Conversation

pkolbus
Copy link
Contributor

@pkolbus pkolbus commented Nov 29, 2020

Steps

  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

The terms whitelist/blacklist are considered problematic/oppressive, and usage runs contrary to many projects' goals of fostering an inclusive and welcoming community. This PR aims to reduce usage in cases and ways that are nonbreaking, and especially to allow pylint users to remove usage of these terms in their projects.

For further discussion of the concern see e.g.:

Type of Changes

Type
🐛 Bug fix
📜 Docs

Related Issue

Closes #3669

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 90.888% when pulling f0079a4 on pkolbus:inclusive-terminology into 2e21eb4 on PyCQA:master.

@coveralls
Copy link

coveralls commented Nov 29, 2020

Coverage Status

Coverage decreased (-0.006%) to 91.423% when pulling 90af4f3 on pkolbus:inclusive-terminology into 0f1245c on PyCQA:master.

@pkolbus
Copy link
Contributor Author

pkolbus commented Nov 29, 2020

Note for reviewers- this makes significant progress but there are still a few aspects I'd still like to address. From my viewpoint, these could be either in this PR or a follow-on. I'm interested in your thoughts, or pointers in these areas:

  • astroid has some problematic API names (e.g. AstroidManager.extension_package_whitelist); astroid itself would need a PR then pylint is updated to use the new terms.
  • the dest names black_list and black_list_re in PyLinter.make_options(); black_list is noted in documentation for plugins. Can probably do this with some aliasing extensions in the options mixin classes, and use of __getattr__ / __setattr__ overrides.
  • the term master (primarily as a pylintrc section name) is also considered problematic. This likely requires work in the options mixins, but I'm less certain how to accomplish this for section/group names.

@Pierre-Sassoulas
Copy link
Member

Changing the "master" in the options and configuration would break compatibility with all the pylint configuration files ever written. This would impact a huge amount of person and would make a lot of documentation and question on stackoverflow and everywhere else obsolete. The change on blacklist/whitelist also breaks the options given to pylint (only for those using those two options, so it's less disruptive), but it still need to be done as a breaking change for 3.0. I expect those kind of changes to be included in a big refactor of the configuration and option parser to use something modern like click or argparse and pyproject.toml that would affect pylint, but also pylint integration in emacs and other editor. I think you saw for yourself that the OptionMixin are not the best example of straightforward and elegant code either, so there's also that to take into account. We would need a tool to upgrade the configuration automatically and or a proper documentation for the migration. So this change is not going to be easy nor quick (similar to the one from isort 4 to isort 5). Also, as you said, astroid should be modified too and probably be modified first, maybe let's start here as change to astroid only affect pylint :)

@Pierre-Sassoulas Pierre-Sassoulas added the High effort 🏋 Difficult solution or problem to solve label Nov 29, 2020
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0 milestone Nov 29, 2020
@pkolbus
Copy link
Contributor Author

pkolbus commented Nov 29, 2020

@Pierre-Sassoulas, thanks for the feedback.

I do understand that pylint is very widely used, so this PR does aim to preserve backwards compatibility and provide a transition path for users. That would also be true for the unfinished parts. For example, if changing the section name to main, pylint would continue to accept master- or even not deprecating master yet but simply accepting main as an equivalent synonym/alias too. Likewise for config.black_list and config.black_list_re. (If there’s a backward compatibility case that I’ve not accounted for, please point out what no longer works but needs to).

I have not actually studied the mixin code in depth yet, but would certainly withdraw my proposal to add the section name and dest name aliases in this PR if those changes damage the readability of the options mixin classes.

Is 3.0 or the option parser refactoring planned already? If so I’m happy to build on (or possibly contribute to) that work, and am also comfortable with including this PR only for 3.0 if backwards compatibility is in doubt or impossible.

I’ll work on the astroid PR soon.

@Pierre-Sassoulas
Copy link
Member

I think most maintainer that had to touch it at some point have it on their radar. The thing is touching the OptionProviderMixin is not something we can do easily, because it's so huge, because all checkers inherit from it and because it's using Optparse that is deprecated in favor of argparse since python 3.2. So it's moving slowly.

We need this refactor for something very useful though : per directory configuration. So, I think the easier way to make that possible, would be to contribute here: https://github.com/PyCQA/pylint/projects/1, and probably this one in particular : #2355

Copy link

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@gpshead
Copy link

gpshead commented Jan 25, 2021

FWIW, I suggest doing this in bite sized pieces. This is already one bite.

make other PRs for other issues. things like [MASTER] in the config file can be addressed, it's a matter of updating the code to look for multiple possible section names for that. but those belong in their own PR to keep things smaller and reviewable.

I also suggest focusing on external user visible things first. as this PR mostly does.

internal API names inside the code can be their own cleanup if desired (like the astroid api name).

End users see command line flags, config files, and lint warning messages every day. A far fewer number of people look within the code.

@pkolbus
Copy link
Contributor Author

pkolbus commented Jan 25, 2021

Thanks. I still intend to continue with this initiative, just haven't found the time recently. If you think this PR is too big of a first bite or anything might be too risky, let me know what should be deferred to later PRs.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I reviewed the actual changes and it look like this is not making any breaking changes so it could go in 2.7.0 after a rebase on latest master.

We're never going to break compatibility for the old name. That would be forcing maintenance work on pylint users for political reasons. The deprecation warnings imply that we will do that in the future so I think that they have to be removed.

@Pierre-Sassoulas Pierre-Sassoulas added Waiting on author Indicate that maintainers are waiting for a message of the author and removed Breaking changes for 3.0 🦤 Work in progress labels Feb 20, 2021
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0, 2.7.0, 2.8.0 Feb 20, 2021
@pkolbus
Copy link
Contributor Author

pkolbus commented Feb 22, 2021

Thanks for the feedback. Aside from the political motivation, there’s also value in having clearer terminology — but regardless of the motive, I agree deprecation is a separate concern and isn’t a requirement here.

Will get back to this shortly.

ChangeLog Outdated Show resolved Hide resolved
@pkolbus pkolbus marked this pull request as ready for review March 7, 2021 16:13
@pkolbus
Copy link
Contributor Author

pkolbus commented Mar 7, 2021

This should be ready for review again. I've dropped the pyreverse commit for now. I think it would be better for a future PR so that this one can focus on the most common user-facing use of this term.

Happy for this to land in 2.8 to not disrupt 2.7.x bug fixing. The fix for #3701 in particular might need this PR gets another rebase, so I'll keep an eye on that.

@Pierre-Sassoulas Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Waiting on author Indicate that maintainers are waiting for a message of the author labels Mar 7, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The fact that there was an old "dest": "black_list_re", and no problem in the tests make me fearful. I don't think we have integration test where we launch pylint and test the actual parsing 😨. What do you think ? Apart from that I made some comment on naming but this not very important. Great job !

pylint/checkers/base.py Outdated Show resolved Hide resolved
pylint/checkers/base.py Outdated Show resolved Hide resolved
@@ -175,8 +175,8 @@ def make_options():
"metavar": "<pattern>[,<pattern>...]",
"dest": "black_list_re",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be changed too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming the dest names (black_list, black_list_re, extension_pkg_whitelist etc.) could be breaking for plugins and other programmatic access since these are the member names in the config object. doc/how_tos/plugins.rst even sets black_list in the example.

It might be possible to clean this up in a way that's still backwards compatible -- I'll need to study optparse further since I'm not familiar with it. If optparse.Values doesn't provide a built-in way to alias the names, we could probably extend optparse.Values and add some @property annotations or __getattr__/__setattr__ to provide compatibility. Or as you've noted previously, maybe it's time for bigger rework of the config system. But because it won't make a difference to most users, this seems like something to discuss and address in a later issue/PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, lack of sleep, you're right, we should not touch it (not before 3.0 at least).

Replace usage of the term 'blacklist' in the context of ignored files
and directories (--ignore and --ignore-patterns), except in cases where
backward compatibility is needed. In documentation and help, supplement
'ignore' with 'skip'; in code use the term 'ignore list'.
Replace blacklist/whitelist terminology in the context of identifier
names. The message identifier 'blacklisted-name' will be addressed in
a future patch; a transition path is needed because this message may
be referenced in pylintrc files or disable comments.
Add an option extension-pkg-allow-list to the main checker. This is an
alternate name for extension-pkg-whitelist.
In the base checker, change the 'blacklisted-name' message to
'disallowed-name'. For backward compatibility, blacklisted-name is
an old_name for disallowed-name.
Replace usage of a whitelist parameter in checkers.python3.
Replace usage of a whitelist mention in a checkers.imports comment.
Add an entry to whatsnew/2.8.rst announcing the change.
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for this merge request, that was a lot of work !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 28e8e2a into pylint-dev:master Mar 9, 2021
@ashb
Copy link

ashb commented Mar 9, 2021

Thanks for picking this up @pkolbus! ❤️

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Mar 9, 2021
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.9.0, 2.8.3 May 4, 2021
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.8.3, 2.9.0 May 31, 2021
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Oct 25, 2021
4 tasks
@Talador12
Copy link

Talador12 commented Dec 3, 2021

Quoting @gpshead and continuing my discussion with @Pierre-Sassoulas

things like [MASTER] in the config file can be addressed

It looks like this has not yet been addressed. There has been discussion about this here and on #5465

To change the references to MASTER in the code to support MASTER and MAIN requires:

  • 4 references to MASTER in run.py
  • 2 references to MASTER in lint_module_test.py
  • a constant MAIN_CHECKER_NAME found in constants.py
  • tests using a lower case master
  • documentation using all forms of master

If we are able to support both MAIN and MASTER, then we can update all docs to MAIN and still be backwards compatible. Changing those six MASTER references should be quick, but having a flexible MAIN_CHECKER_NAME does not look as easy. Only 11 references to the variable itself, but the value is inherited into a web of use cases. From constants.py:

# You probably don't want to change the MAIN_CHECKER_NAME
# This would affect rcfile generation and retro-compatibility
# on all project using [MASTER] in their rcfile.
MAIN_CHECKER_NAME = "master"

This is not something requested for political reasons. It is required in many technology organizations to use this terminology. There has already been great work on this effort, but this MASTER to MAIN request is another that should be handled eventually. It is clear that backwards compatibility to old terminology is required, but defaults should use new terminology going forward.

@Pierre-Sassoulas
Copy link
Member

@Talador12 please don't spam older issues. I locked #5465 for a reason.

should be handled eventually
should use new terminology

I will add that no one can order open-source contributor around. We're volunteer and we work on what we want to work. Decisions are reached via consensus between maintainers and active contributors. So you're especially in no position to take decision or give order as you never contributed to pylint and just opened your first issue. So be mindful of your phrasing here. I'm ready to do the changes required so that it's transparent for users. Could you assign me #XXXX, please would work better than what you're currently saying.

You can open a new issue for the master to main change that you researched here (thank you) so it can be discussed and is not lost in this old MR. The fact that we won't do breaking changes until it's painless for users will not be up for discussion.

@pylint-dev pylint-dev locked as off-topic and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
High effort 🏋 Difficult solution or problem to solve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename extension-pkg-whitelist to extension-pkg-allow-list
6 participants