[PSA] Upcoming changes to flake8 linter

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PSA] Upcoming changes to flake8 linter

Andrew Halberstadt-2
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:

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)

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
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)

Currently we register 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)

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

_______________________________________________
dev-builds mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-builds
Reply | Threaded
Open this post in threaded view
|

Re: [PSA] Upcoming changes to flake8 linter

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
_______________________________________________
dev-builds mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-builds
Reply | Threaded
Open this post in threaded view
|

Re: [PSA] Upcoming changes to flake8 linter

Andrew Halberstadt-2
On Fri, Feb 1, 2019 at 4:39 PM Mike Hommey <[hidden email]> 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.

_______________________________________________
dev-builds mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-builds
Reply | Threaded
Open this post in threaded view
|

Re: [PSA] Upcoming changes to flake8 linter

Andrew Halberstadt-2
In reply to this post by Andrew Halberstadt-2
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 <[hidden email]> 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

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

For their meanings see:

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)

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
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)

Currently we register 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)

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

_______________________________________________
dev-builds mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-builds
Reply | Threaded
Open this post in threaded view
|

Re: [PSA] Upcoming changes to flake8 linter

Andrew Halberstadt-2
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 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.

On Fri, Feb 8, 2019 at 9:50 AM Andrew Halberstadt <[hidden email]> wrote:
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 <[hidden email]> 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

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

For their meanings see:

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)

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
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)

Currently we register 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)

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

_______________________________________________
dev-builds mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-builds