Re: [Kimchi-devel] [PATCH 01/16] Move generate_action_handler() function to Resource() class

On 12/24/2013 10:26 AM, Sheldon wrote: > On 12/24/2013 02:41 AM, Aline Manera wrote: >> From: Aline Manera <alinefm(a)br.ibm.com&gt; >> >> generate_action_handler() function is only used by Resources >> instances so move >> it to Resource() >> >> Signed-off-by: Aline Manera <alinefm(a)br.ibm.com&gt; >> --- >> src/kimchi/controller.py | 75 >> +++++++++++++++++++++++----------------------- >> 1 file changed, 37 insertions(+), 38 deletions(-) >> >> diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py >> index 2940278..ba8b53d 100644 >> --- a/src/kimchi/controller.py >> +++ b/src/kimchi/controller.py >> @@ -103,33 +103,6 @@ def validate_params(params, instance, action): >> e.message for e in validator.iter_errors(request))) >> >> >> -def generate_action_handler(instance, action_name, action_args=None): >> - def wrapper(*args, **kwargs): >> - validate_method(('POST')) >> - try: >> - model_args = list(instance.model_args) >> - if action_args is not None: >> - model_args.extend(parse_request()[key] for key in >> action_args) >> - fn = getattr(instance.model, model_fn(instance, >> action_name)) >> - fn(*model_args) >> - raise internal_redirect(instance.uri_fmt % >> - tuple(instance.model_args)) >> - except MissingParameter, param: >> - raise cherrypy.HTTPError(400, "Missing parameter: '%s'" >> % param) >> - except InvalidParameter, param: >> - raise cherrypy.HTTPError(400, "Invalid parameter: '%s'" >> % param) >> - except InvalidOperation, msg: >> - raise cherrypy.HTTPError(400, "Invalid operation: '%s'" >> % msg) >> - except OperationFailed, msg: >> - raise cherrypy.HTTPError(500, "Operation Failed: '%s'" >> % msg) >> - except NotFoundError, msg: >> - raise cherrypy.HTTPError(404, "Not found: '%s'" % msg) >> - >> - wrapper.__name__ = action_name >> - wrapper.exposed = True >> - return wrapper >> - >> - >> class Resource(object): >> """ >> A Resource represents a single entity in the API (such as a >> Virtual Machine) >> @@ -153,6 +126,32 @@ class Resource(object): >> self.model_args = (ident,) >> self.update_params = [] >> >> + def generate_action_handler(self, instance, action_name, >> action_args=None): > do we still need instance? Here instance is same with self. > > I think it is OK. It's good to move it to Resource class, but you need remove the redundant parameter 'instance' as Sheldon suggested.

> > + def generate_action_handler(self, action_name, action_args=None): >> + def wrapper(*args, **kwargs): >> + validate_method(('POST')) >> + try: >> + model_args = list(instance.model_args) >> + if action_args is not None: >> + model_args.extend(parse_request()[key] for key >> in action_args) >> + fn = getattr(instance.model, model_fn(instance, >> action_name)) >> + fn(*model_args) >> + raise internal_redirect(instance.uri_fmt % >> + tuple(instance.model_args)) >> + except MissingParameter, param: >> + raise cherrypy.HTTPError(400, "Missing parameter: >> '%s'" % param) >> + except InvalidParameter, param: >> + raise cherrypy.HTTPError(400, "Invalid parameter: >> '%s'" % param) >> + except InvalidOperation, msg: >> + raise cherrypy.HTTPError(400, "Invalid operation: >> '%s'" % msg) >> + except OperationFailed, msg: >> + raise cherrypy.HTTPError(500, "Operation Failed: >> '%s'" % msg) >> + except NotFoundError, msg: >> + raise cherrypy.HTTPError(404, "Not found: '%s'" % msg) >> + >> + wrapper.__name__ = action_name >> + wrapper.exposed = True >> + return wrapper >> + >> def lookup(self): >> try: >> lookup = getattr(self.model, model_fn(self, 'lookup')) >> @@ -369,9 +368,9 @@ class VM(Resource): >> self.update_params = ["name"] >> self.screenshot = VMScreenShot(model, ident) >> self.uri_fmt = '/vms/%s' >> - self.start = generate_action_handler(self, 'start') >> - self.stop = generate_action_handler(self, 'stop') >> - self.connect = generate_action_handler(self, 'connect') >> + self.start = self.generate_action_handler(self, 'start') > then you can call > + self.start = self.generate_action_handler('start') >> + self.stop = self.generate_action_handler(self, 'stop') >> + self.connect = self.generate_action_handler(self, 'connect') >> >> @property >> def data(self): >> @@ -453,8 +452,8 @@ class Network(Resource): >> def __init__(self, model, ident): >> super(Network, self).__init__(model, ident) >> self.uri_fmt = "/networks/%s" >> - self.activate = generate_action_handler(self, 'activate') >> - self.deactivate = generate_action_handler(self, 'deactivate') >> + self.activate = self.generate_action_handler(self, 'activate') >> + self.deactivate = self.generate_action_handler(self, >> 'deactivate') >> >> @property >> def data(self): >> @@ -475,8 +474,8 @@ class StorageVolume(Resource): >> self.info = {} >> self.model_args = [self.pool, self.ident] >> self.uri_fmt = '/storagepools/%s/storagevolumes/%s' >> - self.resize = generate_action_handler(self, 'resize', >> ['size']) >> - self.wipe = generate_action_handler(self, 'wipe') >> + self.resize = self.generate_action_handler(self, 'resize', >> ['size']) >> + self.wipe = self.generate_action_handler(self, 'wipe') >> >> @property >> def data(self): >> @@ -522,8 +521,8 @@ class StoragePool(Resource): >> super(StoragePool, self).__init__(model, ident) >> self.update_params = ["autostart"] >> self.uri_fmt = "/storagepools/%s" >> - self.activate = generate_action_handler(self, 'activate') >> - self.deactivate = generate_action_handler(self, 'deactivate') >> + self.activate = self.generate_action_handler(self, 'activate') >> + self.deactivate = self.generate_action_handler(self, >> 'deactivate') >> >> @property >> def data(self): >> @@ -689,8 +688,8 @@ class Host(Resource): >> self.stats = HostStats(self.model) >> self.stats.exposed = True >> self.uri_fmt = '/host/%s' >> - self.reboot = generate_action_handler(self, 'reboot') >> - self.shutdown = generate_action_handler(self, 'shutdown') >> + self.reboot = self.generate_action_handler(self, 'reboot') >> + self.shutdown = self.generate_action_handler(self, 'shutdown') >> self.partitions = Partitions(self.model) >> self.partitions.exposed = True >> > >