Add pylint

README.mdown

@@ -240,6 +240,19 @@ issues.
 
 * `ignore` A comma separated list of error codes you wish to ignore.
 
+#### pylint
+
+Uses [pylint](https://www.pylint.org/) to check python files.
+
+*Options*
+
+* `disable` A comma separated list of error codes you wish to ignore. Passed as
+   a command line option to pylint.
+* `enable` A comma separated list of error codes you wish to include. Best if
+   used with `disable: "all"`. Passed as a command line option to
+   pylint
+* `config` Provide a path to the pylintrc config file for pylint.
+
 ### PHP
 
 #### PHPCS

docker/python3.Dockerfile

@@ -1,7 +1,9 @@
 FROM python:3.6-alpine
 
 RUN mkdir /src \
-  && mkdir /tool
+  && mkdir /tool \
+  && apk add musl-dev gcc  \
+  && rm -rf /var/cache/apk/*
 
 COPY requirements-py3.txt /tool
 

docker/requirements-py3.txt

@@ -2,3 +2,4 @@ pep8>=1.7.0,<2.0.0
 flake8>=3.3.0,<3.4.0
 autopep8>=1.3.0,<2.0.0
 black==18.6b4
+pylint>=2.3.1,<3.0.0

lintreview/tools/py3k.py

@@ -1,66 +1,32 @@
 from __future__ import absolute_import
-import os
+
 import logging
-import lintreview.docker as docker
-from lintreview.tools import Tool, process_quickfix, stringify
+
+from lintreview.tools import stringify
+from lintreview.tools.pylint import Pylint
 
 log = logging.getLogger(__name__)
 
 
-class Py3k(Tool):
+class Py3k(Pylint):
     """
     $ pylint --py3k is a special mode for porting to python 3 which
     disables other pylint checkers.
     see https://github.com/PyCQA/pylint/issues/761
     """
 
     name = 'py3k'
+    accepted_options = ('ignore', )
+    python_version = 'python2'
 
-    def check_dependencies(self):
-        """
-        See if python image is available
-        """
-        return docker.image_exists('python2')
-
-    def match_file(self, filename):
-        base = os.path.basename(filename)
-        name, ext = os.path.splitext(base)
-        return ext == '.py'
-
-    def process_files(self, files):
-        """
-        Run code checks with pylint --py3k.
-        Only a single process is made for all files
-        to save resources.
-        """
-        log.debug('Processing %s files with %s', files, self.name)
-        command = self.make_command(files)
-        output = docker.run('python2', command, self.base_path)
-        if not output:
-            log.debug('No py3k errors found.')
-            return False
-
-        output = output.split("\n")
-        output = [line for line in output if not line.startswith("*********")]
+    def make_command(self, files):
+        self.check_options()
 
-        process_quickfix(self.problems, output, docker.strip_base)
+        command = self._base_command
+        command.append('--py3k')
 
-    def make_command(self, files):
-        msg_template = '{path}:{line}:{column}:{msg_id} {msg}'
-        command = [
-            'pylint',
-            '--py3k',
-            '--reports=n',
-            '--msg-template',
-            msg_template,
-        ]
-        accepted_options = ('ignore')
         if 'ignore' in self.options:
             command.extend(['-d', stringify(self.options['ignore'])])
 
-        for option in self.options:
-            if option in accepted_options:
-                continue
-            log.warning('Set non-existent py3k option: %s', option)
         command.extend(files)
         return command

lintreview/tools/pylint.py

@@ -0,0 +1,76 @@
+from __future__ import absolute_import
+
+import logging
+import os
+
+import lintreview.docker as docker
+from lintreview.tools import Tool, process_quickfix, python_image, stringify
+
+log = logging.getLogger(__name__)
+
+
+class Pylint(Tool):
+
+    name = 'pylint'
+
+    accepted_options = ('python', 'disable', 'enable', 'config')
+
+    @property
+    def _base_command(self):
+        return [
+            'pylint',
+            '--persistent=n',
+            '--reports=n',
+            '--score=n',
+            '--msg-template',
+            '{path}:{line}:{column}:{msg_id} {msg}'
+        ]
+
+    @property
+    def python_version(self):
+        return python_image(self.options)
+
+    def check_dependencies(self):
+        """
+        See if python image is available
+        """
+        return docker.image_exists(self.python_version)
+
+    def match_file(self, filename):
+        base = os.path.basename(filename)
+        name, ext = os.path.splitext(base)
+        return ext == '.py'
+
+    def process_files(self, files):
+        log.debug('Processing %s files with %s', files, self.name)
+        command = self.make_command(files)
+        output = docker.run(self.python_version, command, source_dir=self.base_path)
+        if not output:
+            log.debug('No %s errors found.', self.name)
+            return False
+
+        output = output.split("\n")
+        output = [line for line in output if not line.startswith("*********")]
+
+        process_quickfix(self.problems, output, docker.strip_base)
+
+    def make_command(self, files):
+        self.check_options()
+
+        command = self._base_command
+
+        if self.options.get('disable'):
+            command.extend(['--disable', stringify(self.options['disable'])])
+
+        if self.options.get('enable'):
+            command.extend(['--enable', stringify(self.options['enable'])])
+
+        if self.options.get('config'):
+            command.extend(['--rcfile', docker.apply_base(stringify(self.options['config']))])
+
+        command.extend(files)
+        return command
+
+    def check_options(self):
+        for unaccepted_option in set(self.options.keys()) - set(self.accepted_options):
+            log.warning('Set non-existent %s option: %s', self.name, unaccepted_option)

tests/fixtures/pylint/has_errors.py

@@ -0,0 +1,16 @@
+# Create unused imports
+import os, re
+
+def thing(self):
+    return  thing_two('arg1',
+     'arg2')
+
+
+def thing_two(arg1, arg2, arg3):
+    """A thinger for thinging but returning nothing
+    """
+    result=arg1*arg2
+    if result == arg1:
+        pass
+    elif result == '':
+        pass

tests/fixtures/pylint/no_errors.py

@@ -0,0 +1,12 @@
+"""Sample python file with no pylint errors.
+used for testing the pylint.Tool
+"""
+
+from __future__ import print_function
+
+
+def does_something(thing):
+    """Function docstring
+    """
+    print('hello')
+    return thing.buzz()

tests/fixtures/pylint/sample_rcfile

@@ -0,0 +1,30 @@
+[MASTER]
+load-plugins=pylint.extensions.emptystring
+
+[MESSAGES CONTROL]
+
+disable=W0611,
+        E1120,
+        W0613,
+        C0111,
+        C0330,
+        C0326,
+        C0410
+
+[REPORTS]
+# test that msg_template is not overwritten by the rcfile, otherwise
+# lint-review won't know what to do with the messages
+msg-template="{path}:{line}:{column}:{msg_id} foo"
+
+[FORMAT]
+
+# Maximum number of characters on a single line.
+max-line-length=50
+
+# Maximum number of lines in a module
+max-module-lines=3
+
+[DESIGN]
+
+# Maximum number of arguments for function / method
+max-args=1

tests/tools/test_py3k.py

@@ -74,3 +74,7 @@ def test_process_files_two_files(self):
             Comment(fname, 11, 11,
                     'W1638 range built-in referenced when not iterating')
         ], problems)
+
+    def test_py3k_ignore_py3(self):
+        self.tool = Py3k(self.problems, {'python': 3})
+        eq_('python2', self.tool.python_version)

tests/tools/test_pylint.py

@@ -0,0 +1,129 @@
+from __future__ import absolute_import
+
+from unittest import TestCase
+
+from nose.tools import eq_
+from tests import requires_image, root_dir
+
+from lintreview.review import Comment, Problems
+from lintreview.tools.pylint import Pylint
+
+
+class TestPylint(TestCase):
+
+    fixtures = [
+        'tests/fixtures/pylint/no_errors.py',
+        'tests/fixtures/pylint/has_errors.py',
+    ]
+
+    def setUp(self):
+        self.problems = Problems()
+        self.tool = Pylint(self.problems, {}, root_dir)
+
+    def test_match_file(self):
+        self.assertFalse(self.tool.match_file('test.php'))
+        self.assertFalse(self.tool.match_file('test.js'))
+        self.assertFalse(self.tool.match_file('dir/name/test.js'))
+        self.assertTrue(self.tool.match_file('test.py'))
+        self.assertTrue(self.tool.match_file('dir/name/test.py'))
+
+    def test_process_files__one_file_pass(self):
+        self.tool.process_files([self.fixtures[0]])
+        eq_([], self.problems.all(self.fixtures[0]))
+
+    @requires_image('python2')
+    def test_process_files__one_file_fail(self):
+        self.tool.process_files([self.fixtures[1]])
+        problems = self.problems.all(self.fixtures[1])
+        eq_(7, len(problems))
+
+        fname = self.fixtures[1]
+        # pylint outputs are sorted by 'category', not line number
+        eq_([
+            Comment(fname, 6, 6, "C0330 Wrong continued indentation (add 17 spaces)."),
+            Comment(fname, 12, 12, "C0326 Exactly one space required around assignment"),
+            Comment(fname, 1, 1, "C0111 Missing module docstring"),
+            Comment(fname, 2, 2, "C0410 Multiple imports on one line (os, re)\n"
+                    "W0611 Unused import re\nW0611 Unused import os"),
+            Comment(fname, 4, 4, "C0111 Missing function docstring\nW0613 Unused argument 'self'"),
+            Comment(fname, 5, 5, "E1120 No value for argument 'arg3' in function call"),
+            Comment(fname, 9, 9, "W0613 Unused argument 'arg3'")
+        ], problems)
+
+    @requires_image('python2')
+    def test_process_files_two_files(self):
+        self.tool.process_files(self.fixtures)
+
+        eq_([], self.problems.all(self.fixtures[0]))
+
+        problems = self.problems.all(self.fixtures[1])
+        eq_(7, len(problems))
+        expected = Comment(self.fixtures[1], 6, 6,
+                           "C0330 Wrong continued indentation (add 17 spaces).")
+        eq_(expected, problems[0])
+
+    @requires_image('python3')
+    def test_process_files_two_files__python3(self):
+        self.tool.options['python'] = 3
+        self.tool.process_files(self.fixtures)
+
+        eq_([], self.problems.all(self.fixtures[0]))
+
+        problems = self.problems.all(self.fixtures[1])
+        eq_(7, len(problems))
+        expected = Comment(self.fixtures[1], 6, 6,
+                           "C0330 Wrong continued indentation (add 17 spaces).")
+        eq_(expected, problems[0])
+
+    @requires_image('python2')
+    def test_process_absolute_container_path(self):
+        fixtures = ['/src/' + path for path in self.fixtures]
+        self.tool.process_files(fixtures)
+
+        eq_([], self.problems.all(self.fixtures[0]))
+
+        problems = self.problems.all(self.fixtures[1])
+        assert len(problems) >= 6
+
+    @requires_image('python2')
+    def test_process_files__disable(self):
+        options = {
+            'disable': 'C,W0611,E1120',
+        }
+        self.tool = Pylint(self.problems, options, root_dir)
+        self.tool.process_files([self.fixtures[1]])
+        problems = self.problems.all(self.fixtures[1])
+        eq_([
+            Comment(self.fixtures[1], 4, 4, "W0613 Unused argument 'self'"),
+            Comment(self.fixtures[1], 9, 9, "W0613 Unused argument 'arg3'"),
+        ], problems)
+
+    @requires_image('python2')
+    def test_process_files__enable(self):
+        options = {
+            'disable': 'C,W,E',
+            'enable': 'W0613',
+        }
+        self.tool = Pylint(self.problems, options, root_dir)
+        self.tool.process_files([self.fixtures[1]])
+        problems = self.problems.all(self.fixtures[1])
+        eq_([
+            Comment(self.fixtures[1], 4, 4, "W0613 Unused argument 'self'"),
+            Comment(self.fixtures[1], 9, 9, "W0613 Unused argument 'arg3'"),
+        ], problems)
+
+    @requires_image('python2')
+    def test_process_files__config(self):
+        options = {
+            'config': 'tests/fixtures/pylint/sample_rcfile',
+        }
+        self.tool = Pylint(self.problems, options, root_dir)
+        filename = self.fixtures[1]
+        self.tool.process_files([filename])
+        problems = self.problems.all(self.fixtures[1])
+        eq_([
+            Comment(filename, 10, 10, "C0301 Line too long (51/50)"),
+            Comment(filename, 1, 1, "C0302 Too many lines in module (16/3)"),
+            Comment(filename, 9, 9, "R0913 Too many arguments (3/1)"),
+            Comment(filename, 15, 15, "C1901 Avoid comparisons to empty string"),
+        ], problems)