Improved tools/hacking.py

* cleaner output
* fix bug 980009
* Fix N201
* N306: alphabetical order imports
* N401: docstring start
* N402: one line docstring start
* N403: multi line docstring end
* Until fixed, N40* will be disabled by default

Change-Id: I9addafdaa7a1f8fb950e14a5409f661dec6c7b87

4 files changed, 146 insertions, 46 deletions

diff --git a/HACKING.rst b/HACKING.rst
index 467248e0e9..a28bc37432 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -82,7 +82,7 @@ Example::
   """A one line docstring looks like this and ends in a period."""
 
 
-  """A multiline docstring has a one-line summary, less than 80 characters.
+  """A multi line docstring has a one-line summary, less than 80 characters.
 
   Then a new paragraph after a newline that explains in more detail any
   general information about the function, class or method. Example usages
diff --git a/nova/virt/libvirt/connection.py b/nova/virt/libvirt/connection.py
index a01d96377c..a07e394682 100644
--- a/nova/virt/libvirt/connection.py
+++ b/nova/virt/libvirt/connection.py
@@ -1038,7 +1038,7 @@ class LibvirtConnection(driver.ComputeDriver):
         finally:
             try:
                 os.unlink(testfile)
-            except:
+            except Exception:
                 pass
 
         return hasDirectIO
diff --git a/run_tests.sh b/run_tests.sh
index 97d229f673..62327ad23f 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -108,7 +108,10 @@ function run_pep8 {
   echo "Running PEP8 and HACKING compliance check..."
   # Just run PEP8 in current environment
   #
-  ${wrapper} python tools/hacking.py ${srcfiles}
+
+  # Until all these issues get fixed, ignore.
+  ignore='--ignore=N4,N306'
+  ${wrapper} python tools/hacking.py ${ignore} ${srcfiles}
 }
 
 
diff --git a/tools/hacking.py b/tools/hacking.py
index 187fba8c83..849ac898e2 100755
--- a/tools/hacking.py
+++ b/tools/hacking.py
@@ -22,14 +22,18 @@ built on top of pep8.py
 """
 
 import inspect
+import logging
 import os
 import re
 import sys
 import tokenize
 import traceback
+import warnings
 
 import pep8
 
+# Don't need this for testing
+logging.disable('LOG')
 
 #N1xx comments
 #N2xx except
@@ -40,6 +44,8 @@ import pep8
 #N7xx localization
 
 IMPORT_EXCEPTIONS = ['sqlalchemy', 'migrate', 'nova.db.sqlalchemy.session']
+DOCSTRING_TRIPLE = ['"""', "'''"]
+VERBOSE_MISSING_IMPORT = False
 
 
 def is_import_exception(mod):
@@ -47,8 +53,24 @@ def is_import_exception(mod):
         any(mod.startswith(m + '.') for m in IMPORT_EXCEPTIONS)
 
 
+def import_normalize(line):
+    # convert "from x import y" to "import x.y"
+    # handle "from x import y as z" to "import x.y as z"
+    split_line = line.split()
+    if (line.startswith("from ") and "," not in line and
+           split_line[2] == "import" and split_line[3] != "*" and
+           split_line[1] != "__future__" and
+           (len(split_line) == 4 or
+           (len(split_line) == 6  and split_line[4] == "as"))):
+        mod = split_line[3]
+        return "import %s.%s" % (split_line[1], split_line[3])
+    else:
+        return line
+
+
 def nova_todo_format(physical_line):
-    """
+    """Check for 'TODO()'.
+
     nova HACKING guide recommendation for TODO:
     Include your name with TODOs as in "#TODO(termie)"
     N101
@@ -61,7 +83,8 @@ def nova_todo_format(physical_line):
 
 
 def nova_except_format(logical_line):
-    """
+    """Check for 'except:'.
+
     nova HACKING guide recommends not using except:
     Do not write "except:", use "except Exception:" at the very least
     N201
@@ -70,8 +93,9 @@ def nova_except_format(logical_line):
         return 6, "NOVA N201: no 'except:' at least use 'except Exception:'"
 
 
-def nova_except_format(logical_line):
-    """
+def nova_except_format_assert(logical_line):
+    """Check for 'assertRaises(Exception'.
+
     nova HACKING guide recommends not using assertRaises(Exception...):
     Do not use overly broad Exception type
     N202
@@ -81,7 +105,8 @@ def nova_except_format(logical_line):
 
 
 def nova_one_import_per_line(logical_line):
-    """
+    """Check for import format.
+
     nova HACKING guide recommends one import per line:
     Do not import more than one module per line
 
@@ -96,9 +121,12 @@ def nova_one_import_per_line(logical_line):
        not is_import_exception(parts[1]):
         return pos, "NOVA N301: one import per line"
 
+_missingImport = set([])
+
 
 def nova_import_module_only(logical_line):
-    """
+    """Check for import module only.
+
     nova HACKING guide recommends importing only modules:
     Do not import objects, only modules
     N302 import only modules
@@ -112,24 +140,28 @@ def nova_import_module_only(logical_line):
         """
         current_path = os.path.dirname(pep8.current_file)
         try:
-            valid = True
-            if parent:
-                if is_import_exception(parent):
-                    return
-                parent_mod = __import__(parent, globals(), locals(), [mod], -1)
-                valid = inspect.ismodule(getattr(parent_mod, mod))
-            else:
-                __import__(mod, globals(), locals(), [], -1)
-                valid = inspect.ismodule(sys.modules[mod])
-            if not valid:
-                if added:
-                    sys.path.pop()
-                    added = False
-                    return logical_line.find(mod), ("NOVA N304: No relative "
-                        "imports. '%s' is a relative import" % logical_line)
-
-                return logical_line.find(mod), ("NOVA N302: import only "
-                    "modules. '%s' does not import a module" % logical_line)
+            with warnings.catch_warnings():
+                warnings.simplefilter('ignore', DeprecationWarning)
+                valid = True
+                if parent:
+                    if is_import_exception(parent):
+                        return
+                    parent_mod = __import__(parent, globals(), locals(),
+                        [mod], -1)
+                    valid = inspect.ismodule(getattr(parent_mod, mod))
+                else:
+                    __import__(mod, globals(), locals(), [], -1)
+                    valid = inspect.ismodule(sys.modules[mod])
+                if not valid:
+                    if added:
+                        sys.path.pop()
+                        added = False
+                        return logical_line.find(mod), ("NOVA N304: No "
+                            "relative  imports. '%s' is a relative import"
+                            % logical_line)
+                    return logical_line.find(mod), ("NOVA N302: import only "
+                        "modules. '%s' does not import a module"
+                        % logical_line)
 
         except (ImportError, NameError) as exc:
             if not added:
@@ -137,8 +169,12 @@ def nova_import_module_only(logical_line):
                 sys.path.append(current_path)
                 return importModuleCheck(mod, parent, added)
             else:
-                print >> sys.stderr, ("ERROR: import '%s' failed: %s" %
-                                                       (logical_line, exc))
+                name = logical_line.split()[1]
+                if (name not in _missingImport and name):
+                    if VERBOSE_MISSING_IMPORT:
+                        print >> sys.stderr, ("ERROR: import '%s' failed: %s" %
+                            (name, exc))
+                    _missingImport.add(name)
                 added = False
                 sys.path.pop()
                 return
@@ -148,28 +184,84 @@ def nova_import_module_only(logical_line):
             return logical_line.find(mod), ("NOVA N303: Invalid import, "
                 "AttributeError raised")
 
+    # convert "from x import y" to " import x.y"
+    # convert "from x import y as z" to " import x.y"
+    import_normalize(logical_line)
     split_line = logical_line.split()
 
-    # handle "import x"
-    # handle "import x as y"
     if (logical_line.startswith("import ") and "," not in logical_line and
             (len(split_line) == 2 or
             (len(split_line) == 4 and split_line[2] == "as"))):
         mod = split_line[1]
         return importModuleCheck(mod)
 
-    # handle "from x import y"
-    # handle "from x import y as z"
-    elif (logical_line.startswith("from ") and "," not in logical_line and
-           split_line[2] == "import" and split_line[3] != "*" and
-           split_line[1] != "__future__" and
-           (len(split_line) == 4 or
-           (len(split_line) == 6  and split_line[4] == "as"))):
-        mod = split_line[3]
-        return importModuleCheck(mod, split_line[1])
-
     # TODO(jogo) handle "from x import *"
 
+#TODO(jogo): import template: N305
+
+
+def nova_import_alphabetical(physical_line, line_number, lines):
+    """Check for imports in alphabetical order.
+
+    nova HACKING guide recommendation for imports:
+    imports in human alphabetical order
+    N306
+    """
+    # handle import x
+    # use .lower since capitalization shouldn't dictate order
+    split_line = import_normalize(physical_line.strip()).lower().split()
+    split_previous = import_normalize(lines[line_number - 2]
+            ).strip().lower().split()
+    # with or without "as y"
+    length = [2, 4]
+    if (len(split_line) in length  and len(split_previous) in length and
+        split_line[0] == "import" and  split_previous[0] == "import"):
+        if split_line[1] < split_previous[1]:
+            return (0, "NOVA N306: imports not in alphabetical order (%s, %s)"
+                % (split_previous[1], split_line[1]))
+
+
+def nova_docstring_start_space(physical_line):
+    """Check for docstring not start with space.
+
+    nova HACKING guide recommendation for docstring:
+    Docstring should not start with space
+    N401
+    """
+    pos = max([physical_line.find(i) for i in DOCSTRING_TRIPLE])  # start
+    if (pos != -1 and len(physical_line) > pos + 1):
+        if (physical_line[pos + 3] == ' '):
+            return (pos, "NOVA N401: one line docstring should not start with"
+                " a space")
+
+
+def nova_docstring_one_line(physical_line):
+    """Check one line docstring end.
+
+    nova HACKING guide recommendation for one line docstring:
+    A one line docstring looks like this and ends in a period.
+    N402
+    """
+    pos = max([physical_line.find(i) for i in DOCSTRING_TRIPLE])  # start
+    end = max([physical_line[-4:-1] == i for i in DOCSTRING_TRIPLE])  # end
+    if (pos != -1  and end and len(physical_line) > pos + 4):
+        if (physical_line[-5] != '.'):
+            return pos, "NOVA N402: one line docstring needs a period"
+
+
+def nova_docstring_multiline_end(physical_line):
+    """Check multi line docstring end.
+
+    nova HACKING guide recommendation for docstring:
+    Docstring should end on a new line
+    N403
+    """
+    pos = max([physical_line.find(i) for i in DOCSTRING_TRIPLE])  # start
+    if (pos != -1 and len(physical_line) == pos):
+        print physical_line
+        if (physical_line[pos + 3] == ' '):
+            return (pos, "NOVA N403: multi line docstring end on new line")
+
 
 FORMAT_RE = re.compile("%(?:"
                             "%|"           # Ignore plain percents
@@ -264,15 +356,14 @@ current_file = ""
 
 
 def readlines(filename):
-    """
-    record the current file being tested
-    """
+    """Record the current file being tested."""
     pep8.current_file = filename
     return open(filename).readlines()
 
 
 def add_nova():
-    """
+    """Monkey patch in nova guidelines.
+
     Look for functions that start with nova_  and have arguments
     and add them to pep8 module
     Assumes you know how to write pep8.py checks
@@ -292,4 +383,10 @@ if __name__ == "__main__":
     add_nova()
     pep8.current_file = current_file
     pep8.readlines = readlines
-    pep8._main()
+    try:
+        pep8._main()
+    except SystemExit:
+        if len(_missingImport) > 0:
+            print >> sys.stderr, ("%i Missing imports in this test environment"
+                    % len(_missingImport))
+        raise