[mozlint] Better flake8 configuration support

RESOLVED WONTFIX

Status

defect
P5
normal
RESOLVED WONTFIX
3 years ago
7 months ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

It turns out that just adding new .flake8 files to subdirectories doesn't work out as nicely as I said it did. I should have caught this earlier. Here's some extra context.

Flake8 currently defers all its configuration handling to the pep8 module, and pep8 handles configuration really stupidly. It will use the first configuration file that is common to *all specified paths*.

So if you run:
flake8 testing/marionette/client

Then it will use 'testing/marionette/client/.flake8'. But if you specify a second path, e.g:
flake8 testing/marionette/client tools/lint

Then it will search for a single common config file (which in our case is the global .flake8 file). This is incredibly dumb, I should have noticed this earlier. There is some bad news and good news.

The bad news is that pep8 as a project is stalled out, and the likelihood of them either implementing multiple configs or accepting multiple configs is pretty low. The good news is that flake8 is planning a version 3 which aims to stop using pep8 for things like configuration, and multiple config files is something they're at least interested in.

So until flake8 3 comes out with better config file support, we have two options:
1) Invoke a new flake8 subprocess for each .flake8 file we find, this will involve walking the tree of each file path we pass in.
2) Only use the global config (no subdirectory .flake8 files)

I'll take a quick look into 1) to see how feasible it is.
This is a crude workaround to get subdirectory .flake8 files working. Hopefully
this is temporary until flake8 3.0 is released with support for multiple config
files.

Note that .flake8 files that live outside of a directory that is explicitly
listed in the 'include' directive, will not be considered.

Review commit: https://reviewboard.mozilla.org/r/57590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57590/
Attachment #8759686 - Flags: review?(dburns)
Attachment #8759686 - Flags: review?(dburns) → review?(mjzffr)
Comment on attachment 8759686 [details]
Bug 1277851 - [mozlint] Run flake8 directories that contain a .flake8 file separately,

https://reviewboard.mozilla.org/r/57590/#review54392
Attachment #8759686 - Flags: review?(mjzffr) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a825772c31
[mozlint] Run flake8 directories that contain a .flake8 file separately, r=maja_zf
I consider this more of a quick hack than long term solution, so marking [leave-open] to tackle this properly in the future.
Whiteboard: [leave-open]
Component: General → Lint
Depends on: 1373294
Blocks: 1367092
Flake8 no longer relies on pep8's configuration, but this is still a problem. For posterity, I filed an issue about this a year ago:
https://gitlab.com/pycqa/flake8/issues/143

It looks unlikely that feature request will be accepted though. Maybe a plugin could implement something like that.
Priority: -- → P5
The yamllint linter has a similar problem to this, and a fix just landed to make it a bit better in bug 1392795. What it does is preprocess the paths by searching the ancestors of each path passed in for .yamllint configs. It then groups all paths that share the same configuration file and runs them together.

We should do this for flake8 as well. It still isn't ideal, but would be a big improvement on the status quo.
Depends on: 1395126
See Also: → 1432331
Product: Testing → Firefox Build System
Keywords: leave-open
Whiteboard: [leave-open]

Instead of this, I've decided to go the easier route of removing subdirectory .flake8 files. This is now possible because flake8 3.7 introduced a new feature that allows us to ignore specific errors in subdirectories in the root config file.

WONTFIXing in favour of bug 1515746.

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → WONTFIX
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.