[arm/master] Checking pep8 as part of our tests

1 Jan 2014

commit a9e1a21545a4016f865f2c97014db6e9e6ae9206
Author: Damian Johnson <atagar@torproject.org>
Date:   Wed Jan 1 13:08:20 2014 -0800

    Checking pep8 as part of our tests
    
    This presently blacklists all of the files we haven't rewritten yet (which is
    most of them). Pep8 works on a directory level so unfortunately we need to skip
    after the actual check (so this still adds a substantial bit to our test
    runtime).
---
 run_tests.py      |  152 ++++++++++++++++++++++++++++++++++++++++++++---------
 test/settings.cfg |   47 +++++++++++++++++
 2 files changed, 173 insertions(+), 26 deletions(-)

diff --git a/run_tests.py b/run_tests.py
index 0135708..3ed5e25 100755
--- a/run_tests.py
+++ b/run_tests.py
@@ -18,6 +18,7 @@ from arm.util import load_settings
 
 CONFIG = stem.util.conf.config_dict("test", {
   "pep8.ignore": [],
+  "pep8.blacklist": [],
   "pyflakes.ignore": [],
 })
 
@@ -29,6 +30,41 @@ SRC_PATHS = [os.path.join(ARM_BASE, path) for path in (
 )]
 
 
+def main():
+  load_settings()
+
+  test_config = stem.util.conf.get_config("test")
+  test_config.load(os.path.join(ARM_BASE, "test", "settings.cfg"))
+
+  clean_orphaned_pyc()
+
+  tests = unittest.defaultTestLoader.discover('test', pattern='*.py')
+  test_runner = unittest.TextTestRunner()
+  test_runner.run(tests)
+
+  print
+
+  static_check_issues = {}
+
+  if stem.util.system.is_available("pyflakes"):
+    static_check_issues.update(get_pyflakes_issues(SRC_PATHS))
+
+  if stem.util.system.is_available("pep8"):
+    static_check_issues.update(get_stylistic_issues(SRC_PATHS))
+
+  if static_check_issues:
+    print "STATIC CHECKS"
+
+    for file_path in static_check_issues:
+      print "* %s" % file_path
+
+      for line_number, msg in static_check_issues[file_path]:
+        line_count = "%-4s" % line_number
+        print "  line %s - %s" % (line_count, msg)
+
+      print
+
+
 def clean_orphaned_pyc():
   for root, _, files in os.walk(os.path.dirname(__file__)):
     for filename in files:
@@ -43,6 +79,80 @@ def clean_orphaned_pyc():
           os.remove(pyc_path)
 
 
+def get_stylistic_issues(paths):
+  """
+  Checks for stylistic issues that are an issue according to the parts of PEP8
+  we conform to. This alsochecks a few other stylistic issues:
+
+  * two space indentations
+  * tabs are the root of all evil and should be shot on sight
+  * standard newlines (\\n), not windows (\\r\\n) nor classic mac (\\r)
+  * checks that we're using 'as' for exceptions rather than a comma
+
+  :param list paths: paths to search for stylistic issues
+
+  :returns: **dict** of the form ``path => [(line_number, message)...]``
+  """
+
+  ignored_issues = ','.join(CONFIG["pep8.ignore"])
+  issues = {}
+
+  for path in paths:
+    pep8_output = stem.util.system.call(
+      "pep8 --ignore %s %s" % (ignored_issues, path),
+      ignore_exit_status = True,
+    )
+
+    for line in pep8_output:
+      line_match = re.match("^(.*):(\d+):(\d+): (.*)$", line)
+
+      if line_match:
+        path, line, _, issue = line_match.groups()
+
+        if path in CONFIG["pep8.blacklist"]:
+          continue
+
+        issues.setdefault(path, []).append((int(line), issue))
+
+    for file_path in _get_files_with_suffix(path):
+      if file_path in CONFIG["pep8.blacklist"]:
+        continue
+
+      with open(file_path) as f:
+        file_contents = f.read()
+
+      lines, file_issues, prev_indent = file_contents.split("\n"), [], 0
+      is_block_comment = False
+
+      for index, line in enumerate(lines):
+        whitespace, content = re.match("^(\s*)(.*)$", line).groups()
+
+        if '"""' in content:
+          is_block_comment = not is_block_comment
+
+        if "\t" in whitespace:
+          file_issues.append((index + 1, "indentation has a tab"))
+        elif "\r" in content:
+          file_issues.append((index + 1, "contains a windows newline"))
+        elif content != content.rstrip():
+          file_issues.append((index + 1, "line has trailing whitespace"))
+        elif content.lstrip().startswith("except") and content.endswith(", exc:"):
+          # Python 2.6 - 2.7 supports two forms for exceptions...
+          #
+          #   except ValueError, exc:
+          #   except ValueError as exc:
+          #
+          # The former is the old method and no longer supported in python 3
+          # going forward.
+
+          file_issues.append((index + 1, "except clause should use 'as', not comma"))
+
+      if file_issues:
+        issues[file_path] = file_issues
+
+  return issues
+
+
 def get_pyflakes_issues(paths):
   """
   Performs static checks via pyflakes.
@@ -97,36 +207,26 @@ def get_pyflakes_issues(paths):
   return issues
 
 
-def main():
-  load_settings()
-
-  test_config = stem.util.conf.get_config("test")
-  test_config.load(os.path.join(ARM_BASE, "test", "settings.cfg"))
-
-  clean_orphaned_pyc()
-
-  tests = unittest.defaultTestLoader.discover('test', pattern='*.py')
-  test_runner = unittest.TextTestRunner()
-  test_runner.run(tests)
-
-  print
-
-  static_check_issues = {}
-
-  if stem.util.system.is_available("pyflakes"):
-    static_check_issues.update(get_pyflakes_issues(SRC_PATHS))
+def _get_files_with_suffix(base_path, suffix = ".py"):
+  """
+  Iterates over files in a given directory, providing filenames with a certain
+  suffix.
 
-  if static_check_issues:
-    print "STATIC CHECKS"
+  :param str base_path: directory to be iterated over
+  :param str suffix: filename suffix to look for
 
-    for file_path in static_check_issues:
-      print "* %s" % file_path
+  :returns: iterator that yields the absolute path for files with the given suffix
+  """
 
-      for line_number, msg in static_check_issues[file_path]:
-        line_count = "%-4s" % line_number
-        print "  line %s - %s" % (line_count, msg)
+  if os.path.isfile(base_path):
+    if base_path.endswith(suffix):
+      yield base_path
+  else:
+    for root, _, files in os.walk(base_path):
+      for filename in files:
+        if filename.endswith(suffix):
+          yield os.path.join(root, filename)
 
-      print
 
 if __name__ == '__main__':
   main()
diff --git a/test/settings.cfg b/test/settings.cfg
index cf0049c..169a016 100644
--- a/test/settings.cfg
+++ b/test/settings.cfg
@@ -1,3 +1,50 @@
+# PEP8 compliance issues that we're ignoreing.
+
+pep8.ignore E111
+pep8.ignore E121
+pep8.ignore E501
+pep8.ignore E251
+pep8.ignore E127
+
+# Files we haven't gotten around to revisiing yet. This list will be going away
+# completely before our next release.
+
+pep8.blacklist ./arm/connections/__init__.py
+pep8.blacklist ./arm/connections/circEntry.py
+pep8.blacklist ./arm/connections/connEntry.py
+pep8.blacklist ./arm/connections/connPanel.py
+pep8.blacklist ./arm/connections/countPopup.py
+pep8.blacklist ./arm/connections/descriptorPopup.py
+pep8.blacklist ./arm/connections/entries.py
+
+pep8.blacklist ./arm/graphing/__init__.py
+pep8.blacklist ./arm/graphing/bandwidthStats.py
+pep8.blacklist ./arm/graphing/connStats.py
+pep8.blacklist ./arm/graphing/graphPanel.py
+pep8.blacklist ./arm/graphing/resourceStats.py
+
+pep8.blacklist ./arm/menu/__init__.py
+pep8.blacklist ./arm/menu/actions.py
+pep8.blacklist ./arm/menu/item.py
+pep8.blacklist ./arm/menu/menu.py
+
+pep8.blacklist ./arm/util/connections.py
+pep8.blacklist ./arm/util/panel.py
+pep8.blacklist ./arm/util/sysTools.py
+pep8.blacklist ./arm/util/textInput.py
+pep8.blacklist ./arm/util/torConfig.py
+pep8.blacklist ./arm/util/torTools.py
+pep8.blacklist ./arm/util/uiTools.py
+
+pep8.blacklist ./arm/__init__.py
+pep8.blacklist ./arm/configPanel.py
+pep8.blacklist ./arm/controller.py
+pep8.blacklist ./arm/headerPanel.py
+pep8.blacklist ./arm/logPanel.py
+pep8.blacklist ./arm/popups.py
+pep8.blacklist ./arm/prereq.py
+pep8.blacklist ./arm/torrcPanel.py
+
 # False positives from pyflakes. These are mappings between the path and the
 # issue.

    

atagar＠torproject.org

tags

participants (1)