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

[Zhou Zheng Sheng]

on 2013/12/24 02:41, 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):

Hi, it seems the instance argument is redundant here. While we call
"obj.method(a, b, c)", the "self" is passed to "def method(self, a, b,
c)" as the first argument implicitly. Here "instance" and "self" serves
the same purposes and refer to the same thing. So I think, "instance" in
the function argument list can be removed, and change all "instance" in
the function body to "self". This method should be called like following.
  self.start = self.generate_action_handler('start')
This is how object works in Python.

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


-- 
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397