1395126 - Support cascading configuration in flake8

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andrew Halberstadt [:ahal]]

This will make the situation in bug 1277851 much better, though it still won't make it perfect, so I'm leaving the other one open.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1395126 - Support cascading configuration for flake8,

This allows .flake8 files to override one another, and fixes a pretty bad known
bug with our flake8 implementation. For example, say we have a .flake8 file at:
/foo/.flake8

Before this patch, if we ran |mach lint foo/bar|, the configuration defined in
that .flake8 file wouldn't get picked up. It would only work if running the
specific directory that contains it, e.g |mach lint foo|.

This change additionally allows multiple .flake8 files to be used. So if
there's one defined at both:
/.flake8
/foo/.flake8

Then running |mach lint foo/bar| will first apply the root .flake8, then the
one under /foo (overriding earlier configuration).

This bug still doesn't make flake8 configuration perfect though. Any directory
containing a .flake8 file still needs to be explicitly listed in the "include"
section of /tools/lint/flake8.yml. Otherwise in the example above, if running
|mach lint /|, it wouldn't be able to find /foo/.flake8. This is a hard problem
and is likely best solved by fixing flake8's upstream configuration handling.

Unfortunately this means we still can't switch from a whitelist to a blacklist.

Review commit: https://reviewboard.mozilla.org/r/174316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174316/

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 8902650 [details]
Bug 1395126 - Support cascading configuration for flake8,

https://reviewboard.mozilla.org/r/174316/#review179582

Seems to be ok. I'm a bit out of the loop regarding the files/paths but it appears to be an existing condition for good or ill. r+ with a minimum of addressing the inconsistent spacing around =.

::: python/mozlint/mozlint/pathutils.py:191
(Diff revision 1)
> +            break
> +
> +
> +def get_relevant_configs(name, path, root):
> +    """Returns a list of configuration files that exist in `path`'s ancestors,
> +    sorted from closest->furthest.

This really has nothing to do with configuration files. Instead it searchs the paths from root down through path looking for instances of whatever file was specified in |name|. 

I see this isn't new so it is not a big deal. Address it how you like.

Should we also assert or test that path is a directory?

::: testing/marionette/client/.flake8:2
(Diff revision 1)
>  [flake8]
> -max-line-length = 99
> +exclude=

inconsistent spacing around =

::: testing/marionette/puppeteer/.flake8:2
(Diff revision 1)
>  [flake8]
> -max-line-length = 99
> +exclude=

ditto spacing.

::: tools/lint/flake8_/__init__.py:162
(Diff revision 1)
>      # Run any paths with a .flake8 file in the directory separately so
>      # it gets picked up. This means only .flake8 files that live in
>      # directories that are explicitly included will be considered.
>      # See bug 1277851
> -    no_config = []
> +    paths_by_config = defaultdict(list)
>      for f in files:

The name |files| is confusing to me since it is really a list of paths. But looking at http://searchfox.org/mozilla-central/source/python/mozlint/mozlint/types.py#26 it appears it could really be either...

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8902650 [details]
Bug 1395126 - Support cascading configuration for flake8,

https://reviewboard.mozilla.org/r/174316/#review179582

> This really has nothing to do with configuration files. Instead it searchs the paths from root down through path looking for instances of whatever file was specified in |name|. 
> 
> I see this isn't new so it is not a big deal. Address it how you like.
> 
> Should we also assert or test that path is a directory?

Sure, I can rename it, how about `get_ancestors_by_name`?

I don't think asserting that path is a directory is needed. It'll get converted to a directory on the second iteration of `ancestors()` anyway.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8902650 [details]
Bug 1395126 - Support cascading configuration for flake8,

https://reviewboard.mozilla.org/r/174316/#review179582

No you're right, files is confusing and it should be called `paths`.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8902650 [details]
Bug 1395126 - Support cascading configuration for flake8,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174316/diff/1-2/

Comment 6

[Bob Clary [:bc] (inactive)]

Comment on attachment 8902650 [details]
Bug 1395126 - Support cascading configuration for flake8,

https://reviewboard.mozilla.org/r/174316/#review179632

Looks fine. Just a question about some trailing commas in this last version.

::: .flake8:6
(Diff revisions 1 - 2)
>  [flake8]
>  # See http://pep8.readthedocs.io/en/latest/intro.html#configuration
>  ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402
>  max-line-length = 99
> -exclude=
> -    testing/mochitest/pywebsocket
> +exclude =
> +    testing/mochitest/pywebsocket,

Why the trailing comma?

::: testing/marionette/harness/.flake8:7
(Diff revisions 1 - 2)
>  exclude =
>      __init__.py,
>      disti/*,
>      build/*,
>      marionette_harness/runner/mixins/*,
> -    marionette_harness/tests/*
> +    marionette_harness/tests/*,

comma?

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8902650 [details]
Bug 1395126 - Support cascading configuration for flake8,

https://reviewboard.mozilla.org/r/174316/#review179632

> Why the trailing comma?

Because even though they are newline delimited, flake8 will still apparently break if there is no comma separating them. I figured it would be safer to add trailing commas everywhere so people don't accidentally forget them when adding new paths and cause silent failures. It'll also make the diff cleaner for future modifications.

Also I already autolanded this, so too late to fix anyway.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0145883aea3a
Support cascading configuration for flake8, r=bc

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0145883aea3a