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

[Martin Kosek]

On Wed, 2012-04-25 at 10:41 +0200, Petr Viktorin wrote:
> 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
> 

Pushed to master.

Martin