Closed
Bug 1270506
Opened 9 years ago
Closed 9 years ago
Add flake8 integration for mozlint
Categories
(Testing :: General, defect)
Testing
General
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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/
Assignee | ||
Comment 3•9 years ago
|
||
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/
Assignee | ||
Comment 4•9 years ago
|
||
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/
Updated•9 years ago
|
Attachment #8749712 -
Flags: review?(smacleod) → review+
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
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/
Assignee | ||
Comment 8•9 years ago
|
||
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/
Assignee | ||
Comment 9•9 years ago
|
||
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.
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2d21e14d40c
https://hg.mozilla.org/mozilla-central/rev/971d076cfe29
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•