Closed Bug 1395126 Opened 2 years ago Closed 2 years ago

Support cascading configuration in flake8

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

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 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...
Attachment #8902650 - Flags: review?(bob) → review+
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 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 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 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.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0145883aea3a
Support cascading configuration for flake8, r=bc
https://hg.mozilla.org/mozilla-central/rev/0145883aea3a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.