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

[Aline Manera]

On 12/24/2013 01:16 AM, Mark Wu wrote:
> On 12/24/2013 10:26 AM, Sheldon wrote:
>> On 12/24/2013 02:41 AM, Aline Manera wrote:
>>> From: Aline Manera <alinefm at br.ibm.com>
>>>
>>> generate_action_handler() function is only used by Resources 
>>> instances so move
>>> it to Resource()
>>>
>>> Signed-off-by: Aline Manera <alinefm at br.ibm.com>
>>> ---
>>>   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.
>

ACK.

>>
>> + 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
>>>
>>
>>
>