Closed Bug 1270506 Opened 8 years ago Closed 8 years ago

Add flake8 integration for mozlint

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(2 files)

We should add an integration for the flake8 linter to mozlint framework, which is landing soon.

By default, we'll need to only lint directories that are explicitly included. For this we can either figure out a way to do it via the flake8 configuration, or else we can use mozlint's include/exclude directives.
The current algorithm for filtering down tests is too naive. For example, given the following
directory structured:

parent
  - foo
    - bar
    - baz

And the following include/exclude directives:
include = ['foo']
exclude = ['foo/bar']

Then running ./mach lint parent and ./mach lint foo/baz should both lint all files in baz but
no files in bar. This provides a nice way to include/exclude directories, while allowing the
underlying linters to find appropriate files to lint *within* those directories.

tl;dr - Straight file paths (no globs) will be passed straight to the underlying linter as is.
While paths with globs will first be resolved to a list of matching file paths.

Review commit: https://reviewboard.mozilla.org/r/51125/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51125/
Attachment #8749712 - Flags: review?(smacleod)
Attachment #8749713 - Flags: review?(smacleod)
For now, only the following two directories will be linted:
python/mozlint
tools/lint

New directories can be added by adding them to the 'include'
directive in tools/lint/flake8.lint. They all default to the
configuration specified in topsrcdir/.flake8. Subdirectories
can override this configuration by creating their own .flake8
file.

Review commit: https://reviewboard.mozilla.org/r/51127/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51127/
Comment on attachment 8749712 [details]
MozReview Request: Bug 1270506 - [mozlint] Refactor the include/exclude path filtering algorithm, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51125/diff/1-2/
Comment on attachment 8749713 [details]
MozReview Request: Bug 1270506 - [mozlint] Add python flake8 linter, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51127/diff/1-2/
Attachment #8749712 - Flags: review?(smacleod) → review+
Comment on attachment 8749712 [details]
MozReview Request: Bug 1270506 - [mozlint] Refactor the include/exclude path filtering algorithm, r?smacleod

https://reviewboard.mozilla.org/r/51125/#review49766

::: python/mozlint/mozlint/pathutils.py:13
(Diff revision 2)
> +
> +from mozpack import path as mozpath
> +from mozpack.files import FileFinder
> +
> +
> +class fpath(object):

Please use "CapWords" capitalization for class names.

::: python/mozlint/mozlint/pathutils.py:46
(Diff revision 2)
> +        return fpath(os.path.join(self, *args))
> +
> +    def match(self, patterns):
> +        return any(mozpath.match(self.path, pattern.path) for pattern in patterns)
> +
> +    def __cmp__(self, other):

I find the use of `__cmp__` and the `<`, `>` operators for determining subdirectory and ancestry a little harder to follow than it ought to be. I think things would be much clearer if you just implemented these as separate methods `is_subdirectory`, etc.

::: python/mozlint/mozlint/pathutils.py:52
(Diff revision 2)
> +        if isinstance(other, fpath):
> +            other = other.path
> +        a = os.path.abspath(self.path)
> +        b = os.path.normpath(os.path.abspath(other))
> +
> +        if b.startswith(a):

If the paths are equal, this will return `True`, meaning you'll return `-1` in the equal case, and never reach `return 0`.
Comment on attachment 8749713 [details]
MozReview Request: Bug 1270506 - [mozlint] Add python flake8 linter, r?smacleod

https://reviewboard.mozilla.org/r/51127/#review49774

::: python/mozlint/test/linters/regex.lint:7
(Diff revision 2)
>  # vim: set filetype=python:
>  
>  LINTER = {
>      'name': "RegexLinter",
> -    'description': "Make sure the string 'foobar' never appears in a js variable files because it is bad.",
> +    'description': "Make sure the string 'foobar' never appears "
> +                   "in a js variable files because it is bad.",

might as well fix "files" vs "file"

::: tools/lint/flake8.lint:53
(Diff revision 2)
> +    results = []
> +    for line in output.splitlines():
> +        try:
> +            res = json.loads(line)
> +        except ValueError:
> +            continue
> +
> +        if 'code' in res and res['code'].startswith('W'):
> +            res['level'] = 'warning'
> +        results.append(result.from_linter(LINTER, **res))
> +
> +    return results

It'd be nice to use the offset information mozreviewbots already came up with for flake8 to fill in the optional result fields. You could just steal the dictionary from https://dxr.mozilla.org/hgcustom_version-control-tools/rev/dc0cdff5206baec1e2d16f62604c68f8db65e1af/pylib/mozreviewbots/pylintbot/__main__.py#18-36

I don't think this needs to be a blocker, but if you don't implement it maybe just a comment pointing at it would be good for later use.
Attachment #8749713 - Flags: review?(smacleod) → review+
Comment on attachment 8749712 [details]
MozReview Request: Bug 1270506 - [mozlint] Refactor the include/exclude path filtering algorithm, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51125/diff/2-3/
Comment on attachment 8749713 [details]
MozReview Request: Bug 1270506 - [mozlint] Add python flake8 linter, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51127/diff/2-3/
https://reviewboard.mozilla.org/r/51125/#review49766

> I find the use of `__cmp__` and the `<`, `>` operators for determining subdirectory and ancestry a little harder to follow than it ought to be. I think things would be much clearer if you just implemented these as separate methods `is_subdirectory`, etc.

is_subdir wasn't totally clear which direction it was checking (is self a subdir of other? Or other a subdir of self..). I called it 'contains' which is still a little confusing, but I can't think of a better name.

> If the paths are equal, this will return `True`, meaning you'll return `-1` in the equal case, and never reach `return 0`.

This was actually on purpose, but you're right in your other comment that it wasn't at all easy to understand. I added a docstring to the new contains method to explain this a bit better.
Blocks: 1273634
https://hg.mozilla.org/mozilla-central/rev/a2d21e14d40c
https://hg.mozilla.org/mozilla-central/rev/971d076cfe29
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.