diff --git a/HACKING b/HACKING deleted file mode 100644 index c2c403bd1d..0000000000 --- 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 0000000000..103f8cd763 --- /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 059ac75d36..7016a30b6f 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -52,10 +52,13 @@ function run_tests { 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 0000000000..6e66005771 --- /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 433c55f2c3..e2dbdc842a 100644 --- a/tox.ini +++ b/tox.ini @@ -15,4 +15,4 @@ commands = nosetests {posargs} [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 .