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

[Petr Viktorin]

On 04/24/2012 05:38 PM, Jan Cholasta wrote:
> 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
>

ACK

-- 
Petr³