Adopting use of flake8 plugins #5537
Comments
pradyunsg
added
the
type: maintenance
|
|
|
Actually, all of those are errors from a newer version of flake8. I have a local branch that fixes that. It might also be a PR already but in case it isn't, I'll make one for that in the evening today. (on mobile, it's 3am, I'm too sleepy to do anything) |
Indeed >.< |
Yeah. FWIW, pylint doesn't really have sane defaults and it just comes down to actually sitting down and deciding what checks should be a part of your code-style and then enforcing them. I'm personally not convinced that to do this though. Though, I do think if someone wants to put in the effort to do that, it'd be a net positive at the end of it for everyone.
|
|
Bumping this, to check if folks are still interested in these checks. |
|
Noting |
pradyunsg
added
the
help wanted
|
If anyone wants to work on this, the way we'd add a flake8 plugin to our linting setup, is to add an additional_dependencies: [
'flake8-bugbear==19.8.0',
'flake8-coding==1.3.2',
'flake8-comprehensions==3.0.1',
'flake8-debugger==3.2.1',
'flake8-deprecated==1.3',
'flake8-docstrings==1.5.0',
'flake8-pep3101==1.2.1',
'flake8-polyfill==1.0.2',
'flake8-print==3.1.4',
'flake8-quotes==2.1.1',
'flake8-string-format==0.2.3',
]Note: Do not use the exact versions/plugins as in the example above -- please check what plugins have been discussed in this issue and what their current versions are. Relevant: pre-commit/pre-commit-hooks#311 |
|
@pradyunsg I recently discovered a flake8 plugin collection that I mostly like: https://wemake-python-stylegui.de/. It even supports a legacy-first mode (where it doesn't yell at you for the errors that existed prior to the integration of the linter). |
|
Yea, I've seen that. Sadly, one of the objectives is to "Enforce python3.6+ usage". :( |
Where? I'm pretty sure it's Python 2.7 compatible, I'm trying it out on Cheroot now. If there's some plugins that are incompatible, they are easily disabled. |
|
Hi @pradyunsg Is this something I can take up? I assume that after adding this plugins, we would also have to fix code which doesn't follow the conventions set by these. |
|
Revisiting this issue, how would we want to add flake8 plugins mentioned in the issue? Do we want to add them one at a time, fix the codebase to get rid of any warnings that the plugin generates and then move on to the next one? |
Sounds about right to me. We'd basically need to install them as |
Okay, so I added all flake8-plugins mentioned above in pre-commit task with their latest versions $ git diff
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 0a7847c1..689f1f3a 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -20,6 +20,19 @@ repos:
rev: 3.8.1
hooks:
- id: flake8
+ additional_dependencies: [
+ 'flake8-bugbear==20.1.4',
+ 'flake8-coding==1.3.2',
+ 'flake8-comprehensions==3.2.2',
+ 'flake8-debugger==3.2.1',
+ 'flake8-deprecated==1.3',
+ 'flake8-docstrings==1.5.0',
+ 'flake8-pep3101==1.3.0',
+ 'flake8-polyfill==1.0.2',
+ 'flake8-print==3.1.4',
+ 'flake8-quotes==3.2.0',
+ 'flake8-string-format==0.3.0',
+ ]
exclude: tests/databut then on running Is flake8 actually picking the dependencies, or is there anything else we need to add here? |
|
Do you need to rebuild the env with |
Okay, doing I also see that some of them have a list of rules, e.g. flake8-comprehensions, and the violations only get picked when we specify a certain rule via For e.g. if we have something like this for that plugin x = tuple()Running So it might be the case that most of the plugins need to have the rules to apply explicitly declared? |
|
As an example, adding I can take this exercise as an example, and start adding We can also use --enable-extensions=C4 for the same, which will not cause us to lose the default --select values. |
|
Hi @asottile Sorry for pinging you here, but I wanted to get your input as well on how to proceed with this task, especially in light of the last 4-5 comments in this issue. |
|
the reason plugins are not being reported is from this line (which can just be removed, that's the default setting) I'd also suggest changing the line after it to |
Thanks for that pointer. I got rid of that line, and the pre-commit check of flake-8 outputted 9910 lines, with all plugins listed in #5537 (comment) added to the Also if I am planning to add these plugins one-by one, is there any specific ones which you suggest I should start with (given you have had some experience with them in the past). Also what steps should ideally be followed for adding these plugins? Are there a list of best-practices somewhere. From what I can think of right now is to add the plugin in (I can also add more options to |
|
fwiw (this isn't my project, but) -- I would suggest starting with the plugins which are the least controversial and/or with the lowest number of violations and add them one by one (probably deciding on a per-plugin basis which ones to keep or not) it looks to me like almost all of the violations in your report above come from flake8-quotes and flake8-docstrings (and I'm fairly certain you don't want to sign up for adding docstrings in a few thousand places!) |
|
Thanks for all your inputs. I think starting from the least controversial and with lowest violations make sense, since it should have a smaller footprint of changes, and can serve as a baseline on adding future plugins in terms of process being followed. Given these inputs, I would probably start with either (The footprint of Hi Pradyun, Paul, Let me know if those two plugins are a good starting point for this exercise, and they are not too intrusive in the ongoing PRs (requiring a major rebase effort after a PR merge) |
|
you'll want to |
Are you talking about https://docs.openstack.org/bandit/latest/plugins/b101_assert_used.html in the above statement? (I couldn't find a |
|
That would solve GH-7009, which I am still interested in taken care of. |
From what I could gather from the repo, flake8-comprehensions would look at the existing list/set/dict comprehensions in pip's codebase, flag unnecessary constructs used in them and suggest simpler equivalent of those according to the rules defined in the readme. Some examples of existing code in pip, and pip/tests/functional/test_search.py Line 200 in a4933e4 complains with log_messages = sorted(r.getMessage() for r in caplog.records)pip/src/pip/_internal/utils/wheel.py Line 124 in a4933e4 complains with subdirs = list({p.split("/")[0] for p in source.namelist()})pip/src/pip/_internal/req/req_uninstall.py Lines 125 to 128 in a4933e4 complains with and can be rewritten as case_map = {os.path.normcase(p): p for p in paths}
remaining = set(case_map)
unchecked = sorted({os.path.split(p)[0]
for p in case_map.values()}, key=len)These are just some of the refactorings suggested by the plugin.
Yes, it currently flags some existing code in pip. The full set of violations for the plugin are as follows: |
I will look into adding this as well, but should I add this before |
That's great to hear and adding a plugin will allow us to move forward in some existing issues. Although on second look, that issue is for regular strings, whereas |
|
There are only (non-progressbar, non-logging) pip/src/pip/_internal/operations/build/wheel.py Lines 29 to 30 in a4933e4 The rest will be pretty much mindless OCD-favored reformatting which I adore 😄 |
Adding to that, here are the list of current violations caught by |
|
I think I understood
Furthermore, what's the benefit of letting |
|
The reason for that (I don't necessarily agree in all cases since it's been shown that eagerly formatting f-strings is actually faster in most cases) is that you incur unnecessary overhead for logging statements that may be entirely discarded (performance). Avoiding the format and letting the logging machinery do it only when necessary saves a few cycles. # good
logging.debug('foo %s %s', thing1, thing2)
# bad
logging.debug(f'foo {thing1} {thing2}') |
|
Thanks, @asottile. So do you think it'd be reasonable to go with |
|
I think But I agree that if we use |
|
I've just filed GH-8423 for some of these additional good practices. I'm not sure if we want to opt-in any other before wrapping |
|
Hi @pfmoore , @pradyunsg Given my understanding of |
|
I wonder if we want to add flake8-builtins to the pending list of plugins to try. |
That's a good idea too. I will probably give it a shot in a PR once I get some clarity on whether we want to use |
|
@deveshks @McSinyx how about adding some plugins from the list here https://wemake-python-stylegui.de/en/latest/pages/usage/violations/index.html? Plus maybe https://github.com/m-burst/flake8-pytest-style |
|
That's a good list for reference. I also see that most of them are listed in the comments here as well. But we would need to get agreement on which of these are most useful from the maintainers, as well as in what order should we be adding them (probably from least invasive to the most). |
|
Right. Also, FTR wemake-python-styleguide is developing a baseline support that would be very helpful in terms of invasiveness. Here's also a few words on supporting "legacy" codebases that you may find interesting: https://wemake-python-stylegui.de/en/latest/pages/usage/integrations/legacy.html. |
Thanks for that. I will keep this in mind when integrating more plugins into |
|
Given that |
|
If someone wants to figure out what the next steps here are, that'd be great! |
|
HI @pradyunsg , As per my earlier comment, I was thinking of integrating |
|
We don't need this issue around anymore as flake8 got replaced by ruff here: #12073. |
|
Does Ruff cover all the plugins discussed? If not, it could be reasonable to run flake8 against a small subset of rules. |
|
As far as I understood the Ruff PR, a big point was to take advantage of Ruff's speed and the fact it's just one tool. Bringing back just a few flake8 plugins would defeat the purpose of using Ruff. |
|
ruff has many of the rules discussed and they are often adding more: https://docs.astral.sh/ruff/rules/#flake8-2020-ytt I'm sure someone sufficiently motivated could add more rules discussed here to pip's configuration that was discussed here. |
|
I just went through and checked...
I think we'll be OK. |
In general, there's some flake8 plugins that might be useful to us. In case of some of these plugins, it means adopting a newer code style so we should probably start with that discussion.
flake8-commasPyPI: enforce commas after the last item of multiline literals/function calls.flake8-mutablePyPI: enforce not using mutable values as defaultsflake8-logging-formatPyPI: check that strings aren't formatted while passing to logging (should depend on logging doing the interpolation)flake8-pep3101PyPI: enforces the use of.formatinstead of%in the codebaseflake8-gramexPyPI: checks for magic numbers and requiredencodingfor calls to file opening functions + 2/3 more (but we'll ignore those)I think some of these might be good additions. The only thing is that adopting them would mean going through the entire codebase to make things consistent according to them wherever there's a violation (which is kind of the point of doing this).
Thoughts @pypa/pip-committers?
The text was updated successfully, but these errors were encountered: