Code review comment for lp:~raxnetworking/nova/melange

Revision history for this message
Rick Harris (rconradharris) wrote :

+1 on Jay's comments

Some other minor nits:

> 268 +from gettext import gettext as _

Nova uses this pattern for importing gettext (might be a good idea to match):

    import gettext
    gettext.install('melange', unicode=1)

> 1005 + def do_request(self, method, path, body=None, headers={}, params={}):

Usually not a good idea to use mutable types as default arguments since the
default is shared across invocations which can lead to some very strange bugs.
A better approach is usually to do something like:

    def do_request(self, method, path, body=None, headers=None, params=None):
        headers = headers or {}
        params = params or {}

        # OR

        if not headers:
            headers = {}

> 1384 +def boolean(subject):

Nova already has a pretty well tested boolean to string method
`utils.bool_from_str`. It would probably be a good idea to use that if
possible.

> === added file 'melange/common/utils.py'
>

Pertains to all of melange/common/ really:

Since melange resides in the same source tree as Nova, it would probably be a
good idea to pull as much as this as possible from nova.utils directly (rather than duplicate it).

As mentioned above, an even better solution is to have openstack.common as an external dependency (at some point).

review: Needs Fixing

« Back to merge proposal