twistedchecker

Merge lp:~hawkowl/twistedchecker/fix-percentages-1162202 into lp:~twisted-dev/twistedchecker/trunk

  1. fix-percentages-1162202
  2. Merge into trunk
Proposed by HawkOwl
Status: Merged
Merged at revision: 31
Proposed branch: lp:~hawkowl/twistedchecker/fix-percentages-1162202
Merge into: lp:~twisted-dev/twistedchecker/trunk
Diff against target: 398 lines (+125/-135)
3 files modified
twistedchecker/checkers/pep8format.py (+111/-133)
twistedchecker/functionaltests/general_pep8.py (+11/-1)
twistedchecker/functionaltests/general_pep8.result (+3/-1)
To merge this branch: bzr merge lp:~hawkowl/twistedchecker/fix-percentages-1162202
Reviewer Review Type Date Requested Status
Richard Wall (community) Approve
HawkOwl Needs Resubmitting
Twisted-dev Pending
Review via email: mp+183255@code.launchpad.net

Description of the change

Fixes the "too many blank lines, expected %s" problem

To post a comment you must log in.
lp:~hawkowl/twistedchecker/fix-percentages-1162202 updated
31. By HawkOwl

Fixed PEP8 monkeypatching. Also removed 'old pep8' stuff, as v1-v1.3 onwards support it, which people should be using.

Revision history for this message
Richard Wall (richardw) wrote :
Download full text (6.7 KiB)

Thanks Hawkowl.

The branch works well. It fixes all the incorrect blank line warnings
in the files that I tested with.

Points:
 1. twistedchecker/checkers/pep8format.py
    1. Remove unused imports

       [richard@zorin fix-percentages-1162202]$ pyflakes twistedchecker/checkers/pep8format.py
       twistedchecker/checkers/pep8format.py:4: 'inspect' imported but unused
       twistedchecker/checkers/pep8format.py:6: 'astng' imported but unused
       twistedchecker/checkers/pep8format.py:7: 'Table' imported but unused
       twistedchecker/checkers/pep8format.py:8: 'are_exclusive' imported but unused
       twistedchecker/checkers/pep8format.py:11: 'diff_string' imported but unused
       twistedchecker/checkers/pep8format.py:12: 'EmptyReport' imported but unused

    2. "15 pep8.options = pep8.process_options([file])[0]"
       1. That looks dodgy -- and I'm not convinced it does anything
          -- we're assigning to a global pep8.options but I can't see that being used.

       2. And we're passing the python-file-under-test as an argument
          -- but it looks to me that process_options expects some sort
          of configuration file.

    3. "18 + for item in self._logical_checks:"
       1. The new comment explains well why you're doing this -- but I
          went off checking whether there was a neater workaround -- I
          didn't find anything :(

          1. There's pep8.register_check which seems to be the official
             way for adding new checks.

             + yield 0, "E303 too many blank lines (%d)" % max_blank_lines
             +
             +pep8.register_check(modifiedBlankLines)

          2. ...actually I give up, it seems that register_check just
             appends to the existing check even when the new function
             has the same name 'blank_lines',

          3. Maybe we could disable the standard blank line codes and
             add new ones of our own? Then maybe we wouldn't have
             to do the fragile mapping of pep8 codes to pylint -- if i
             understood the code correctly.

    4. "82 + if text.startswith(msgidInPEP8):" --

       1. Isn't the pep8 message ID always at the start of the output line?

    5. I got this error in the pep8 functional test

       [FAIL]
       Traceback (most recent call last):
         File "/home/richard/projects/TwistedChecker/branches/fix-percentages-1162202/twistedchecker/test/test_runner.py", line 248, in test_functions
           (modulename, predictResult))
         File "/home/richard/projects/Twisted/branches/xmlrpc-multicall-5732-3/twisted/trial/_synctest.py", line 356, in assertEqual
           % (msg, pformat(first), pformat(second)))
       twisted.trial.unittest.FailTest: Incorrect result of twistedchecker.functionaltests.general_pep8, should be:
       ---
       4:W9019
       7:W9020
       10:W9021
       16:W9022
       19:W9023
       22:W9024
       28:W9025
       34:W9026
       ---
       not equal:
       a = '4:W9019\n7:W9020\n10:W9021\n16:W9022\n19:W9023\n22:W9024\n28:W9025\n28:W9025\n34:W9026'
       b = '4:W9019\n7:W9020\n10:W9021\n16:W9022\n19:W9023\n22:W9024\n28:W9025\n34:W9026'

And, here are the hac...

Read more...

review: Needs Fixing
lp:~hawkowl/twistedchecker/fix-percentages-1162202 updated
32. By HawkOwl

Updated pep8format.py according to review, made the tests pass, got rid of some pyflakes reported problems

Revision history for this message
HawkOwl (hawkowl) wrote :

Pushed up something that should fix most of those things!

I am not sure about #2 though - I think it pushes options that the rest of PEP8.py uses, and it works for now...

review: Needs Resubmitting
lp:~hawkowl/twistedchecker/fix-percentages-1162202 updated
33. By Richard Wall

Tidy up pep imports and add trailing newline.

34. By Richard Wall

Fix String formatting operations warnings

35. By Richard Wall

fix comment capitalization and add test-name header

36. By Richard Wall

add missing epydoc

Revision history for this message
Richard Wall (richardw) wrote :

hawkowl.

I made one or two cosmetic changes to your branch in ~richardw/twistedchecker/fix-percentages-1162202-2 which make pep8format.py pass all but the twistedchecker variable name tests.

My view is that we should aim to fix as many twistedchecker warnings as we can in the modules that we touch

I also noticed that you've uncommented the "blank lines between decorator and function" test, but there's no associated pylint code or message so it never shows up.

Also there should be a new section added to the corresponding functional test to test blank lines after decorators.

Finally, I'm almost certain that "15 pep8.options = pep8.process_options([file])[0]" is junk - probably an abandoned attempt to override the blank_lines checker without monkeypatching.
I say delete it, but if not then raise a new ticket to clean up the integration with pep8.

I also note that the original https://github.com/cyli/TwistySublime/blob/master/twisted_pep8.py file from where this was copied, has changed in the last month.

But if anything the monkey patching there is dirtier than in twistedchecker.

So ...let me think....

I'll be glad to see this merged as it is. I'd be even happier if you merge the cosmetic changes from my branch...

and I think you should raise another ticket to design and implement better pep8 integration.

or maybe to abandon pep8 altogether....

or maybe to reimplement twistedchecker as a series of pep8 plugin functions.

review: Approve
lp:~hawkowl/twistedchecker/fix-percentages-1162202 updated
37. By HawkOwl

Added warning for incorrect function decorator spacing

38. By HawkOwl

Less twistedchecker errors! Better pep8 functional tests!

Revision history for this message
HawkOwl (hawkowl) wrote :

Hi Richard,

Working off your branch, I've fixed up most of the things you've mentioned.

I'm inclined to keep twistedchecker's pep8 stuff as is - it works for right now. :)

- hawkowl

review: Needs Resubmitting
lp:~hawkowl/twistedchecker/fix-percentages-1162202 updated
39. By HawkOwl

EOF newline

Revision history for this message
Richard Wall (richardw) wrote :

Thanks hawkowl.

That looks fine now.

jml - Please merge.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'twistedchecker/checkers/pep8format.py'
2--- twistedchecker/checkers/pep8format.py 2012-08-20 10:02:23 +0000
3+++ twistedchecker/checkers/pep8format.py 2013-09-12 20:10:10 +0000
4@@ -1,21 +1,24 @@
5+# -*- test-case-name: twistedchecker.test.test_runner -*-
6+# Copyright (c) Twisted Matrix Laboratories.
7+# See LICENSE for details.
8+
9+"""
10+Checks the code using pep8.py and a custom blank line checker that matches the
11+Twisted Coding Standard.
12+"""
13+
14 import sys
15 import StringIO
16 import re
17-import inspect
18-
19-from logilab import astng
20-from logilab.common.ureports import Table
21-from logilab.astng import are_exclusive
22
23 from pylint.interfaces import IASTNGChecker
24-from pylint.reporters import diff_string
25-from pylint.checkers import BaseChecker, EmptyReport
26+from pylint.checkers import BaseChecker
27
28 import pep8
29-from pep8 import DOCSTRING_REGEX
30-from pep8 import Checker as PEP8OriginalChecker
31-
32-class PEP8WarningRecorder(PEP8OriginalChecker):
33+
34+
35+
36+class PEP8WarningRecorder(pep8.Checker):
37 """
38 A subclass of pep8's checker that records warnings.
39 """
40@@ -26,8 +29,21 @@
41
42 @param file: file to be checked
43 """
44- pep8.options = pep8.process_options([file])[0]
45- PEP8OriginalChecker.__init__(self, file)
46+ pep8.Checker.__init__(self, file)
47+
48+ for item in self._logical_checks:
49+ # This cycles through all of the logical checks that the Checker
50+ # does. The reason why we have to monkeypatch it here is that
51+ # upon init, it makes a list of all of the plugins/functions it
52+ # has, and therefore already /has/ a reference to the
53+ # blank_lines that we don't want. So this cycles over the logical
54+ # checks, and replaces the old blank_lines with the one that
55+ # matches Twisted's blank spaces convention.
56+ if item[0] == "blank_lines":
57+ replacetuple = (item[0], modifiedBlankLines, item[2])
58+ self._logical_checks[
59+ self._logical_checks.index(item)] = replacetuple
60+
61 self.report_error = self.errorRecorder
62 self.warnings = []
63 self.run()
64@@ -44,11 +60,7 @@
65 @param check: check object in pep8
66 """
67 code = text.split(" ")[0]
68- if hasattr(self, 'report'):
69- lineOffset = self.report.line_offset
70- else:
71- # old pep8.py
72- lineOffset = self.line_offset
73+ lineOffset = self.report.line_offset
74
75 self.warnings.append((lineOffset + lineNumber,
76 offset + 1, code, text))
77@@ -58,12 +70,12 @@
78 """
79 Run pep8 checker and record warnings.
80 """
81- # set a stream to replace stdout, and get results in it
82+ # Set a stream to replace stdout, and get results in it
83 stdoutBak = sys.stdout
84 streamResult = StringIO.StringIO()
85 sys.stdout = streamResult
86 try:
87- PEP8OriginalChecker.check_all(self)
88+ pep8.Checker.check_all(self)
89 finally:
90 sys.stdout = stdoutBak
91
92@@ -75,58 +87,62 @@
93 Need pep8 installed.
94 """
95 msgs = {
96- 'W9010': ('Trailing whitespace found in the end of line',
97- 'Used when a line contains a trailing space.'),
98- 'W9011': ('Blank line contains whitespace',
99- 'Used when found a line contains whitespace.'),
100- # messages for checking blank lines
101- 'W9012': ('Expected 2 blank lines, found %s',
102- 'Class-level functions should be separated '
103- 'with 2 blank lines.'),
104- 'W9013': ('Expected 3 blank lines, found %s',
105- 'Top-level functions should be separated '
106- 'with 3 blank lines.'),
107- 'W9015': ('Too many blank lines, expected %s',
108- 'Used when too many blank lines are found.'),
109- 'W9016': ('Too many blank lines after docstring, found %s',
110- 'Used when too many blank lines after docstring are found.'),
111- # general pep8 warnings
112- 'W9017': ('Blank line at end of file',
113- 'More than one blank line found at end of file (W391 in pep8).'),
114- 'W9018': ('No newline at end of file',
115- 'No blank line is found at end of file (W292 in pep8).'),
116- 'W9019': ("Whitespace after '%s'",
117- 'Redundant whitespace found after a symbol (E201 in pep8).'),
118- 'W9020': ("Whitespace before '%s'",
119- 'Redundant whitespace found before a symbol (E202 in pep8).'),
120- 'W9021': ("Missing whitespace after '%s'",
121- "Expect a whitespace after a symbol (E231 in pep8)."),
122- 'W9022': ("Multiple spaces after operator",
123- "Found multiple spaces after an operator (E222 in pep8)."),
124- 'W9023': ("Multiple spaces before operator",
125- "Found multiple spaces before an operator (E221 in pep8)."),
126- 'W9024': ("Missing whitespace around operator",
127- "No space found around an operator (E225 in pep8)."),
128- 'W9025': ("No spaces should be around keyword / parameter equals",
129- "Spaces found around keyword or parameter equals "
130- "(E251 in pep8)."),
131- 'W9026': ("At least two spaces before inline comment",
132- "Found less than two spaces before inline comment "
133- "(E261 in pep8)."),
134+ 'W9010': ('Trailing whitespace found in the end of line',
135+ 'Used when a line contains a trailing space.'),
136+ 'W9011': ('Blank line contains whitespace',
137+ 'Used when found a line contains whitespace.'),
138+ # Messages for checking blank lines
139+ 'W9012': ('Expected 2 blank lines, found %s',
140+ 'Class-level functions should be separated '
141+ 'with 2 blank lines.'),
142+ 'W9013': ('Expected 3 blank lines, found %s',
143+ 'Top-level functions should be separated '
144+ 'with 3 blank lines.'),
145+ 'W9015': ('Too many blank lines, found %s',
146+ 'Used when too many blank lines are found.'),
147+ 'W9016': ('Too many blank lines after docstring, found %s',
148+ 'Used when too many blank lines after docstring are found.'),
149+ 'W9027': ("Blank lines found after a function decorator",
150+ "Function decorators should be followed with blank lines."),
151+ # General pep8 warnings
152+ 'W9017': ('Blank line at end of file',
153+ 'More than one blank line found at EOF (W391 in pep8).'),
154+ 'W9018': ('No newline at end of file',
155+ 'No blank line is found at end of file (W292 in pep8).'),
156+ 'W9019': ("Whitespace after '%s'",
157+ 'Redundant whitespace found after a symbol (E201 in pep8).'),
158+ 'W9020': ("Whitespace before '%s'",
159+ 'Redundant whitespace found before a symbol '
160+ '(E202 in pep8).'),
161+ 'W9021': ("Missing whitespace after '%s'",
162+ "Expect a whitespace after a symbol (E231 in pep8)."),
163+ 'W9022': ("Multiple spaces after operator",
164+ "Found multiple spaces after an operator (E222 in pep8)."),
165+ 'W9023': ("Multiple spaces before operator",
166+ "Found multiple spaces before an operator (E221 in pep8)."),
167+ 'W9024': ("Missing whitespace around operator",
168+ "No space found around an operator (E225 in pep8)."),
169+ 'W9025': ("No spaces should be around keyword / parameter equals",
170+ "Spaces found around keyword or parameter equals "
171+ "(E251 in pep8)."),
172+ 'W9026': ("At least two spaces before inline comment",
173+ "Found less than two spaces before inline comment "
174+ "(E261 in pep8)."),
175 }
176- standardPEP8Messages = ['W%d' % id for id in range(9017,9027)]
177+ standardPEP8Messages = ['W%d' % (id,) for id in range(9017,9027)]
178 pep8Enabled = None
179 __implements__ = IASTNGChecker
180 name = 'pep8'
181- # map pep8 messages to messages in pylint.
182- # it's foramt should look like this:
183+ # Map pep8 messages to messages in pylint.
184+ # The format should look like this:
185 # 'msgid in pep8' : ('msgid in pylint','a string to extract arguments')
186 mapPEP8Messages = {
187 'W291': ('W9010', ''),
188 'W293': ('W9011', ''),
189 'E301': ('W9012', r'expected 2 blank lines, found (\d+)'),
190 'E302': ('W9013', r'expected 3 blank lines, found (\d+)'),
191- 'E303': ('W9015', r'too many blank lines, expected \((\d+)\)'),
192+ 'E303': ('W9015', r'too many blank lines \((\d+)\)'),
193+ 'E304': ('W9027', ''),
194 'E305': ('W9016', r'too many blank lines after docstring \((\d+)\)'),
195 'W391': ('W9017', ''),
196 'W292': ('W9018', ''),
197@@ -151,18 +167,14 @@
198 @param linter: current C{PyLinter} object.
199 """
200 BaseChecker.__init__(self, linter)
201- argumentsBlankLines = inspect.getargspec(pep8.blank_lines).args
202- if 'blank_lines_before_comment' in argumentsBlankLines:
203- # using old pep8.py
204- pep8.blank_lines = modifiedBlankLinesForOldPEP8
205- else:
206- pep8.blank_lines = modifiedBlankLines
207 self.pep8Enabled = self.linter.option_value("pep8")
208
209
210 def visit_module(self, node):
211 """
212 A interface will be called when visiting a module.
213+
214+ @param node: The module node to check.
215 """
216 self._runPEP8Checker(node.file)
217
218@@ -185,10 +197,15 @@
219 line number and message id
220 """
221 if not warnings:
222- # no warnings were found
223+ # No warnings were found
224 return
225 for warning in warnings:
226 linenum, offset, msgidInPEP8, text = warning
227+
228+ if text.startswith(msgidInPEP8):
229+ # If the PEP8 code is at the start of the text, trim it out
230+ text = text[len(msgidInPEP8) + 1:]
231+
232 if msgidInPEP8 in self.mapPEP8Messages:
233 msgid, patternArguments = self.mapPEP8Messages[msgidInPEP8]
234 if (not self.pep8Enabled and
235@@ -204,10 +221,8 @@
236
237
238
239-def checkBlankLinesForPEP8(logical_line, blank_lines,
240- indent_level, line_number,
241- previous_logical, previous_indent_level,
242- blank_lines_before_comment):
243+def modifiedBlankLines(logical_line, blank_lines, indent_level, line_number,
244+ previous_logical, previous_indent_level):
245 """
246 This function is copied from a modified pep8 checker for Twisted.
247 See https://github.com/cyli/TwistySublime/blob/master/twisted_pep8.py
248@@ -233,6 +248,13 @@
249 E304: @decorator\n\ndef a():\n pass
250 E305: "comment"\n\n\ndef a():\n pass
251 E306: variable="value"\ndef a(): pass
252+
253+ @param logical_line: Supplied by PEP8. The content of the line it is dealing with.
254+ @param blank_lines: Supplied by PEP8.
255+ @param indent_level: Supplied by PEP8. The current indent level.
256+ @param line_number: Supplied by PEP8. The current line number.
257+ @param previous_logical: Supplied by PEP8. The previous logical line.
258+ @param previous_indent_level: Supplied by PEP8. The indent level of the previous line.
259 """
260 def isClassDefDecorator(thing):
261 return (thing.startswith('def ') or
262@@ -243,15 +265,14 @@
263 if line_number == 1:
264 return
265
266+ blank_lines_before_comment = 0
267 max_blank_lines = max(blank_lines, blank_lines_before_comment)
268- previous_is_comment = DOCSTRING_REGEX.match(previous_logical)
269+ previous_is_comment = pep8.DOCSTRING_REGEX.match(previous_logical)
270
271 # Check blank lines after a decorator,
272- # but this checking is removed because no this requirement in
273- # Twisted Coding Standard.
274- # if previous_logical.startswith('@'):
275- # if max_blank_lines:
276- # return 0, "E304 blank lines found after function decorator"
277+ if previous_logical.startswith('@'):
278+ if max_blank_lines:
279+ yield 0, "E304 blank lines found after function decorator"
280
281 if isClassDefDecorator(logical_line):
282 if indent_level:
283@@ -259,67 +280,24 @@
284 # the next function
285 if previous_is_comment:
286 if max_blank_lines > 1:
287- return 0, (
288- "E305 too many blank lines after docstring (%d)" %
289- max_blank_lines)
290+ yield 0, (
291+ "E305 too many blank lines after docstring "
292+ "(%d)" % (max_blank_lines,))
293
294- # between first level functions, there should be 2 blank lines.
295+ # Between first level functions, there should be 2 blank lines.
296 # any further indended functions can have one or zero lines
297 else:
298 if not (max_blank_lines == 2 or
299 indent_level > 4 or
300 previous_indent_level <= indent_level):
301- return 0, ("E301 expected 2 blank lines, found %d" %
302- max_blank_lines)
303+ yield 0, ("E301 expected 2 blank lines, "
304+ "found %d" % (max_blank_lines,))
305
306- # top level, there should be 3 blank lines between class/function
307- # definitions (but not necessarily after varable declarations)
308+ # Top level, there should be 3 blank lines between class/function
309+ # definitions (but not necessarily after variable declarations)
310 elif previous_indent_level and max_blank_lines != 3:
311- return (0,
312- "E302 expected 3 blank lines, found %d" % max_blank_lines)
313+ yield 0, ("E302 expected 3 blank lines, "
314+ "found %d" % (max_blank_lines,))
315
316 elif max_blank_lines > 1 and indent_level:
317- return 0, "E303 too many blank lines, expected (%d)" % max_blank_lines
318-
319-
320-
321-def modifiedBlankLinesForOldPEP8(logical_line, blank_lines,
322- indent_level, line_number,
323- previous_logical, previous_indent_level,
324- blank_lines_before_comment):
325- """
326- This function is same as modifiedBlankLines,
327- but supports old version of pep8.py.
328- """
329- return checkBlankLinesForPEP8(logical_line, blank_lines,
330- indent_level, line_number,
331- previous_logical, previous_indent_level,
332- blank_lines_before_comment)
333-
334-
335-
336-def modifiedBlankLines(logical_line, blank_lines, indent_level, line_number,
337- previous_logical, previous_indent_level):
338- """
339- A function for checking blank lines as a replacement for that in pep8.py.
340-
341- Okay: def a():\n pass\n\n\n\ndef b():\n pass
342- Okay: class A():\n pass\n\n\n\nclass B():\n pass
343- Okay: def a():\n pass\n\n\n# Foo\n# Bar\n\ndef b():\n pass
344-
345- E301: class Foo:\n b = 0\n def bar():\n pass
346- E302: def a():\n pass\n\ndef b(n):\n pass
347- E303: def a():\n pass\n\n\n\ndef b(n):\n pass
348- E303: def a():\n\n\n\n pass
349- E304: @decorator\n\ndef a():\n pass
350- E305: "comment"\n\n\ndef a():\n pass
351- E306: variable="value"\ndef a(): pass
352- """
353- result = checkBlankLinesForPEP8(logical_line, blank_lines,
354- indent_level, line_number,
355- previous_logical, previous_indent_level,
356- 0)
357- if result:
358- yield result
359- else:
360- return
361+ yield 0, "E303 too many blank lines (%d)" % (max_blank_lines,)
362
363=== modified file 'twistedchecker/functionaltests/general_pep8.py'
364--- twistedchecker/functionaltests/general_pep8.py 2012-08-06 01:16:51 +0000
365+++ twistedchecker/functionaltests/general_pep8.py 2013-09-12 20:10:10 +0000
366@@ -1,4 +1,4 @@
367-# enable: W9019,W9020,W9021,W9022,W9023,W9024,W9025,W9026
368+# enable: W9019,W9020,W9021,W9022,W9023,W9024,W9025,W9026,W9027
369
370 # Whitespace after '['.
371 a = [ 1, 2]
372@@ -36,3 +36,13 @@
373 # Good example.
374 c = 1 # an inline comment
375
376+# Example of a well-done decorator
377+@imaginarydecorator
378+def correct_decorator():
379+ pass
380+
381+# Example of a decorator with incorrect spacing
382+@imaginarydecorator
383+
384+def incorrect_decorator():
385+ pass
386
387=== modified file 'twistedchecker/functionaltests/general_pep8.result'
388--- twistedchecker/functionaltests/general_pep8.result 2012-08-06 01:16:51 +0000
389+++ twistedchecker/functionaltests/general_pep8.result 2013-09-12 20:10:10 +0000
390@@ -5,4 +5,6 @@
391 19: W9023
392 22: W9024
393 28: W9025
394-34: W9026
395\ No newline at end of file
396+28: W9025
397+34: W9026
398+47: W9027

Subscribers

People subscribed via source and target branches

to all changes: