[flake8] Switch from whitelist to blacklist in flake8 linter
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement, P2)
Tracking
(firefox67 fixed)
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: ahal, Assigned: ahal)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
Assignee | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Updated•7 years ago
|
Assignee | ||
Comment 5•7 years ago
|
||
Updated•7 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 6•6 years ago
|
||
This allows us to only skip the "unused import" config in these files rather
than the entire thing. This also removes the only two uses of "**" in the
exclusion rules which made things a bit simpler for me later on in the series.
Assignee | ||
Comment 7•6 years ago
|
||
This will be re-used by the flake8 linter, so moving it into mozlint for
re-useability.
Depends on D20493
Assignee | ||
Comment 8•6 years ago
|
||
This is required for a future commit which will monkeypatch flake8's
configuration to fit our needs.
But it has a couple nice benefits anyway:
1. Less process overhead.
2. Less complexity around handling SIGINT.
3. Less complexity in the code.
Depends on D20494
Assignee | ||
Comment 9•6 years ago
|
||
The motivations for this are:
-
Apply global excludes. This merges the exclusion rules defined in
tools/lint/mach_commands.py with the ones in .flake8. -
Improve performance. Switching to a blacklist will result in a much longer
runtime for linting the entire tree and flake8 handles exclusions incredibly
slowly. Without this patch (and the blacklist change applied), I gave up
waiting after 30 minutes. With this patch, it takes 30 seconds.
Depends on D20495
Assignee | ||
Comment 10•6 years ago
|
||
This ensures that the default for new python files is to be linted by flake8.
Depends on D20496
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 11•6 years ago
|
||
I figure it's worth a comment explaining some of the performance implications this series has:
D20495 actually slighty harms performance (as egao found in the review), though not by a significant amount. This was surprising to me as I thought removing the extra process overhead would improve it. This perf regression was absolutely dwarfed by further perf regressions though, so I didn't bother looking into it.
D20497 causes a big perf regression. This is expected because flake8 is now searching the entire tree instead of a tiny fraction of subdirs. However, the performance of native flake8 was so bad here that I had to ctrl-c after 30min of waiting (when linting the entire tree). Maybe there was a bug causing a deadlock or something.
D20496 was created to use our own logic to find files (via FileFinder). This brought the runtime down from 30+ min to ~35 seconds on my machine. It might be worth investigating upstreaming our logic to replace the built-in flake8 mechanism, but doing this wouldn't be trivial.
To summarize, when linting the entire tree via ./mach lint -l flake8
:
No patch: ~14s
D20495: ~15s
D20495+D20497: 30+ min
Entire series: ~35s
So in my testing linting the entire tree takes a little over twice as long. This seems like an acceptable tradeoff for using a blacklist. Especially since the performance of linting specific directories should not have nearly as big of an effect. For reference, linting the entire tree with eslint takes over 4min.
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ef9a57429d59
https://hg.mozilla.org/mozilla-central/rev/ba172b704def
https://hg.mozilla.org/mozilla-central/rev/cd46126bc592
https://hg.mozilla.org/mozilla-central/rev/41fdb372e22c
https://hg.mozilla.org/mozilla-central/rev/4624c850f711
Updated•2 years ago
|
Description
•