Align to OpenStack Hacking guidelines.

Change-Id: I8eeea3be46d4943c462772de26b10142f426a00c

HACKING

@@ -1,16 +0,0 @@
-Development of git-review is managed by OpenStack's Gerrit, which can be
-found at https://review.openstack.org/
-
-Instructions on submitting patches can be found at
-http://wiki.openstack.org/GerritWorkflow
-
-git-review should, in general, not depend on a huge number of external
-libraries, so that installing it is a lightweight operation.
-
-All code should be pep8-compliant. It won't merge otherwise.
-
-A tox config file has been added for easy local testing. Just run:
-
-  tox
-
-And it will check the code for you.

HACKING.rst

@@ -0,0 +1,311 @@
+Hacking git-review
+==================
+
+Development of git-review is managed by OpenStack's Gerrit, which can be
+found at https://review.openstack.org/
+
+Instructions on submitting patches can be found at
+http://wiki.openstack.org/GerritWorkflow
+
+git-review should, in general, not depend on a huge number of external
+libraries, so that installing it is a lightweight operation.
+
+OpenStack Style Commandments
+============================
+
+- Step 1: Read http://www.python.org/dev/peps/pep-0008/
+- Step 2: Read http://www.python.org/dev/peps/pep-0008/ again
+- Step 3: Read on
+
+
+General
+-------
+- Put two newlines between top-level code (funcs, classes, etc)
+- Use only UNIX style newlines ("\n"), not Windows style ("\r\n")
+- 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 shadow 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
+
+- Use the "is not" operator when testing for unequal identities. Example::
+
+    if not X is Y:  # BAD, intended behavior is ambiguous
+        pass
+
+    if X is not Y:  # OKAY, intuitive
+        pass
+
+- Use the "not in" operator for evaluating membership in a collection. Example::
+
+    if not X in Y:  # BAD, intended behavior is ambiguous
+        pass
+
+    if X not in Y:  # OKAY, intuitive
+        pass
+
+    if not (X in Y or X in Z):  # OKAY, still better than all those 'not's
+        pass
+
+
+Imports
+-------
+- Do not import objects, only modules (*)
+- Do not import more than one module per line (*)
+- Do not use wildcard ``*`` import (*)
+- Do not make relative imports
+- Do not make new nova.db imports in nova/virt/*
+- Order your imports by the full module path
+- Organize your imports according to the following template
+
+(*) exceptions are:
+
+- imports from ``migrate`` package
+- imports from ``sqlalchemy`` package
+- imports from ``nova.db.sqlalchemy.session`` module
+- imports from ``nova.db.sqlalchemy.migration.versioning_api`` package
+
+Example::
+
+  # vim: tabstop=4 shiftwidth=4 softtabstop=4
+  {{stdlib imports in human alphabetical order}}
+  \n
+  {{third-party lib imports in human alphabetical order}}
+  \n
+  {{nova 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 nova.api.ec2
+  from nova.api import openstack
+  from nova.auth import users
+  from nova.endpoint import cloud
+  import nova.flags
+  from nova import 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)
+
+
+Internationalization (i18n) Strings
+-----------------------------------
+In order to support multiple languages, we have a mechanism to support
+automatic translations of exception and log strings.
+
+Example::
+
+    msg = _("An error occurred")
+    raise HTTPBadRequest(explanation=msg)
+
+If you have a variable to place within the string, first internationalize the
+template string then do the replacement.
+
+Example::
+
+    msg = _("Missing parameter: %s") % ("flavor",)
+    LOG.error(msg)
+
+If you have multiple variables to place in the string, use keyword parameters.
+This helps our translators reorder parameters when needed.
+
+Example::
+
+    msg = _("The server with id %(s_id)s has no key %(m_key)s")
+    LOG.error(msg % {"s_id": "1234", "m_key": "imageId"})
+
+
+Creating Unit Tests
+-------------------
+For every new feature, unit tests should be created that both test and
+(implicitly) document the usage of said feature. If submitting a patch for a
+bug that had no unit test, a new passing unit test should be added. If a
+submitted bug fix does have a unit test, be sure to add a new one that fails
+without the patch and passes with the patch.
+
+For more information on creating unit tests and utilizing the testing
+infrastructure in OpenStack Nova, please read nova/tests/README.rst.
+
+
+Running Tests
+-------------
+The testing system is based on a combination of tox and testr. The canonical
+approach to running tests is to simply run the command `tox`. This will
+create virtual environments, populate them with depenedencies and run all of
+the tests that OpenStack CI systems run. Behind the scenes, tox is running
+`testr run --parallel`, but is set up such that you can supply any additional
+testr arguments that are needed to tox. For example, you can run:
+`tox -- --analyze-isolation` to cause tox to tell testr to add
+--analyze-isolation to its argument list.
+
+It is also possible to run the tests inside of a virtual environment
+you have created, or it is possible that you have all of the dependencies
+installed locally already. In this case, you can interact with the testr
+command directly. Running `testr run` will run the entire test suite. `testr
+run --parallel` will run it in parallel (this is the default incantation tox
+uses.) More information about testr can be found at:
+http://wiki.openstack.org/testr
+
+
+openstack-common
+----------------
+
+A number of modules from openstack-common are imported into the project.
+
+These modules are "incubating" in openstack-common and are kept in sync
+with the help of openstack-common's update.py script. See:
+
+  http://wiki.openstack.org/CommonLibrary#Incubation
+
+The copy of the code should never be directly modified here. Please
+always update openstack-common first and then run the script to copy
+the changes across.
+
+OpenStack Trademark
+-------------------
+
+OpenStack is a registered trademark of the OpenStack Foundation, 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 of 50 characters or less.  Summaries
+  of greater then 72 characters will be rejected by the gate.
+
+  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

MANIFEST.in

@@ -1,6 +1,6 @@
 include README.md
 include LICENSE
 include AUTHORS
-include HACKING
+include HACKING.rst
 include git-review.1
 include tox.ini

git-review

@@ -28,17 +28,21 @@ import sys
 import time
 
 if sys.version < '3':
-    from urllib import urlopen
-    from urlparse import urlparse
     import ConfigParser
+    import urllib
+    import urlparse
+    urlopen = urllib.urlopen
+    urlparse = urlparse.urlparse
     do_input = raw_input
 else:
-    from urllib.request import urlopen
-    from urllib.parse import urlparse
     import configparser as ConfigParser
+    import urllib.parse
+    import urllib.request
+    urlopen = urllib.request.urlopen
+    urlparse = urllib.parse.urlparse
     do_input = input
 
-from distutils.version import StrictVersion
+from distutils import version as du_version
 
 version = "1.21"
 
@@ -127,7 +131,8 @@ def run_command(*argv, **env):
 def run_command_exc(klazz, *argv, **env):
     """Run command *argv, on failure raise 'klazz
 
-    klass should be derived from CommandFailed"""
+    klass should be derived from CommandFailed
+    """
     (rc, output) = run_command_status(*argv, **env)
     if rc != 0:
         raise klazz(rc, output, argv, env)
@@ -135,7 +140,7 @@ def run_command_exc(klazz, *argv, **env):
 
 
 def update_latest_version(version_file_path):
-    """ Cache the latest version of git-review for the upgrade check. """
+    """Cache the latest version of git-review for the upgrade check."""
 
     if not os.path.exists(CONFIGDIR):
         os.makedirs(CONFIGDIR)
@@ -147,7 +152,7 @@ def update_latest_version(version_file_path):
     latest_version = version
     try:
         latest_version = json.load(urlopen(PYPI_URL))['info']['version']
-    except:
+    except Exception:
         pass
 
     with open(version_file_path, "w") as version_file:
@@ -155,7 +160,7 @@ def update_latest_version(version_file_path):
 
 
 def latest_is_newer():
-    """ Check if there is a new version of git-review. """
+    """Check if there is a new version of git-review."""
 
     # Skip version check if distro package turns it off
     if os.path.exists(GLOBAL_CONFIG):
@@ -170,14 +175,14 @@ def latest_is_newer():
 
     latest_version = None
     with open(version_file_path, "r") as version_file:
-        latest_version = StrictVersion(version_file.read())
+        latest_version = du_version.StrictVersion(version_file.read())
-    if latest_version > StrictVersion(version):
+    if latest_version > du_version.StrictVersion(version):
         return True
     return False
 
 
 def git_directories():
-    """Determine (absolute git work directory path, .git subdirectory path)"""
+    """Determine (absolute git work directory path, .git subdirectory path)."""
     cmd = ("git", "rev-parse", "--show-toplevel", "--git-dir")
     out = run_command_exc(GitDirectoriesException, *cmd)
     try:
@@ -187,18 +192,19 @@ def git_directories():
 
 
 class GitDirectoriesException(CommandFailed):
-    "Cannot determine where .git directory is"
+    "Cannot determine where .git directory is."
     EXIT_CODE = 70
 
 
 class CustomScriptException(CommandFailed):
-    """Custom script execution failed"""
+    """Custom script execution failed."""
     EXIT_CODE = 71
 
 
 def run_custom_script(action):
     """Get status and output of .git/hooks/$action-review or/and
-    ~/.config/hooks/$action-review if existing."""
+    ~/.config/hooks/$action-review if existing.
+    """
     returns = []
     script_file = "%s-review" % (action)
     (top_dir, git_dir) = git_directories()
@@ -240,7 +246,7 @@ class CannotInstallHook(CommandFailed):
 
 
 def set_hooks_commit_msg(remote, target_file):
-    """ Install the commit message hook if needed. """
+    """Install the commit message hook if needed."""
 
     # Create the hooks directory if it's not there already
     hooks_dir = os.path.dirname(target_file)
@@ -272,7 +278,7 @@ def set_hooks_commit_msg(remote, target_file):
 
 
 def test_remote(username, hostname, port, project):
-    """ Tests that a possible gerrit remote works """
+    """Tests that a possible gerrit remote works."""
 
     if port is not None:
         port = "-p %s" % port
@@ -298,7 +304,7 @@ def test_remote(username, hostname, port, project):
 
 
 def make_remote_url(username, hostname, port, project):
-    """ Builds a gerrit remote URL """
+    """Builds a gerrit remote URL."""
     if username is None:
         return "ssh://%s:%s/%s" % (hostname, port, project)
     else:
@@ -306,7 +312,7 @@ def make_remote_url(username, hostname, port, project):
 
 
 def add_remote(hostname, port, project, remote):
-    """ Adds a gerrit remote. """
+    """Adds a gerrit remote."""
     asked_for_username = False
 
     username = os.getenv("USERNAME")
@@ -460,7 +466,7 @@ def check_remote(branch, remote, hostname, port, project):
     # Gerrit remote not present, try to add it
     try:
         add_remote(hostname, port, project, remote)
-    except:
+    except Exception:
         print(sys.exc_info()[2])
         print("We don't know where your gerrit is. Please manually create ")
         print("a remote named \"%s\" and try again." % remote)
@@ -564,7 +570,7 @@ def assert_one_change(remote, branch, yes, have_hook):
 
 
 def use_topic(why, topic):
-    """Inform the user about why a particular topic has been selected"""
+    """Inform the user about why a particular topic has been selected."""
     if VERBOSE:
         print(why % ('"%s"' % topic,))
     return topic
@@ -653,7 +659,7 @@ def list_reviews(remote):
             continue
         try:
             review_info = json.loads(line)
-        except:
+        except Exception:
             if VERBOSE:
                 print(output)
             raise(CannotParseOpenChangesets, sys.exc_info()[1])
@@ -711,7 +717,8 @@ class ReviewNotFound(ChangeSetException):
 class PatchSetGitFetchFailed(CommandFailed):
     """Cannot fetch patchset contents
 
-Does specified change number belong to this project?"""
+Does specified change number belong to this project?
+"""
     EXIT_CODE = 37
 
 
@@ -769,7 +776,7 @@ def fetch_review(review, masterbranch, remote):
         try:
             review_info = json.loads(review_json)
             found_review = True
-        except:
+        except Exception:
             pass
         if found_review:
             break
@@ -814,7 +821,8 @@ def fetch_review(review, masterbranch, remote):
 
 def checkout_review(branch_name):
     """Checkout a newly fetched (FETCH_HEAD) change
-       into a branch"""
+       into a branch
+    """
 
     try:
         run_command_exc(CheckoutNewBranchFailed,
@@ -954,7 +962,8 @@ def main():
 
     class DownloadFlag(argparse.Action):
         """Additional option parsing: store value in 'dest', but
-           at the same time set one of the flag options to True"""
+           at the same time set one of the flag options to True
+        """
         def __call__(self, parser, namespace, values, option_string=None):
             setattr(namespace, self.dest, values)
             setattr(namespace, self.const, True)

setup.py

@@ -14,10 +14,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from setuptools import setup
+from distutils.command import install as du_install
-from distutils.command.install import install as du_install
-from setuptools.command.install import install
 import os.path
+import setuptools
+from setuptools.command import install
 import sys
 
 version = None
@@ -28,12 +28,12 @@ exec(open("git-review").read())
 __name__ = savename
 
 
-class git_review_install(install):
+class git_review_install(install.install):
     # Force single-version-externally-managed
     # This puts the manpage in the right location (instead of buried
     # in an egg)
     def run(self):
-        return du_install.run(self)
+        return du_install.install.run(self)
 
 git_review_cmdclass = {'install': git_review_install}
 
@@ -45,7 +45,7 @@ if os.path.exists(os.path.join(sys.prefix, 'man')):
     # like to symlink Unixish /usr/local/man to /usr/local/share/man.
     manpath = 'man'
 
-setup(
+setuptools.setup(
     name='git-review',
     version=version,
     cmdclass=git_review_cmdclass,

tox.ini

@@ -7,6 +7,7 @@ setenv = VIRTUAL_ENV={envdir}
 [testenv:pep8]
 deps = flake8
        argparse
+       hacking
 commands = flake8
 
 [testenv:sdist]