diff --git a/HACKING b/HACKING deleted file mode 100644 index 77c4ee0f..00000000 --- a/HACKING +++ /dev/null @@ -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. diff --git a/HACKING.rst b/HACKING.rst new file mode 100644 index 00000000..fc48d8d3 --- /dev/null +++ b/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 diff --git a/MANIFEST.in b/MANIFEST.in index 9e45182e..cfb5483b 100644 --- a/MANIFEST.in +++ b/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 diff --git a/git-review b/git-review index 8dcb58ae..ff2b9deb 100755 --- a/git-review +++ b/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()) - if latest_version > StrictVersion(version): + latest_version = du_version.StrictVersion(version_file.read()) + 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) diff --git a/setup.py b/setup.py index 8cdbad91..2b1de917 100755 --- a/setup.py +++ b/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.install import install as du_install -from setuptools.command.install import install +from distutils.command import install as du_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, diff --git a/tox.ini b/tox.ini index d1ec544c..f1160e70 100644 --- a/tox.ini +++ b/tox.ini @@ -7,6 +7,7 @@ setenv = VIRTUAL_ENV={envdir} [testenv:pep8] deps = flake8 argparse + hacking commands = flake8 [testenv:sdist]