Merge lp:~adiroiban/pocket-lint/1237862-pep-257 into lp:pocket-lint

[Adi Roiban]

Why
---

pep257 looks like a nice effort in docstring format checking.

Changes
-------

Add optional pep257 check if module is present.

I added pep257 as setup.py dependency, but maybe we should remove it.

pep257 has no errror codes yet (wip: https://github.com/GreenSteam/pep257/pull/53) so the filtering is done on message string.

I also made a drive-by fix for pdb check.

Please let me know if changes make sense.

[Curtis Hovey (sinzui)]

Hi Adi.

The tests don't pass when pep257 is not installed. We don't require the lib so I think the tests should skip.

I am not sure about the requires pep257. It is not packaged in Ubuntu. The dh will report an error when it calls setup to install during the package build. While I can copy the pep257 from someone-else's archive, pocket-lint will be removed from Ubuntu unless someone commits to creating python3-pep257 and possibly python2-pep257. I'd think removing pep257 from requires will prevent regressions.

pocket-lint now wrongly reports that a pdb is left in the code. Unless you know how to update the pdb check to know the string is not a function, this block needs to be restored.
- pdb_call = 'pdb.' + 'set_trace'
- if pdb_call in line:

review: Needs Information (code)

[Adi Roiban (adiroiban)]

Thanks for the review.

pep257 is still young and unstable so I don't think that it needs to be an Ubuntu package.

I will push a new change removing the install requirement and also skipping the tests when pep257 is not there.

I will also revert the pdb_call variable, but I think that it needs a comment. Otherwise is just triggers a WTF moment.

[lp:~adiroiban/pocket-lint/1237862-pep-257]

512. By Adi Roiban <email address hidden> on 2013-11-07

Fix after review.

[Adi Roiban (adiroiban)]

Please check latest changes.

Thanks!

review: Needs Resubmitting

[Adi Roiban (adiroiban)]

When you have time, please check latest changes. Thanks!

[lp:~adiroiban/pocket-lint/1237862-pep-257]

513. By Adi Roiban <email address hidden> on 2014-02-05

Merge branch 'master' into 1237862-pep-257

[Curtis Hovey (sinzui)]

Thank you Adi.

I am very sorry for ignoring your reviews. I saw you updated the reviews while I was travelling. I was so exhausted when I returned home, I forgot about them. I should have set a reminder.

review: Approve (code)

=== modified file 'pocketlint/formatcheck.py'
--- pocketlint/formatcheck.py	2013-11-06 15:21:20 +0000
+++ pocketlint/formatcheck.py	2014-02-05 15:58:01 +0000
@@ -83,6 +83,13 @@
 except ImportError:
     closure_linter = None
 
+try:
+    import pep257
+    # Shut up the linter.
+    pep257
+except ImportError:
+    pep257 = None
+
 
 IS_PY3 = True if sys.version_info >= (3,) else False
 
@@ -627,6 +634,7 @@
         self.check_text()
         self.check_flakes()
         self.check_pep8()
+        self.check_pep257()
         self.check_windows_endlines()
 
     def check_flakes(self):
@@ -665,6 +673,24 @@
             message = "%s: %s" % (message, location[3].strip())
             self.message(location[1], message, icon='error')
 
+    def check_pep257(self):
+        """PEP 257 docstring style checker."""
+        if not pep257:
+            # PEP257 is not available.
+            return
+
+        ignore_list = getattr(self.options, 'pep257_ignore', [])
+
+        results = pep257.check_source(self.text, self.file_path)
+
+        for error in results:
+            # PEP257 message contains the short error as first line from
+            # the long docstring explanation.
+            error_message = error.explanation.splitlines()[0]
+            if error_message in ignore_list:
+                continue
+            self.message(error.line, error_message, icon='error')
+
     def check_text(self):
         """Call each line_method for each line in text."""
         for line_no, line in enumerate(self.text.splitlines()):
@@ -678,7 +704,8 @@
             self.check_ascii(line_no, line)
 
     def check_pdb(self, line_no, line):
-        """Check the length of the line."""
+        """Check for pdb breakpoints."""
+        # Set trace call is split so that this file will pass the linter.
         pdb_call = 'pdb.' + 'set_trace'
         if pdb_call in line:
             self.message(

=== modified file 'pocketlint/tests/test_python.py'
--- pocketlint/tests/test_python.py	2013-02-16 18:03:51 +0000
+++ pocketlint/tests/test_python.py	2014-02-05 15:58:01 +0000
@@ -7,10 +7,13 @@
     unicode_literals,
 )
 
+import unittest
 from tempfile import NamedTemporaryFile
 
 from pocketlint.formatcheck import (
     get_option_parser,
+    # Imported via pocketlint to avoid duplication of conditional import.
+    pep257,
     PythonChecker,
 )
 from pocketlint.tests import CheckerTestCase
@@ -82,6 +85,12 @@
 """
 
 
+class Bunch(object):
+    """Collector of a bunch of named stuff."""
+    def __init__(self, **kwds):
+        self.__dict__.update(kwds)
+
+
 class TestPyflakes(CheckerTestCase):
     """Verify pyflakes integration."""
 
@@ -233,6 +242,89 @@
             self.reporter.messages)
 
 
[e-mail](pep257 is None, 'pep257 is not available.')
+class TestPEP257(CheckerTestCase):
+    """Verify PEP257 integration."""
+
+    def setUp(self):
+        super(TestPEP257, self).setUp()
+        self.file = NamedTemporaryFile(prefix='pocketlint_')
+
+    def tearDown(self):
+        self.file.close()
+
+    def makeChecker(self, content, options=None):
+        """Create a Python checker with file from `content`."""
+        # Don't know why multi-line text is interpreted as Unicode.
+        content = content.encode('utf-8')
+        return PythonChecker(
+            self.file.name, content, self.reporter, options=options)
+
+    def test_code_without_issues(self):
+        """No errors are reported if everything has a valid docstring."""
+        checker = self.makeChecker('''
+"""Module's docstring."""
+
+class SomeClass(object):
+
+        """Class with short docstring."""
+
+        def method(self):
+            """Method with short docstring."""
+
+        def otherMethod(self):
+            """
+            Method with multi.
+
+            Line docstring.
+
+            """
+        ''')
+
+        checker.check_pep257()
+
+        self.assertEqual([], self.reporter.messages)
+
+    pep257_without_docstrings = '''
+def some_function():
+    """Bad multi line without point"""
+    '''
+
+    def test_code_with_issues(self):
+        """Errors are reported when docstrings are missing or in bad format.
+
+        """
+        checker = self.makeChecker(self.pep257_without_docstrings)
+
+        checker.check_pep257()
+
+        self.assertEqual([
+            (1, 'All modules should have docstrings.'),
+            (3, 'First line should end with a period.'),
+            ],
+            self.reporter.messages,
+            )
+
+    def test_code_with_ignore(self):
+        """A list with error message which should be ignored can be
+        provided as `pep257_ignore` option.
+
+        Hope that pep257 will add error codes soon.
+        """
+        options = Bunch(
+            pep257_ignore=['First line should end with a period.'])
+        checker = self.makeChecker(
+            self.pep257_without_docstrings, options=options)
+
+        checker.check_pep257()
+
+        self.assertEqual([
+            (1, 'All modules should have docstrings.'),
+            ],
+            self.reporter.messages,
+            )
+
+
 class TestText(CheckerTestCase, TestAnyTextMixin):
     """Verify text integration."""