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

[Jan Cholasta]

On 23.4.2012 18:47, Petr Viktorin wrote:
> 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.

OK, thanks.

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

Added.

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

Changed the code a bit so that it is more readable.

>
>> + return func(*call_args, **call_kwargs)
>> + except errors.ExecutionError, e:
>> + if len(callbacks) == 0:
>
> Use just `if not callbacks`, as per PEP8.

OK.

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

Fixed.

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

They are not strictly needed, I added them just to be on the safe side.

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

Updated patch attached.

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-76.1-refactor-exc-callback.patch
Type: text/x-patch
Size: 25151 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120424/0a532887/attachment.bin>