-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[WIP] pylint and related cleanups #6002
Conversation
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.
I'm personally in favor 👍
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.
Some quick comments. I'm not opposed to using a new linter.
Bwahahahaha. pylint crashes when run on req_install.py: pylint-dev/pylint#2591 (comment). After spending a more-than-justified amount of time digging through pylint's and astroid's code (in vain), I found a wonderful workaround, on pylint's issue tracker, for hitting the recursion limit:
:') In other news, over the next 2 weeks, I have my semester-end exams. Hopefully for my academics, I'll be spotted rarely during that time. I might start working on #5051 in the mean time. :) |
I've added 4 new commits (no new rules yet) and rebased to include current master and PEP 517 changes. ✨ |
7267e92
to
a6351b9
Compare
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Me wants to do this. Will do it, eventually. Closing due to lack of time. |
pylint is a pretty noisy and powerful linter, capable of detecting code smells and in general is more capable than flake8. One place where pylint's superior rule disabling capabilities 1 will be handy is the typing import ignores, where we won't have to annotate the every line.
It does however need some configuration before being usable. My current plan is to use pylint for a bit, get to know which linting rules are actually useful, have a discussion about them then finally adopt them for us.
As an example, this PR contains cleanups for 2 rules, I think would be reasonable to adopt. There's a couple of extra changes here as well but they're basically parts that pylint flagged and I saw a cleaner way to fix them. I suggest reviewing this PR commit by commit and looking at the entire function's body when reviewing the changes.
If @pypa/pip-committers are okay with adding pylint to our CI, let's do this. :)
TODO:
# pylint: disable=...
.