Bug 1288425 - Make sure we skip invalid extensions when linting with --rev or --workdir, r?smacleod draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 09 Aug 2016 16:24:04 -0400
changeset 403977 dae28fe41c6e019dc98388b26619378ae941cfc9
parent 403970 194fe275b4e60ded2af6b25173eec421f0dba8ad
child 529046 1e72482a8643f8ccea7eec61148d922e5a28aa37
push id27060
push userahalberstadt@mozilla.com
push dateMon, 22 Aug 2016 14:09:27 +0000
reviewerssmacleod
bugs1288425
milestone51.0a1
Bug 1288425 - Make sure we skip invalid extensions when linting with --rev or --workdir, r?smacleod Some linters, such as flake8, will lint invalid file extensions if you explicitly pass them in. E.g, |flake8 foobar.js| will result in flake8 attempting to lint a JS file. This is a problem because passing in files explicitly is exactly what the --rev/--workdir options do. If a developer modifies a JS file then runs |mach lint -l flake8 -w|, that JS file will get linted. To prevent this, mozlint needs to handle file extensions instead of relying on the underlying linter to do it. This patch adds an "extensions" config option to the LINTER dict, and will filter these files out as part of the 'filterpaths' steps. MozReview-Commit-ID: KYhC6SEySC3
python/mozlint/mozlint/pathutils.py
python/mozlint/test/files/foobar.py
python/mozlint/test/linters/external.lint
python/mozlint/test/test_roller.py
python/mozlint/test/test_types.py
tools/lint/docs/create.rst
tools/lint/flake8.lint
--- a/python/mozlint/mozlint/pathutils.py
+++ b/python/mozlint/mozlint/pathutils.py
@@ -21,16 +21,20 @@ class FilterPath(object):
     def finder(self):
         if self._finder:
             return self._finder
         self._finder = FileFinder(
             self.path, find_executables=False, ignore=self.exclude)
         return self._finder
 
     @property
+    def ext(self):
+        return os.path.splitext(self.path)[1]
+
+    @property
     def exists(self):
         return os.path.exists(self.path)
 
     @property
     def isfile(self):
         return os.path.isfile(self.path)
 
     @property
@@ -88,19 +92,27 @@ def filterpaths(paths, linter, **lintarg
     # Paths with and without globs will be handled separately,
     # pull them apart now.
     includepaths = [p for p in include if p.exists]
     excludepaths = [p for p in exclude if p.exists]
 
     includeglobs = [p for p in include if not p.exists]
     excludeglobs = [p for p in exclude if not p.exists]
 
+    extensions = linter.get('extensions')
     keep = set()
     discard = set()
     for path in map(FilterPath, paths):
+        # Exclude bad file extensions
+        if extensions and path.isfile and path.ext not in extensions:
+            continue
+
+        if path.match(excludeglobs):
+            continue
+
         # First handle include/exclude directives
         # that exist (i.e don't have globs)
         for inc in includepaths:
             # Only excludes that are subdirectories of the include
             # path matter.
             excs = [e for e in excludepaths if inc.contains(e)]
 
             if path.contains(inc):
@@ -124,22 +136,21 @@ def filterpaths(paths, linter, **lintarg
         # Next handle include/exclude directives that
         # contain globs.
         if path.isfile:
             # If the specified path is a file it must be both
             # matched by an include directive and not matched
             # by an exclude directive.
             if not path.match(includeglobs):
                 continue
-            elif path.match(excludeglobs):
-                continue
+
             keep.add(path)
         elif path.isdir:
             # If the specified path is a directory, use a
             # FileFinder to resolve all relevant globs.
-            path.exclude = excludeglobs
+            path.exclude = [e.path for e in excludeglobs]
             for pattern in includeglobs:
                 for p, f in path.finder.find(pattern.path):
                     keep.add(path.join(p))
 
     # Only pass paths we couldn't exclude here to the underlying linter
     lintargs['exclude'] = [f.path for f in discard]
     return [f.path for f in keep]
new file mode 100644
--- /dev/null
+++ b/python/mozlint/test/files/foobar.py
@@ -0,0 +1,2 @@
+# Oh no.. we called this variable foobar, bad!
+foobar = "a string"
--- a/python/mozlint/test/linters/external.lint
+++ b/python/mozlint/test/linters/external.lint
@@ -17,14 +17,14 @@ def lint(files, **lintargs):
                         LINTER, path=path, lineno=i+1, column=1, rule="no-foobar"))
     return results
 
 
 LINTER = {
     'name': "ExternalLinter",
     'description': "It's bad to have the string foobar in js files.",
     'include': [
-        '**/*.js',
-        '**/*.jsm',
+        'python/mozlint/test/files',
     ],
     'type': 'external',
+    'extensions': ['.js', '.jsm'],
     'payload': lint,
 }
--- a/python/mozlint/test/test_roller.py
+++ b/python/mozlint/test/test_roller.py
@@ -15,18 +15,18 @@ from mozlint.errors import LintersNotCon
 here = os.path.abspath(os.path.dirname(__file__))
 
 
 class TestLintRoller(TestCase):
 
     def __init__(self, *args, **kwargs):
         TestCase.__init__(self, *args, **kwargs)
 
-        filedir = os.path.join(here, 'files')
-        self.files = [os.path.join(filedir, f) for f in os.listdir(filedir)]
+        self.filedir = os.path.join(here, 'files')
+        self.files = [os.path.join(self.filedir, f) for f in os.listdir(self.filedir)]
         self.lintdir = os.path.join(here, 'linters')
 
         names = ('string.lint', 'regex.lint', 'external.lint')
         self.linters = [os.path.join(self.lintdir, n) for n in names]
 
     def setUp(self):
         TestCase.setUp(self)
         self.lint = LintRoller()
@@ -65,11 +65,16 @@ class TestLintRoller(TestCase):
     def test_roll_with_excluded_path(self):
         self.lint.lintargs.update({'exclude': ['**/foobar.js']})
 
         self.lint.read(self.linters)
         result = self.lint.roll(self.files)
 
         self.assertEqual(len(result), 0)
 
+    def test_roll_with_invalid_extension(self):
+        self.lint.read(os.path.join(self.lintdir, 'external.lint'))
+        result = self.lint.roll(os.path.join(self.filedir, 'foobar.py'))
+        self.assertEqual(len(result), 0)
+
 
 if __name__ == '__main__':
     main()
--- a/python/mozlint/test/test_types.py
+++ b/python/mozlint/test/test_types.py
@@ -66,13 +66,13 @@ class TestLinterTypes(TestCase):
 
     def test_no_filter(self):
         self.lint.read(os.path.join(self.lintdir, 'explicit_path.lint'))
         result = self.lint.roll(self.files)
         self.assertEqual(len(result), 0)
 
         self.lint.lintargs['use_filters'] = False
         result = self.lint.roll(self.files)
-        self.assertEqual(len(result), 1)
+        self.assertEqual(len(result), 2)
 
 
 if __name__ == '__main__':
     main()
--- a/tools/lint/docs/create.rst
+++ b/tools/lint/docs/create.rst
@@ -48,16 +48,17 @@ Each ``.lint`` file must have a variable
 linter. Here are the supported keys:
 
 * name - The name of the linter (required)
 * description - A brief description of the linter's purpose (required)
 * type - One of 'string', 'regex' or 'external' (required)
 * payload - The actual linting logic, depends on the type (required)
 * include - A list of glob patterns that must be matched (optional)
 * exclude - A list of glob patterns that must not be matched (optional)
+* extensions - A list of file extensions to be considered (optional)
 * setup - A function that sets up external dependencies (optional)
 
 In addition to the above, some ``.lint`` files correspond to a single lint rule. For these, the
 following additional keys may be specified:
 
 * message - A string to print on infraction (optional)
 * hint - A string with a clue on how to fix the infraction (optional)
 * rule - An id string for the lint rule (optional)
--- a/tools/lint/flake8.lint
+++ b/tools/lint/flake8.lint
@@ -40,16 +40,17 @@ LINE_OFFSETS = {
     'E302': (-2, 3),
 }
 """Maps a flake8 error to a lineoffset tuple.
 
 The offset is of the form (lineno_offset, num_lines) and is passed
 to the lineoffset property of `ResultContainer`.
 """
 
+EXTENSIONS = ['.py', '.lint']
 results = []
 
 
 def process_line(line):
     try:
         res = json.loads(line)
     except ValueError:
         return
@@ -130,11 +131,12 @@ LINTER = {
         'taskcluster',
         'testing/firefox-ui',
         'testing/marionette/client',
         'testing/puppeteer',
         'testing/talos/',
         'tools/lint',
     ],
     'exclude': [],
+    'extensions': EXTENSIONS,
     'type': 'external',
     'payload': lint,
 }