This is quite a lengthy review for a lengthy merge request. I tried to be as pedantic as possible, so you're going to see a LOT of me repeating myself. Overall the code looks pretty good from a style perspective although I haven't even started testing the code for functionality, so this review is all about style. Top issues: 1) Newlines between imports and the copyright header. For some reason no separation here makes me bonkers. I don't think this is a PEP8 requirement, but it seems to be consistent with Nova to have a space here. 2) Importing classes and methods. Having imports only deal with modules makes things easier down the line. ----------------- 96 +# under the License. 97 +import gettext Please separate with an empty line. 112 +gettext.install('melange', unicode=1) In nova, we put this in nova/__init__.py because it was used so frequently. I think it makes sense for melange to do the same. 119 +def create_options(parser): 120 + """ 121 + Sets up the CLI and config-file options that may be 122 + parsed and program commands. 123 + :param parser: The option parser 124 + """ According the HACKING, this docstring should read something along the lines of: def create_options(parser): """Adds needed options to the given parser. :param parser: The option parser to add options to. :returns: None """ 134 + oparser = optparse.OptionParser(version='%%prog VERSION') Should there be a VERSION substitute here? 144 + except RuntimeError, e: As a rule for Python future compatibility, I would suggets this be replaced with: except RuntimeError as error: In general replace all commas in 'except' statements with "as" and try not to use single letter names. 168 +""" 169 + CLI interface for IPAM. 170 +""" 171 +import gettext Space to look like: # under the License. """CLI interface for IPAM.""" import gettext 185 +gettext.install("melange", unicode=1) Same gettext suggestion as earlier. 188 +from melange.common.auth import KeystoneClient 189 +from melange.common.client import HTTPClient 190 +from melange.common.utils import MethodInspector 191 +from melange.ipam.client import (IpBlockClient, SubnetClient, PolicyClient, 192 + UnusableIpOctetsClient, 193 + UnusableIpRangesClient) No importing classes, and only one import per line. 196 +def create_options(parser): 197 + """ 198 + Sets up the CLI and config-file options that may be 199 + parsed and program commands. 200 + 201 + :param parser: The option parser 202 + """ Fix docstring according to HACKING. 237 + """ 238 + Returns the parsed CLI options, command to run and its arguments, merged 239 + with any same-named options found in a configuration file 240 + 241 + :param parser: The option parser 242 + """ Fix docstring according to HACKING. 252 +%prog category action [args] [options]" Not sure if the " should be at the end there. 262 +categories = dict(ip_block=IpBlockClient, subnet=SubnetClient, 263 + policy=PolicyClient, unusable_ip_range=UnusableIpRangesClient, 264 + unusable_ip_octet=UnusableIpOctetsClient) Might be a personal preference, but it follows HACKING and creates a better diff down the line if this is formatted: categories = { "ip_block": IpBlockClient, "subnet": SubnetClient, "policy": PolicyClient, "unusable_ip_range": UnusableIpRangesClient, "unusable_ip_octet": UnusableIpOctetsClient, } 283 + """Get all callable methods of an object that don't start with underscore 284 + returns a list of tuples of the form (method_name, method)""" Fix docstring according to HACKING. 286 + def is_public_method(attr): 287 + return callable(getattr(obj, attr)) and not attr.startswith('_') 288 + 289 + return dict((attr, getattr(obj, attr)) for attr in dir(obj) 290 + if is_public_method(attr)) The fact this has to be written with an 'inner method' shows how complicated it is. IMO it makes more sense this way, but this also might be personal preference: def methods_of(obj): for attr_name in dir(obj): attr = getattr(obj, attr) if callable(attr) and not attr_name.startswith("_"): yield (attr, attr_name) Although now that I look at it the method seems to be returning a dictionary when the docstring says that it should be return a list of tuples. 296 + usage=usage().strip()) Really small but can we return usage.strip() from the usage() method defined above? I'd rather do the work in the method. 328 + print _("Command failed, please check log for more info") 329 + raise Should this always raise or if options.verbose? 355 +# under the License. 356 +import gettext Newline separation. 370 +gettext.install('melange', unicode=1) Same gettext recommendation as before. 374 +from melange.ipam.models import IpBlock No importing classes. 394 + except RuntimeError, e: except RuntimeError as error: 417 +# under the License. 418 +import gettext Newline separation. 433 +gettext.install('melange', unicode=1) Same gettext recommendation as before. 438 +from melange.common.utils import MethodInspector No importing classes. 443 + """ 444 + Sets up the CLI and config-file options that may be 445 + parsed and program commands. 446 + :param parser: The option parser 447 + """ Format docstring according to HACKING. 462 + print self.conf Was this left in to show the user or for debugging? 518 + except TypeError as e: This 'e' isn't being used. So this can be 'except TypeError:' 523 + except Exception as e: This 'e' isn't being used. So this can be 'except Exception:' 534 +from optparse import OptionParser No importing classes. (Event though this was existing.) 734 +# under the License. 735 +import os Newline separation. 790 +# under the License. 791 +import httplib2 Newline separation. 794 +from webob.exc import HTTPForbidden No importing classes. 802 + def __init__(self, application, *auth_providers, **local_config): 803 + self.auth_providers = auth_providers This seems dangerous/potentially confusing for developers. Is this a paste thing? I don't see AuthorizationMiddleware being used explicitly so what is calling this constructor? Ideally I'd think that auth_providers would be passed in as a list, but if this is a Paste thing just ignore me. :) 826 + if('Admin' in roles): We had an issue with "Admin" being lower case in some cases. Not sure what the right way is, but I thought I'd let you know. 841 + if('Admin' in roles): 842 + return True Same as above. 850 +class KeystoneClient(httplib2.Http): Does the Keystone project have it's own HTTP client? Seems silly to have to make one for each project. 854 + self.url = str(url) + "/v2.0/tokens" Might be prudent to use urlparse.urljoin when joining URLs for saftey. 892 +# under the License. 893 +import httplib Newline separation. 938 + except (socket.error, IOError), e: except (socket.error, IOError) as error: Also I feel like the error message might be a little misleading, these errors aren't exclusively generated for "Unable to connect". 964 +""" 965 +Routines for configuring Melange 966 +""" Format to HACKING specifications. 966 +""" 967 +from openstack.common.config import (parse_options, Newline seperation. 967 +from openstack.common.config import (parse_options, 968 + add_log_options, 969 + add_common_options, 970 + load_paste_config, 971 + setup_logging, 972 + load_paste_app, get_option) I'm not sure how you're running this right now as there isn't an official openstack.common package/module. I'm guessing you're just using the openstack-skeleton project? I hope to get the openstack-common initiative on the road soon so. These lines, however, need to import the module, not the individual methods. (so from openstack.common import config) Actually, these lines are not currently used? So this import can actually go away completely? 975 +class Config(object): 976 + 977 + instance = {} 978 + 979 + @classmethod 980 + def get(cls, key, default=None): 981 + return cls.instance.get(key, default) I've made a lot of mistakes in the past doing this sort of thing...class variables can get hairy. Is there a reason Config isn't instansiated somewhere and passed around instead of using it as a 'global'? 1004 +""" 1005 +Nova base exception handling, including decorator for re-raising 1006 +Nova-type exceptions. SHOULD include dedicated exception logging. 1007 +""" Format to HACKING specifications. 1009 +from openstack.common.exception import (Error, ProcessExecutionError, 1010 + DatabaseMigrationError, 1011 + InvalidContentType) See my comments about openstack.common use, importing classes, and these imports don't seem to be used (except for Error). 1022 === added file 'melange/common/extensions.py' Very much so need to get this isn't OpenStack common so this can go away. I am not reviewing this file or others that were grabbed directly from Nova. 1483 +# under the License. 1484 +import urllib Newline separation. 1488 +from melange.common.utils import merge_dicts No importing methods. 1489 +from melange.common.wsgi import Result No importing classes. 1589 +""" 1590 +System-level utilities and helper functions. 1591 +""" Format to HACKING specs. 1600 +from openstack.common.utils import import_class, import_object No importing methods. 1619 + result = None 1620 + if process_input != None: 1621 + result = obj.communicate(process_input) 1622 + else: 1623 + result = obj.communicate() No need to set result = None if it's defined in all code paths. 1667 +def guid(): GUID is (I believe) the Microsoft implementation of UUID, so I'm not sure this applys here, maybe call it generate_uuid() or similar? 1765 +""" 1766 +Utility methods for working with WSGI servers 1767 +""" Format docstring. 1767 +""" 1768 +import datetime Newline separation. 1776 +from webob import Response 1779 +from webob.exc import HTTPBadRequest 1780 +from webob.exc import HTTPError 1781 +from webob.exc import HTTPInternalServerError 1782 +from webob.exc import HTTPNotAcceptable 1783 +from webob.exc import HTTPNotFound 1787 +from openstack.common.wsgi import Router, Server, Middleware 1789 +from melange.common.exception import InvalidContentType 1790 +from melange.common.exception import MelangeError 1791 +from melange.common.utils import cached_property Don't import classes or methods. 1815 + version, Fault(HTTPNotAcceptable(_("version not supported")))) HTTP 406 should be returned when the request is "not acceptable according to the accept headers sent", which doesn't seem to apply here. Maybe HTTPNotFound? 1910 + except MelangeError as e: except MelangeError as error: 1911 + LOG.debug(traceback.format_exc()) LOG.exception should work here. 1914 + except HTTPError as e: except HTTPError as error: 1915 + LOG.debug(traceback.format_exc()) LOG.exception should work here. 1917 + except Exception as e: except Exception as error: 1929 + """ 1930 + {'x':[1,2,3],'y':[4,5,6]} converted to 1931 + {1:'x',2:'x',3:'x',4:'y',5:'y',6:'y'} 1932 + """ Fix to HACKING specs. 1969 + """ 1970 + WSGI app that reads routing information supplied by RoutesMiddleware 1971 + and calls the requested action method upon itself. All action methods 1972 + must, in addition to their normal parameters, accept a 'req' argument 1973 + which is the incoming webob.Request They raise a webob.exc exception, 1974 + or return a dict which will be serialized by requested content type. 1975 + """ Fix to HACKING specs. 1975 + """ 1976 + exception_map = {} Newline separation. 1989 + """ 1990 + Serializes a dictionary to a Content Type specified by a WSGI environment. 1991 + """ Fix to HACKING specs. 1994 + """ 1995 + Create a serializer based on the given WSGI environment. 1996 + 'metadata' is an optional dict mapping MIME types to information 1997 + needed to serialize a dictionary to that type. 1998 + """ Fix to HACKING specs. 2005 + """ 2006 + Serialize a dictionary into a string. The format of the string 2007 + will be decided based on the Content Type requested in self.environ: 2008 + by Accept: header, or by URL suffix. 2009 + """ Fix to HACKING specs. 2061 + """ 2062 + Create a serializer based on the given WSGI environment. 2063 + 'metadata' is an optional dict mapping MIME types to information 2064 + needed to serialize a dictionary to that type. 2065 + """ Fix to HACKING specs. 2165 +# under the License. 2166 +import optparse Newline separation. 2169 +from melange.common.config import Config No importing classes. 2177 + """ 2178 + Adds any configuration options that the db layer might have. 2179 + 2180 + :param parser: An optparse.OptionParser object 2181 + :retval None 2182 + """ Fix to HACKING specs. 2235 +# under the License. 2236 +from sqlalchemy import and_ Newline separation. 2236 +from sqlalchemy import and_ 2237 +from sqlalchemy import or_ 2238 +from sqlalchemy.orm import aliased 2244 +from melange.db.sqlalchemy.mappers import IpNat No importing methods or classes. This might be OK because of SQLAlchemy...this is most likely copied from Nova, so that's OK I suppose. 2430 + if (marker is not None): Unneeded parenthesis. 2453 +# under the License. 2454 +from sqlalchemy import MetaData Newline separation. 2485 +class IpNat(object): 2486 + def __setitem__(self, key, value): Newline separation. 2523 +# template repository default module Unsure what this means. 2597 +""" 2598 +Various conveniences used for migration scripts 2599 +""" Fix to HACKING specs. 2666 +# under the License. 2667 +from sqlalchemy.schema import Column Newline separation. 2719 +# under the License. 2720 +from sqlalchemy.schema import Column Newline separation. 2778 +# under the License. 2779 +from sqlalchemy.schema import Column Newline separation. 2816 +# under the License. 2817 +import datetime 2818 +from sqlalchemy.schema import Column Newline separation between each of these. 2877 +# under the License. 2878 +from sqlalchemy.schema import Column Newline separation. 2919 +# under the License. 2920 +from sqlalchemy.schema import Column Newline separation. This applies to every migration script. 3501 +# template repository default versions module Not sure what this means. 4799 +# under the License. 4800 +import routes Newline separation. 4801 +from webob.exc import HTTPBadRequest 4802 +from webob.exc import HTTPConflict 4803 +from webob.exc import HTTPNotFound 4804 +from webob.exc import HTTPUnprocessableEntity 4805 + 4807 +from melange.common.auth import RoleBasedAuth 4808 +from melange.common.config import Config 4809 +from melange.common.pagination import PaginatedDataView 4810 +from melange.common.utils import exclude 4811 +from melange.common.utils import filter_dict 4812 +from melange.common.utils import stringify_keys 4813 +from melange.common.wsgi import Result 4815 +from melange.ipam.models import IpAddress 4816 +from melange.ipam.models import IpBlock 4817 +from melange.ipam.models import IpOctet 4818 +from melange.ipam.models import IpRange 4819 +from melange.ipam.models import Network 4820 +from melange.ipam.models import Policy No importing classes or methods. 4825 + exception_map = {HTTPUnprocessableEntity: 4826 + [models.NoMoreAddressesError, 4827 + models.AddressDoesNotBelongError, 4828 + models.AddressLockedError], 4829 + HTTPBadRequest: [models.InvalidModelError, 4830 + models.DataMissingError], 4831 + HTTPNotFound: [models.ModelNotFoundError], 4832 + HTTPConflict: [models.DuplicateAddressError]} I find this pretty hard to read, maybe: exception_map = { HTTPUnprocessableEntity: [ models.NoMoreAddressesError, models.AddressDoesNotBelongError, models.AddressLockedError, ], HTTPBadRequest: [ models.InvalidModelError, models.DataMissingError, ], HTTPNotFound: [ models.ModelNotFoundError, ], HTTPConflict: [ models.DuplicateAddressError, ], } The diff has been truncated for viewing. Well crap. I forgot about this until I got here. I'll have to get the full diff, but I think this will hopefully help with getting Melange perfect for release in the future.