Merge "Improve pep8 checks to be similar to those in nova"
diff --git a/HACKING b/HACKING
deleted file mode 100644
index c2c403b..0000000
--- a/HACKING
+++ /dev/null
@@ -1,14 +0,0 @@
-Test Data/Configuration
------------------------
-- Assume nothing about existing test data
-- Tests should be self contained (provide their own data)
-- Clean up test data at the completion of each test
-- Use configuration files for values that will vary by environment
-
-General
--------
-- Put two newlines between top-level code (funcs, classes, etc)
-- Put one newline between methods in classes and anywhere else
-- Do not write "except:", use "except Exception:" at the very least
-- Include your name with TODOs as in "#TODO(termie)"
-- Do not name anything the same name as a built-in or reserved word
\ No newline at end of file
diff --git a/HACKING.rst b/HACKING.rst
new file mode 100644
index 0000000..103f8cd
--- /dev/null
+++ b/HACKING.rst
@@ -0,0 +1,195 @@
+Test Data/Configuration
+-----------------------
+- Assume nothing about existing test data
+- Tests should be self contained (provide their own data)
+- Clean up test data at the completion of each test
+- Use configuration files for values that will vary by environment
+
+
+General
+-------
+- Put two newlines between top-level code (funcs, classes, etc)
+- Put one newline between methods in classes and anywhere else
+- Long lines should be wrapped in parentheses
+ in preference to using a backslash for line continuation.
+- Do not write "except:", use "except Exception:" at the very least
+- Include your name with TODOs as in "#TODO(termie)"
+- Do not name anything the same name as a built-in or reserved word Example::
+
+ def list():
+ return [1, 2, 3]
+
+ mylist = list() # BAD, shadows `list` built-in
+
+ class Foo(object):
+ def list(self):
+ return [1, 2, 3]
+
+ mylist = Foo().list() # OKAY, does not shadow built-in
+
+Imports
+-------
+- Do not import objects, only modules (*)
+- Do not import more than one module per line (*)
+- Do not make relative imports
+- Order your imports by the full module path
+- Organize your imports according to the following template
+
+Example::
+
+ # vim: tabstop=4 shiftwidth=4 softtabstop=4
+ {{stdlib imports in human alphabetical order}}
+ \n
+ {{third-party lib imports in human alphabetical order}}
+ \n
+ {{tempest imports in human alphabetical order}}
+ \n
+ \n
+ {{begin your code}}
+
+
+Human Alphabetical Order Examples
+---------------------------------
+Example::
+
+ import httplib
+ import logging
+ import random
+ import StringIO
+ import time
+ import unittest
+
+ import eventlet
+ import webob.exc
+
+ import tempest.config
+ from tempest.services.compute.json.limits_client import LimitsClientJSON
+ from tempest.services.compute.xml.limits_client import LimitsClientXML
+ from tempest.services.volume.volumes_client import VolumesClientJSON
+ import tempest.test
+
+
+Docstrings
+----------
+Example::
+
+ """A one line docstring looks like this and ends in a period."""
+
+
+ """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
+ are also great to have here if it is a complex class for function.
+
+ When writing the docstring for a class, an extra line should be placed
+ after the closing quotations. For more in-depth explanations for these
+ decisions see http://www.python.org/dev/peps/pep-0257/
+
+ If you are going to describe parameters and return values, use Sphinx, the
+ appropriate syntax is as follows.
+
+ :param foo: the foo parameter
+ :param bar: the bar parameter
+ :returns: return_type -- description of the return value
+ :returns: description of the return value
+ :raises: AttributeError, KeyError
+ """
+
+
+Dictionaries/Lists
+------------------
+If a dictionary (dict) or list object is longer than 80 characters, its items
+should be split with newlines. Embedded iterables should have their items
+indented. Additionally, the last item in the dictionary should have a trailing
+comma. This increases readability and simplifies future diffs.
+
+Example::
+
+ my_dictionary = {
+ "image": {
+ "name": "Just a Snapshot",
+ "size": 2749573,
+ "properties": {
+ "user_id": 12,
+ "arch": "x86_64",
+ },
+ "things": [
+ "thing_one",
+ "thing_two",
+ ],
+ "status": "ACTIVE",
+ },
+ }
+
+
+Calling Methods
+---------------
+Calls to methods 80 characters or longer should format each argument with
+newlines. This is not a requirement, but a guideline::
+
+ unnecessarily_long_function_name('string one',
+ 'string two',
+ kwarg1=constants.ACTIVE,
+ kwarg2=['a', 'b', 'c'])
+
+
+Rather than constructing parameters inline, it is better to break things up::
+
+ list_of_strings = [
+ 'what_a_long_string',
+ 'not as long',
+ ]
+
+ dict_of_numbers = {
+ 'one': 1,
+ 'two': 2,
+ 'twenty four': 24,
+ }
+
+ object_one.call_a_method('string three',
+ 'string four',
+ kwarg1=list_of_strings,
+ kwarg2=dict_of_numbers)
+
+
+OpenStack Trademark
+-------------------
+
+OpenStack is a registered trademark of OpenStack, LLC, and uses the
+following capitalization:
+
+ OpenStack
+
+
+Commit Messages
+---------------
+Using a common format for commit messages will help keep our git history
+readable. Follow these guidelines:
+
+ First, provide a brief summary (it is recommended to keep the commit title
+ under 50 chars).
+
+ The first line of the commit message should provide an accurate
+ description of the change, not just a reference to a bug or
+ blueprint. It must be followed by a single blank line.
+
+ If the change relates to a specific driver (libvirt, xenapi, qpid, etc...),
+ begin the first line of the commit message with the driver name, lowercased,
+ followed by a colon.
+
+ Following your brief summary, provide a more detailed description of
+ the patch, manually wrapping the text at 72 characters. This
+ description should provide enough detail that one does not have to
+ refer to external resources to determine its high-level functionality.
+
+ Once you use 'git review', two lines will be appended to the commit
+ message: a blank line followed by a 'Change-Id'. This is important
+ to correlate this commit with a specific review in Gerrit, and it
+ should not be modified.
+
+For further information on constructing high quality commit messages,
+and how to split up commits into a series of changes, consult the
+project wiki:
+
+ http://wiki.openstack.org/GitCommitMessages
diff --git a/run_tests.sh b/run_tests.sh
index 059ac75..7016a30 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -52,10 +52,13 @@
function run_pep8 {
echo "Running pep8 ..."
- PEP8_EXCLUDE="etc,include,tools,*venv"
- PEP8_OPTIONS="--exclude=$PEP8_EXCLUDE --repeat"
- PEP8_INCLUDE="."
- pep8 $PEP8_OPTIONS $PEP8_INCLUDE
+ srcfiles="`find tempest -type f -name "*.py"`"
+ srcfiles+=" `find tools -type f -name "*.py"`"
+ srcfiles+=" setup.py"
+
+ ignore='--ignore=N4,E121,E122,E125,E126'
+
+ python tools/hacking.py ${ignore} ${srcfiles}
}
NOSETESTS="nosetests $noseargs"
diff --git a/tools/hacking.py b/tools/hacking.py
new file mode 100755
index 0000000..6e66005
--- /dev/null
+++ b/tools/hacking.py
@@ -0,0 +1,484 @@
+#!/usr/bin/env python
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2012, Cloudscaling
+# All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+"""tempest HACKING file compliance testing
+
+built on top of pep8.py
+"""
+
+import fnmatch
+import inspect
+import logging
+import os
+import re
+import subprocess
+import sys
+import tokenize
+import warnings
+
+import pep8
+
+# Don't need this for testing
+logging.disable('LOG')
+
+#N1xx comments
+#N2xx except
+#N3xx imports
+#N4xx docstrings
+#N5xx dictionaries/lists
+#N6xx calling methods
+#N7xx localization
+#N8xx git commit messages
+
+IMPORT_EXCEPTIONS = ['sqlalchemy', 'migrate']
+DOCSTRING_TRIPLE = ['"""', "'''"]
+VERBOSE_MISSING_IMPORT = os.getenv('HACKING_VERBOSE_MISSING_IMPORT', 'False')
+
+
+# Monkey patch broken excluded filter in pep8
+# See https://github.com/jcrocholl/pep8/pull/111
+def excluded(self, filename):
+ """
+ Check if options.exclude contains a pattern that matches filename.
+ """
+ basename = os.path.basename(filename)
+ return any((pep8.filename_match(filename, self.options.exclude,
+ default=False),
+ pep8.filename_match(basename, self.options.exclude,
+ default=False)))
+
+
+def input_dir(self, dirname):
+ """Check all files in this directory and all subdirectories."""
+ dirname = dirname.rstrip('/')
+ if self.excluded(dirname):
+ return 0
+ counters = self.options.report.counters
+ verbose = self.options.verbose
+ filepatterns = self.options.filename
+ runner = self.runner
+ for root, dirs, files in os.walk(dirname):
+ if verbose:
+ print('directory ' + root)
+ counters['directories'] += 1
+ for subdir in sorted(dirs):
+ if self.excluded(os.path.join(root, subdir)):
+ dirs.remove(subdir)
+ for filename in sorted(files):
+ # contain a pattern that matches?
+ if ((pep8.filename_match(filename, filepatterns) and
+ not self.excluded(filename))):
+ runner(os.path.join(root, filename))
+
+
+def is_import_exception(mod):
+ return (mod in IMPORT_EXCEPTIONS or
+ 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 ("import" in line and 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"))):
+ return "import %s.%s" % (split_line[1], split_line[3])
+ else:
+ return line
+
+
+def tempest_todo_format(physical_line):
+ """Check for 'TODO()'.
+
+ tempest HACKING guide recommendation for TODO:
+ Include your name with TODOs as in "#TODO(termie)"
+ N101
+ """
+ pos = physical_line.find('TODO')
+ pos1 = physical_line.find('TODO(')
+ pos2 = physical_line.find('#') # make sure it's a comment
+ if (pos != pos1 and pos2 >= 0 and pos2 < pos):
+ return pos, "TEMPEST N101: Use TODO(NAME)"
+
+
+def tempest_except_format(logical_line):
+ """Check for 'except:'.
+
+ tempest HACKING guide recommends not using except:
+ Do not write "except:", use "except Exception:" at the very least
+ N201
+ """
+ if logical_line.startswith("except:"):
+ yield 6, "TEMPEST N201: no 'except:' at least use 'except Exception:'"
+
+
+def tempest_except_format_assert(logical_line):
+ """Check for 'assertRaises(Exception'.
+
+ tempest HACKING guide recommends not using assertRaises(Exception...):
+ Do not use overly broad Exception type
+ N202
+ """
+ if logical_line.startswith("self.assertRaises(Exception"):
+ yield 1, "TEMPEST N202: assertRaises Exception too broad"
+
+
+def tempest_one_import_per_line(logical_line):
+ """Check for import format.
+
+ tempest HACKING guide recommends one import per line:
+ Do not import more than one module per line
+
+ Examples:
+ BAD: from tempest.common.rest_client import RestClient, RestClientXML
+ N301
+ """
+ pos = logical_line.find(',')
+ parts = logical_line.split()
+ if (pos > -1 and (parts[0] == "import" or
+ parts[0] == "from" and parts[2] == "import") and
+ not is_import_exception(parts[1])):
+ yield pos, "TEMPEST N301: one import per line"
+
+_missingImport = set([])
+
+
+def tempest_import_module_only(logical_line):
+ """Check for import module only.
+
+ tempest HACKING guide recommends importing only modules:
+ Do not import objects, only modules
+ N302 import only modules
+ N303 Invalid Import
+ N304 Relative Import
+ """
+ def importModuleCheck(mod, parent=None, added=False):
+ """
+ If can't find module on first try, recursively check for relative
+ imports
+ """
+ current_path = os.path.dirname(pep8.current_file)
+ try:
+ 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), ("TEMPEST N304: No "
+ "relative imports. "
+ "'%s' is a relative "
+ "import"
+ % logical_line)
+ return logical_line.find(mod), ("TEMPEST N302: import only"
+ " modules. '%s' does not "
+ "import a module"
+ % logical_line)
+
+ except (ImportError, NameError) as exc:
+ if not added:
+ added = True
+ sys.path.append(current_path)
+ return importModuleCheck(mod, parent, added)
+ else:
+ name = logical_line.split()[1]
+ if name not in _missingImport:
+ if VERBOSE_MISSING_IMPORT != 'False':
+ print >> sys.stderr, ("ERROR: import '%s' in %s "
+ "failed: %s" %
+ (name, pep8.current_file, exc))
+ _missingImport.add(name)
+ added = False
+ sys.path.pop()
+ return
+
+ except AttributeError:
+ # Invalid import
+ return logical_line.find(mod), ("TEMPEST 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()
+
+ 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]
+ rval = importModuleCheck(mod)
+ if rval is not None:
+ yield rval
+
+ # TODO(jogo) handle "from x import *"
+
+#TODO(jogo): import template: N305
+
+
+def tempest_import_alphabetical(logical_line, line_number, lines):
+ """Check for imports in alphabetical order.
+
+ Tempest 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(logical_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]:
+ yield (0, "TEMPEST N306: imports not in alphabetical order"
+ " (%s, %s)"
+ % (split_previous[1], split_line[1]))
+
+
+def tempest_docstring_start_space(physical_line):
+ """Check for docstring not start with space.
+
+ tempest 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, "TEMPEST N401: one line docstring should not start"
+ " with a space")
+
+
+def tempest_docstring_one_line(physical_line):
+ """Check one line docstring end.
+
+ tempest 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, "TEMPEST N402: one line docstring needs a period"
+
+
+def tempest_docstring_multiline_end(physical_line):
+ """Check multi line docstring end.
+
+ Tempest 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):
+ if (physical_line[pos + 3] == ' '):
+ return (pos, "TEMPEST N403: multi line docstring end on new line")
+
+
+FORMAT_RE = re.compile("%(?:"
+ "%|" # Ignore plain percents
+ "(\(\w+\))?" # mapping key
+ "([#0 +-]?" # flag
+ "(?:\d+|\*)?" # width
+ "(?:\.\d+)?" # precision
+ "[hlL]?" # length mod
+ "\w))") # type
+
+
+class LocalizationError(Exception):
+ pass
+
+
+def check_i18n():
+ """Generator that checks token stream for localization errors.
+
+ Expects tokens to be ``send``ed one by one.
+ Raises LocalizationError if some error is found.
+ """
+ while True:
+ try:
+ token_type, text, _, _, line = yield
+ except GeneratorExit:
+ return
+ if (token_type == tokenize.NAME and text == "_" and
+ not line.startswith('def _(msg):')):
+
+ while True:
+ token_type, text, start, _, _ = yield
+ if token_type != tokenize.NL:
+ break
+ if token_type != tokenize.OP or text != "(":
+ continue # not a localization call
+
+ format_string = ''
+ while True:
+ token_type, text, start, _, _ = yield
+ if token_type == tokenize.STRING:
+ format_string += eval(text)
+ elif token_type == tokenize.NL:
+ pass
+ else:
+ break
+
+ if not format_string:
+ raise LocalizationError(start,
+ "TEMEPST N701: Empty localization "
+ "string")
+ if token_type != tokenize.OP:
+ raise LocalizationError(start,
+ "TEMPEST N701: Invalid localization "
+ "call")
+ if text != ")":
+ if text == "%":
+ raise LocalizationError(start,
+ "TEMPEST N702: Formatting "
+ "operation should be outside"
+ " of localization method call")
+ elif text == "+":
+ raise LocalizationError(start,
+ "TEMPEST N702: Use bare string "
+ "concatenation instead of +")
+ else:
+ raise LocalizationError(start,
+ "TEMPEST N702: Argument to _ must"
+ " be just a string")
+
+ format_specs = FORMAT_RE.findall(format_string)
+ positional_specs = [(key, spec) for key, spec in format_specs
+ if not key and spec]
+ # not spec means %%, key means %(smth)s
+ if len(positional_specs) > 1:
+ raise LocalizationError(start,
+ "TEMPEST N703: Multiple positional "
+ "placeholders")
+
+
+def tempest_localization_strings(logical_line, tokens):
+ """Check localization in line.
+
+ N701: bad localization call
+ N702: complex expression instead of string as argument to _()
+ N703: multiple positional placeholders
+ """
+
+ gen = check_i18n()
+ next(gen)
+ try:
+ map(gen.send, tokens)
+ gen.close()
+ except LocalizationError as e:
+ yield e.args
+
+#TODO(jogo) Dict and list objects
+
+current_file = ""
+
+
+def readlines(filename):
+ """Record the current file being tested."""
+ pep8.current_file = filename
+ return open(filename).readlines()
+
+
+def add_tempest():
+ """Monkey patch in tempest guidelines.
+
+ Look for functions that start with tempest_ and have arguments
+ and add them to pep8 module
+ Assumes you know how to write pep8.py checks
+ """
+ for name, function in globals().items():
+ if not inspect.isfunction(function):
+ continue
+ args = inspect.getargspec(function)[0]
+ if args and name.startswith("tempest"):
+ exec("pep8.%s = %s" % (name, name))
+
+
+def once_git_check_commit_title():
+ """Check git commit messages.
+
+ tempest HACKING recommends not referencing a bug or blueprint
+ in first line, it should provide an accurate description of the change
+ N801
+ N802 Title limited to 50 chars
+ """
+ #Get title of most recent commit
+
+ subp = subprocess.Popen(['git', 'log', '--no-merges', '--pretty=%s', '-1'],
+ stdout=subprocess.PIPE)
+ title = subp.communicate()[0]
+ if subp.returncode:
+ raise Exception("git log failed with code %s" % subp.returncode)
+
+ #From https://github.com/openstack/openstack-ci-puppet
+ # /blob/master/modules/gerrit/manifests/init.pp#L74
+ #Changeid|bug|blueprint
+ git_keywords = (r'(I[0-9a-f]{8,40})|'
+ '([Bb]ug|[Ll][Pp])[\s\#:]*(\d+)|'
+ '([Bb]lue[Pp]rint|[Bb][Pp])[\s\#:]*([A-Za-z0-9\\-]+)')
+ GIT_REGEX = re.compile(git_keywords)
+
+ error = False
+ #NOTE(jogo) if match regex but over 3 words, acceptable title
+ if GIT_REGEX.search(title) is not None and len(title.split()) <= 3:
+ print ("N801: git commit title ('%s') should provide an accurate "
+ "description of the change, not just a reference to a bug "
+ "or blueprint" % title.strip())
+ error = True
+ if len(title.decode('utf-8')) > 72:
+ print ("N802: git commit title ('%s') should be under 50 chars"
+ % title.strip())
+ error = True
+ return error
+
+if __name__ == "__main__":
+ #include tempest path
+ sys.path.append(os.getcwd())
+ #Run once tests (not per line)
+ once_error = once_git_check_commit_title()
+ #TEMPEST error codes start with an N
+ pep8.ERRORCODE_REGEX = re.compile(r'[EWN]\d{3}')
+ add_tempest()
+ pep8.current_file = current_file
+ pep8.readlines = readlines
+ pep8.StyleGuide.excluded = excluded
+ pep8.StyleGuide.input_dir = input_dir
+ try:
+ pep8._main()
+ sys.exit(once_error)
+ finally:
+ if len(_missingImport) > 0:
+ print >> sys.stderr, ("%i imports missing in this test environment"
+ % len(_missingImport))
diff --git a/tox.ini b/tox.ini
index 433c55f..e2dbdc8 100644
--- a/tox.ini
+++ b/tox.ini
@@ -15,4 +15,4 @@
[testenv:pep8]
deps = pep8==1.3.3
-commands = pep8 --ignore=E121,E122,E125,E126 --repeat --show-source --exclude=.venv,.tox,dist,doc,openstack .
+commands = python tools/hacking.py --ignore=N4,E121,E122,E125,E126 --repeat --show-source --exclude=.venv,.tox,dist,doc,openstack .