[tor-commits] [stem/master] Running PEP8 checks under a '--style' arg

[atagar at torproject.org]

commit 662a1e00746ecbfdece8136b92f9da9494007ad5
Author: Damian Johnson <atagar at torproject.org>
Date:   Sun Jan 6 19:26:00 2013 -0800

    Running PEP8 checks under a '--style' arg
    
    Well... damn. I had hoped that we would be able to wrap the PEP8 style checks
    into our normal testing runs (the reason check_whitespace.py works so well
    is that it's trivial to do with every test run).
    
    However, pep8 takes on the order of ten seconds to run over our codebase. This
    is twice as long as all of our unit tests and hence too long to include in
    tests without opting in.
    
    I'm keeping my fingers crossed that they're doing something stupid with ignored
    issues so we'll get better performance when we've made it happier. We'll see.
    
    On the up side PEP8 is wonderfully configurable with respect to what it accepts
    and ignores so we can start using it right away. Also, the output is well
    formed so it was trivial to wrap it into our present style checker (which means
    consolidated and colorized output - yay!).
---
 run_tests.py             |   26 +++++++++++++++++---------
 test/check_whitespace.py |   38 ++++++++++++++++++++++++++++++++++++++
 test/settings.cfg        |    1 +
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/run_tests.py b/run_tests.py
index 55948bd..9675706 100755
--- a/run_tests.py
+++ b/run_tests.py
@@ -69,13 +69,14 @@ import test.integ.util.proc
 import test.integ.util.system
 import test.integ.version
 
-OPT = "uit:l:c:h"
-OPT_EXPANDED = ["unit", "integ", "targets=", "test=", "log=", "tor=", "config=", "help"]
+OPT = "uist:l:c:h"
+OPT_EXPANDED = ["unit", "integ", "style", "targets=", "test=", "log=", "tor=", "config=", "help"]
 DIVIDER = "=" * 70
 
 CONFIG = stem.util.conf.config_dict("test", {
   "argument.unit": False,
   "argument.integ": False,
+  "argument.style": False,
   "argument.test": "",
   "argument.log": None,
   "argument.tor": "tor",
@@ -189,6 +190,8 @@ def load_user_configuration(test_config):
       arg_overrides["argument.unit"] = "true"
     elif opt in ("-i", "--integ"):
       arg_overrides["argument.integ"] = "true"
+    elif opt in ("-s", "--style"):
+      arg_overrides["argument.style"] = "true"
     elif opt in ("-c", "--config"):
       config_path = os.path.abspath(arg)
     elif opt in ("-t", "--targets"):
@@ -458,17 +461,22 @@ if __name__ == '__main__':
     # TODO: note unused config options afterward?
   
   base_path = os.path.sep.join(__file__.split(os.path.sep)[:-1])
-  whitespace_issues = test.check_whitespace.get_issues(os.path.join(base_path, "stem"))
-  whitespace_issues.update(test.check_whitespace.get_issues(os.path.join(base_path, "test")))
-  whitespace_issues.update(test.check_whitespace.get_issues(os.path.join(base_path, "run_tests.py")))
+  style_issues = test.check_whitespace.get_issues(os.path.join(base_path, "stem"))
+  style_issues.update(test.check_whitespace.get_issues(os.path.join(base_path, "test")))
+  style_issues.update(test.check_whitespace.get_issues(os.path.join(base_path, "run_tests.py")))
   
-  if whitespace_issues:
-    test.output.print_line("WHITESPACE ISSUES", term.Color.BLUE, term.Attr.BOLD)
+  if CONFIG["argument.style"]:
+    style_issues.update(test.check_whitespace.pep8_issues(os.path.join(base_path, "stem")))
+    style_issues.update(test.check_whitespace.pep8_issues(os.path.join(base_path, "test")))
+    style_issues.update(test.check_whitespace.pep8_issues(os.path.join(base_path, "run_tests.py")))
+  
+  if style_issues:
+    test.output.print_line("STYLE ISSUES", term.Color.BLUE, term.Attr.BOLD)
     
-    for file_path in whitespace_issues:
+    for file_path in style_issues:
       test.output.print_line("* %s" % file_path, term.Color.BLUE, term.Attr.BOLD)
       
-      for line_number, msg in whitespace_issues[file_path]:
+      for line_number, msg in style_issues[file_path]:
         line_count = "%-4s" % line_number
         test.output.print_line("  line %s - %s" % (line_count, msg))
       
diff --git a/test/check_whitespace.py b/test/check_whitespace.py
index 7f7b46e..f9008f5 100644
--- a/test/check_whitespace.py
+++ b/test/check_whitespace.py
@@ -20,9 +20,47 @@ from __future__ import with_statement
 import re
 import os
 
+from stem.util import system
+
 # if ran directly then run over everything one level up
 DEFAULT_TARGET = os.path.sep.join(__file__.split(os.path.sep)[:-1])
 
+def pep8_issues(base_path = DEFAULT_TARGET):
+  """
+  Checks for stylistic issues that are an issue according to the parts of PEP8
+  we conform to.
+  
+  :param str base_path: directory to be iterated over
+  
+  :returns: dict of the form ``path => [(line_number, message)...]``
+  """
+  
+  # pep8 give output of the form...
+  #
+  #   FILE:LINE:CHARACTER ISSUE
+  #
+  # ... for instance...
+  #
+  #   ./test/mocking.py:868:31: E225 missing whitespace around operator
+  
+  # TODO: Presently this is a list of all issues pep8 complains about in stem.
+  # We're gonna trim these down by cateogry but include the pep8 checks to
+  # prevent regression.
+  
+  ignored_issues = "E111,E121,W293,E501,E302,E701,E251,E261,W391,E127,E241,E128,E226,E231,E202,E201,E203,E124,E211,E222,E225,E221,E126,E262,E271,E502,E303,E711"
+  
+  issues = {}
+  pep8_output = system.call("pep8 --ignore %s %s" % (ignored_issues, base_path))
+  
+  for line in pep8_output:
+    line_match = re.match("^(.*):(\d+):(\d+): (.*)$", line)
+    
+    if line_match:
+      path, line, _, issue = line_match.groups()
+      issues.setdefault(path, []).append((int(line), issue))
+  
+  return issues
+
 def get_issues(base_path = DEFAULT_TARGET):
   """
   Checks python source code in the given directory for whitespace issues.
diff --git a/test/settings.cfg b/test/settings.cfg
index 9aadcda..cf827cd 100644
--- a/test/settings.cfg
+++ b/test/settings.cfg
@@ -38,6 +38,7 @@
 
 argument.unit false
 argument.integ false
+argument.style false
 argument.test
 argument.log
 argument.tor tor