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.
« Back to merge proposal
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(): get(int( item_id) ) find(name= item_id)
> 405 + result = manager.
> 406 + else:
> 407 + result = manager.
One common way of handling this is to just go ahead and cast to int and
handle the possible ValueError (EAFP), like:
try: get(int( item_id) ) find(name= item_id)
result = manager.
except ValueError:
result = manager.
> 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" ) (self, context, instance_id):
> 265 def get_diagnostics
Should that be 'get_diagnostics'? I ask because it's the only one that differs
in that regard.