From 1b6d6829b6c9115794f3e9cbfac8d77e9271dc22 Mon Sep 17 00:00:00 2001
From: Farid Rener <proteusvacuum@gmail.com>
Date: Fri, 15 Mar 2019 11:45:31 -0400
Subject: [PATCH 1/4] Add pylint command

---
 docker/python3.Dockerfile           |   4 +-
 docker/requirements-py3.txt         |   1 +
 lintreview/tools/py3k.py            |  48 +++--------
 lintreview/tools/pylint.py          |  67 +++++++++++++++
 tests/fixtures/pylint/has_errors.py |  14 +++
 tests/fixtures/pylint/no_errors.py  |  12 +++
 tests/fixtures/pylint/sample_rcfile |  27 ++++++
 tests/tools/test_pylint.py          | 128 ++++++++++++++++++++++++++++
 8 files changed, 262 insertions(+), 39 deletions(-)
 create mode 100644 lintreview/tools/pylint.py
 create mode 100644 tests/fixtures/pylint/has_errors.py
 create mode 100644 tests/fixtures/pylint/no_errors.py
 create mode 100644 tests/fixtures/pylint/sample_rcfile
 create mode 100644 tests/tools/test_pylint.py

diff --git a/docker/python3.Dockerfile b/docker/python3.Dockerfile
index 66c477bd..0ed5a59d 100644
--- a/docker/python3.Dockerfile
+++ b/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
 
diff --git a/docker/requirements-py3.txt b/docker/requirements-py3.txt
index ca68bc13..890f00c0 100644
--- a/docker/requirements-py3.txt
+++ b/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
diff --git a/lintreview/tools/py3k.py b/lintreview/tools/py3k.py
index 492051e9..350c4670 100644
--- a/lintreview/tools/py3k.py
+++ b/lintreview/tools/py3k.py
@@ -1,13 +1,15 @@
 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.
@@ -15,6 +17,7 @@ class Py3k(Tool):
     """
 
     name = 'py3k'
+    accepted_options = ('ignore', )
 
     def check_dependencies(self):
         """
@@ -22,45 +25,14 @@ def check_dependencies(self):
         """
         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
diff --git a/lintreview/tools/pylint.py b/lintreview/tools/pylint.py
new file mode 100644
index 00000000..64f02b9d
--- /dev/null
+++ b/lintreview/tools/pylint.py
@@ -0,0 +1,67 @@
+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 = ('disable', 'enable', 'config')
+
+    @property
+    def _base_command(self):
+        return [
+            'pylint',
+            '--persistent=n',
+            '--reports=n',
+            '--score=n',
+            '--msg-template',
+            '{path}:{line}:{column}:{msg_id} {msg}'
+        ]
+
+    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)
+        image = python_image(self.options)
+        output = docker.run(image, 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.accepted_options) - set(self.options.keys()):
+            log.warning('Set non-existent %s option: %s', self.name, unaccepted_option)
diff --git a/tests/fixtures/pylint/has_errors.py b/tests/fixtures/pylint/has_errors.py
new file mode 100644
index 00000000..c9343689
--- /dev/null
+++ b/tests/fixtures/pylint/has_errors.py
@@ -0,0 +1,14 @@
+# 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
diff --git a/tests/fixtures/pylint/no_errors.py b/tests/fixtures/pylint/no_errors.py
new file mode 100644
index 00000000..37388355
--- /dev/null
+++ b/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()
diff --git a/tests/fixtures/pylint/sample_rcfile b/tests/fixtures/pylint/sample_rcfile
new file mode 100644
index 00000000..ff7cabe5
--- /dev/null
+++ b/tests/fixtures/pylint/sample_rcfile
@@ -0,0 +1,27 @@
+[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
diff --git a/tests/tools/test_pylint.py b/tests/tools/test_pylint.py
new file mode 100644
index 00000000..cb8f7d75
--- /dev/null
+++ b/tests/tools/test_pylint.py
@@ -0,0 +1,128 @@
+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 (14/3)"),
+            Comment(filename, 9, 9, "R0913 Too many arguments (3/1)"),
+        ], problems)

From 85d07e94e4d79d8950e95c8fa7949f871c4a5eb9 Mon Sep 17 00:00:00 2001
From: Farid Rener <proteusvacuum@gmail.com>
Date: Fri, 15 Mar 2019 11:54:58 -0400
Subject: [PATCH 2/4] Add docs for pylint

---
 README.mdown | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/README.mdown b/README.mdown
index fa99a506..d72e2842 100644
--- a/README.mdown
+++ b/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

From 4e7450f6c0eada432435cd06eddf20bcfa9822b9 Mon Sep 17 00:00:00 2001
From: Farid Rener <proteusvacuum@gmail.com>
Date: Fri, 5 Apr 2019 16:50:43 -0400
Subject: [PATCH 3/4] Only use python2 image for py3k checker

---
 lintreview/tools/py3k.py   |  8 +-------
 lintreview/tools/pylint.py | 17 +++++++++++++----
 tests/tools/test_py3k.py   |  4 ++++
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/lintreview/tools/py3k.py b/lintreview/tools/py3k.py
index 350c4670..139534e2 100644
--- a/lintreview/tools/py3k.py
+++ b/lintreview/tools/py3k.py
@@ -2,7 +2,6 @@
 
 import logging
 
-import lintreview.docker as docker
 from lintreview.tools import stringify
 from lintreview.tools.pylint import Pylint
 
@@ -18,12 +17,7 @@ class Py3k(Pylint):
 
     name = 'py3k'
     accepted_options = ('ignore', )
-
-    def check_dependencies(self):
-        """
-        See if python image is available
-        """
-        return docker.image_exists('python2')
+    python_version = 'python2'
 
     def make_command(self, files):
         self.check_options()
diff --git a/lintreview/tools/pylint.py b/lintreview/tools/pylint.py
index 64f02b9d..3b80271c 100644
--- a/lintreview/tools/pylint.py
+++ b/lintreview/tools/pylint.py
@@ -13,7 +13,7 @@ class Pylint(Tool):
 
     name = 'pylint'
 
-    accepted_options = ('disable', 'enable', 'config')
+    accepted_options = ('python', 'disable', 'enable', 'config')
 
     @property
     def _base_command(self):
@@ -26,6 +26,16 @@ def _base_command(self):
             '{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)
@@ -34,8 +44,7 @@ def match_file(self, filename):
     def process_files(self, files):
         log.debug('Processing %s files with %s', files, self.name)
         command = self.make_command(files)
-        image = python_image(self.options)
-        output = docker.run(image, command, source_dir=self.base_path)
+        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
@@ -63,5 +72,5 @@ def make_command(self, files):
         return command
 
     def check_options(self):
-        for unaccepted_option in set(self.accepted_options) - set(self.options.keys()):
+        for unaccepted_option in set(self.options.keys()) - set(self.accepted_options):
             log.warning('Set non-existent %s option: %s', self.name, unaccepted_option)
diff --git a/tests/tools/test_py3k.py b/tests/tools/test_py3k.py
index 30e8e87a..7def0049 100644
--- a/tests/tools/test_py3k.py
+++ b/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)

From 61d75fb10ae3fa52eab41b8e62f436cf1a995bef Mon Sep 17 00:00:00 2001
From: Farid Rener <proteusvacuum@gmail.com>
Date: Fri, 5 Apr 2019 16:51:07 -0400
Subject: [PATCH 4/4] Add test for pylint plugins

---
 tests/fixtures/pylint/has_errors.py | 2 ++
 tests/fixtures/pylint/sample_rcfile | 3 +++
 tests/tools/test_pylint.py          | 3 ++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/fixtures/pylint/has_errors.py b/tests/fixtures/pylint/has_errors.py
index c9343689..23fe2306 100644
--- a/tests/fixtures/pylint/has_errors.py
+++ b/tests/fixtures/pylint/has_errors.py
@@ -12,3 +12,5 @@ def thing_two(arg1, arg2, arg3):
     result=arg1*arg2
     if result == arg1:
         pass
+    elif result == '':
+        pass
diff --git a/tests/fixtures/pylint/sample_rcfile b/tests/fixtures/pylint/sample_rcfile
index ff7cabe5..ac6be2fa 100644
--- a/tests/fixtures/pylint/sample_rcfile
+++ b/tests/fixtures/pylint/sample_rcfile
@@ -1,3 +1,6 @@
+[MASTER]
+load-plugins=pylint.extensions.emptystring
+
 [MESSAGES CONTROL]
 
 disable=W0611,
diff --git a/tests/tools/test_pylint.py b/tests/tools/test_pylint.py
index cb8f7d75..654e59ce 100644
--- a/tests/tools/test_pylint.py
+++ b/tests/tools/test_pylint.py
@@ -123,6 +123,7 @@ def test_process_files__config(self):
         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 (14/3)"),
+            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)