Code review comment for lp:~sandy-walsh/nova/zones4

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

Nice work, Sandy.

Some really minor nits/suggestions:

> 402 + try:
> 403 + manager = getattr(nova, collection)

The getattr can be outside of the try-block (AFAICT).

> 404 + if isinstance(item_id, int) or item_id.isdigit():
> 405 + result = manager.get(int(item_id))
> 406 + else:
> 407 + result = manager.find(name=item_id)

One common way of handling this is to just go ahead and cast to int and
handle the possible ValueError (EAFP), like:

    try:
        result = manager.get(int(item_id))
    except ValueError:
        result = manager.find(name=item_id)

> 371 +def _wrap_method(function, self):
> 372 + """Wrap method to supply self."""
> 373 + def _wrap(*args, **kwargs):
> 374 + return function(self, *args, **kwargs)
> 375 + return _wrap

For the newly added decorators, it might be worth using @functools.wraps so
that the outer-func inherits the inner-funcs attributes. Can make debugging a
little easier.

> 397 +def _issue_novaclient_command(nova, zone, collection, method_name, \
> 398 + item_id):

Trailing '\' isn't needed.

Could line-up item_id with opening param.

> 264 + @scheduler_api.reroute_compute("diagnostics")
> 265 def get_diagnostics(self, context, instance_id):

Should that be 'get_diagnostics'? I ask because it's the only one that differs
in that regard.

review: Needs Fixing

« Back to merge proposal