[Freeipa-devel] [PATCH] 76 Refactor exc_callback invocation

[Petr Viktorin]

On 04/23/2012 04:33 PM, Jan Cholasta wrote:
> Hi,
>
> this patch replaces _call_exc_callbacks with a function wrapper, which
> will automatically call exception callbacks when an exception is raised
> from the function. This removes the need to specify the function and its
> arguments twice (once in the function call itself and once in
> _call_exc_callbacks).
>
> Code like this:
>
> try:
>      # original call
>      ret = func(arg, kwarg=0)
> except ExecutionError, e:
>      try:
>          # the function and its arguments need to be specified again!
>          ret = self._call_exc_callbacks(args, options, e, func, arg,
> kwarg=0)
>      except ExecutionErrorSubclass, e:
>          handle_error(e)
>
> becomes this:
>
> try:
>      ret = self._exc_wrapper(args, options, func)(arg, kwarg=0)
> except ExecutionErrorSubclass, e:
>      handle_error(e)
>
> As you can see, the resulting code is shorter and you don't have to
> remember to make changes to the arguments in two places.
>
> Honza

Please add a test, too. I've attached one you can use.

See also some style nitpicks below.

> --
> Jan Cholasta
>
> freeipa-jcholast-76-refactor-exc-callback.patch
>
>
>> From 8e070f571472ed5a27339bcc980b67ecca41b337 Mon Sep 17 00:00:00 2001
> From: Jan Cholasta<jcholast at redhat.com>
> Date: Thu, 19 Apr 2012 08:06:32 -0400
> Subject: [PATCH] Refactor exc_callback invocation.
>
> Replace _call_exc_callbacks with a function wrapper, which will automatically
> call exception callbacks when an exception is raised from the function. This
> removes the need to specify the function and its arguments twice (once in the
> function call itself and once in _call_exc_callbacks).
> ---
>   ipalib/plugins/baseldap.py   |  227 ++++++++++++++----------------------------
>   ipalib/plugins/entitle.py    |   19 ++--
>   ipalib/plugins/group.py      |    7 +-
>   ipalib/plugins/permission.py |   27 +++---
>   ipalib/plugins/pwpolicy.py   |   11 +-
>   5 files changed, 109 insertions(+), 182 deletions(-)
>
> diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
> index f185977..f7a3bbc 100644
> --- a/ipalib/plugins/baseldap.py
> +++ b/ipalib/plugins/baseldap.py
> @@ -744,26 +744,24 @@ class CallbackInterface(Method):
>           else:
>               klass.INTERACTIVE_PROMPT_CALLBACKS.append(callback)
>
> -    def _call_exc_callbacks(self, args, options, exc, call_func, *call_args, **call_kwargs):
> -        rv = None
> -        for i in xrange(len(getattr(self, 'EXC_CALLBACKS', []))):
> -            callback = self.EXC_CALLBACKS[i]
> -            try:
> -                if hasattr(callback, 'im_self'):
> -                    rv = callback(
> -                        args, options, exc, call_func, *call_args, **call_kwargs
> -                    )
> -                else:
> -                    rv = callback(
> -                        self, args, options, exc, call_func, *call_args,
> -                        **call_kwargs
> -                    )
> -            except errors.ExecutionError, e:
> -                if (i + 1)<  len(self.EXC_CALLBACKS):
> -                    exc = e
> -                    continue
> -                raise e
> -        return rv
> +    def _exc_wrapper(self, keys, options, call_func):

Consider adding a docstring, e.g.
"""Function wrapper that automatically calls exception callbacks"""

> +        def wrapped(*call_args, **call_kwargs):
> +            func = call_func
> +            callbacks = list(getattr(self, 'EXC_CALLBACKS', []))
> +            while True:
> +                try:

You have some clever code here, rebinding `func` like you do.
It'd be nice if there was a comment warning that you're redefining a 
function, in case someone who's not a Python expert looks at this.
Consider:
# `func` is either the original function, or the current error callback

> +                    return func(*call_args, **call_kwargs)
> +                except errors.ExecutionError, e:
> +                    if len(callbacks) == 0:

Use just `if not callbacks`, as per PEP8.

> +                        raise
> +                    callback = callbacks.pop(0)
> +                    if hasattr(callback, 'im_self'):
> +                        def func(*args, **kwargs):  #pylint: disable=E0102
> +                            return callback(keys, options, e, call_func, *args, **kwargs)
> +                    else:
> +                        def func(*args, **kwargs):  #pylint: disable=E0102
> +                            return callback(self, keys, options, e, call_func, *args, **kwargs)
> +        return wrapped
>
>
>   class BaseLDAPCommand(CallbackInterface, Command):
[...]
> diff --git a/ipalib/plugins/entitle.py b/ipalib/plugins/entitle.py
> index 28d2c5d..6ade854 100644
> --- a/ipalib/plugins/entitle.py
> +++ b/ipalib/plugins/entitle.py
> @@ -642,12 +642,12 @@ class entitle_import(LDAPUpdate):
>           If we are adding the first entry there are no updates so EmptyModlist
>           will get thrown. Ignore it.
>           """
> -        if isinstance(exc, errors.EmptyModlist):
> -            if not getattr(context, 'entitle_import', False):
> -                raise exc
> -            return (call_args, {})
> -        else:
> -            raise exc
> +        if call_func.func_name == 'update_entry':
> +            if isinstance(exc, errors.EmptyModlist):
> +                if not getattr(context, 'entitle_import', False):

You didn't mention the additional checks for 'update_entry' in the 
commit message.

By the way, the need for these checks suggests that a per-class registry 
of error callbacks might not be the best design. But that's for more 
long-term thinking.

> +                    raise exc
> +                return (call_args, {})
> +        raise exc
>
>       def execute(self, *keys, **options):
>           super(entitle_import, self).execute(*keys, **options)
[...]


-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_baseldap_plugin.py
Type: text/x-python
Size: 2161 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120423/ab941cf6/attachment.py>