[PSA] Upcoming changes to flake8 linter

[Andrew Halberstadt]

Hi everyone,

Just wanted to let you know of a few upcoming changes to our
flake8 linter that I'm planning:

*1. Upgrading flake8, pycodestyle and pyflakes*

It's been awhile since we've bumped these tools. The upgrades
will be:
flake8: 3.5.0 -> 3.7.4
pycodestyle: 2.3.1 -> 2.5.0
pyflakes: 1.5.0 -> 2.1.0

These updates add the following new error codes:
F632, F633, F723, F811, E117, W504, W605, W606

For their meanings see:
http://flake8.pycqa.org/en/latest/user/error-codes.html
http://pep8.readthedocs.io/en/latest/intro.html#configuration

I plan to disable these new rules at first. They'll be separated
via comment from the other intentionally disabled rules in the
config.



*2. Deprecating subdirectory .flake8 files (bug 1515746
<https://bugzilla.mozilla.org/show_bug.cgi?id=1515746>)*

There will be a single canonical .flake8 file in the root of the
repository. The main motivation for this is an implementation
detail where this feature is preventing us from switching to a
blacklist. But this will also simplify our configs and code, keep
editor integrations in-line with |mach lint|, and improve perf.

This is now possible because of flake8 3.7's new per-file-ignores
<http://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-per-file-ignores>
feature. This can be implemented without any change to how our
current files are linted, i.e no source code will be modified as part
of this change.



*3. Switching to a blacklist (bug 1367092
<https://bugzilla.mozilla.org/show_bug.cgi?id=1367092>)*

Currently we register
<https://searchfox.org/mozilla-central/source/tools/lint/flake8.yml#4>
which files and directories should be linted.
This is not ideal because any new code added outside of those
directories will not be linted by default. Instead, we will define
which paths *shouldn't* be linted. This will be possible thanks to
the deprecation of subdirectory configs (it's a long story).

As part of this process I intend to only exclude what is absolutely
necessary (within reason). This means some .py files which had
no issues but weren't being linted will start being linted. It also
means new modules will automatically be linted.



*4. Enabling flake8-isort (bug 1492495
<https://bugzilla.mozilla.org/show_bug.cgi?id=1492495>)*

Isort is a tool to format python imports, and flake8-isort is a shim
to use it as a linter. I'm proposing flake8-isort over the more
popular flake8-import-order because we'll get automatic fixing for
free. Manually re-ordering imports is not something I think anyone
should be subjected to.

Isort is configurable, but the default is pretty close to what we
unofficially tend to use anyway, namely:

# stdlib imports
import os
import sys
from collections import OrderedDict, defaultdict

# external dependency imports
import requests
from mozprocess import ProcessHandler

# internal imports
import mymodule
from . import name

I think I'd like to enable this at the 'warning' level to start (so it
doesn't cause backouts or show up without --warnings). This will
give people a feel for the new syntax and allow us to start using
--fix to re-order imports before making it a hard requirement.

This item item is potentially controversial, so let me know if you
have any objections to enabling this.


*5. Mass enabling of rules*

With flake8 + dependencies upgraded and flake8-isort installed,
we'll have many new rules (which were all turned off by default) to
triage and enable. Ideally this would all happen in a single massive
commit similar to our format efforts in other languages. We would
use the magic commit string to keep it out of git/hg blame.


Not everything will happen all at once, the items at the top of the
list will land before the items near the bottom. Let me know if you
have any questions or objections!

Cheers,
Andrew

[James Graham]

On 01/02/2019 16:08, Andrew Halberstadt wrote:
> Hi everyone,
>
> Just wanted to let you know of a few upcoming changes to our
> flake8 linter that I'm planning:
>
> *1. Upgrading flake8, pycodestyle and pyflakes*
>
> It's been awhile since we've bumped these tools. The upgrades
> will be:
> flake8: 3.5.0 -> 3.7.4
> pycodestyle: 2.3.1 -> 2.5.0
> pyflakes: 1.5.0 -> 2.1.0

I would suggest waiting a few days to see if the 3.7.x series of flake8
has reached a stable state; there have been 5 releases in the last 3
days with the latter 4 all fixing regressions in 3.7.

> *4. Enabling flake8-isort (bug 1492495
> <https://bugzilla.mozilla.org/show_bug.cgi?id=1492495>)*
>
> Isort is a tool to format python imports, and flake8-isort is a shim
> to use it as a linter. I'm proposing flake8-isort over the more
> popular flake8-import-order because we'll get automatic fixing for
> free. Manually re-ordering imports is not something I think anyone
> should be subjected to.
>
> Isort is configurable, but the default is pretty close to what we
> unofficially tend to use anyway, namely:
>
> # stdlib imports
> import os
> import sys
> from collections import OrderedDict, defaultdict
>
> # external dependency imports
> import requests
> from mozprocess import ProcessHandler
>
> # internal imports
> import mymodule
> from . import name

treeherder used this and one problem I had at least sometimes was that
the tools would make a different decision about what was an external
dependency vs a static import depending on the environment, so (iirc) I
would sometimes get a passing lint locally with some deps missing and a
failure on CI with all the deps actually installed.

Also it at least used to require a rather awkward style like

from foo import (bar,
baz)

[Mike Hommey]

On Fri, Feb 01, 2019 at 11:08:26AM -0500, Andrew Halberstadt wrote:
> *4. Enabling flake8-isort (bug 1492495
> <https://bugzilla.mozilla.org/show_bug.cgi?id=1492495>)*
>
> Isort is a tool to format python imports, and flake8-isort is a shim
> to use it as a linter. I'm proposing flake8-isort over the more
> popular flake8-import-order because we'll get automatic fixing for
> free. Manually re-ordering imports is not something I think anyone
> should be subjected to.
>
> Isort is configurable, but the default is pretty close to what we
> unofficially tend to use anyway, namely:
>
> # stdlib imports
> import os
> import sys
> from collections import OrderedDict, defaultdict
>
> # external dependency imports
> import requests
> from mozprocess import ProcessHandler
>
> # internal imports
> import mymodule
> from . import name
>

> I think I'd like to enable this at the 'warning' level to start (so it
> doesn't cause backouts or show up without --warnings). This will
> give people a feel for the new syntax and allow us to start using
> --fix to re-order imports before making it a hard requirement.
>
> This item item is potentially controversial, so let me know if you
> have any objections to enabling this.

Considering modules can execute code when they are being imported (and
some do), reordering should be done with extra care.

Relatedly, how is the following handled?

```
import sys
sys.path.append('some path')
import more, stuff.
```

Mike

[Andrew Halberstadt]

On Fri, Feb 1, 2019 at 4:39 PM Mike Hommey <m...@glandium.org> wrote:

> Considering modules can execute code when they are being imported (and
> some do), reordering should be done with extra care.
>

Yes, we'll need to be careful. Though I'd make the argument that if a
module depends on import order to function correctly, that's something
we should fix anyway :).

Relatedly, how is the following handled?
>
> ```
> import sys
> sys.path.append('some path')
> import more, stuff.
> ```
>

This case is not handled. Maybe there's a config setting we can use to
turn this off, but otherwise we can blacklist files that depend on this
pattern using flake8's `per-file-ignores`.

Probably should have made it a bit more clear that the isort change is
not imminent or anything. Items 1-3 should land in the next week or two
and 4-5 are on a bit longer of a time frame. There will be time to make
sure it's done right.

[Andrew Halberstadt]

Quick update:
- Items *1* (flake8 upgrade) and *2* (removing sub .flake8 files) are now
landed.
- Item *3* (blacklist) should land in the next week or two.
- Items *4-5* have no timeline yet.

On Fri, Feb 1, 2019 at 11:08 AM Andrew Halberstadt <ah...@mozilla.com> wrote:

> Hi everyone,
>
> Just wanted to let you know of a few upcoming changes to our
> flake8 linter that I'm planning:
>
> *1. Upgrading flake8, pycodestyle and pyflakes*
>
> It's been awhile since we've bumped these tools. The upgrades
> will be:
> flake8: 3.5.0 -> 3.7.4
> pycodestyle: 2.3.1 -> 2.5.0
> pyflakes: 1.5.0 -> 2.1.0
>

[Andrew Halberstadt]

Another update:
- Items *1-3* are now landed.
- Items *4-5* still have no timeline. Please ping me if you're interested
in pushing these forward.

This means that our flake8 linter finally uses a blacklist! New python
files you add to the tree should automatically get linted (provided they
don't live under one the exclusions in the .flake8
<https://searchfox.org/mozilla-central/source/.flake8> file). As a result
of this change, expect running flake8 on the entire tree to take roughly
twice as long. Linting subdirectories shouldn't have too much of a perf hit
though.

Let me know if you see anything else weird going on.