Re: [Freeipa-devel] [PATCH] 76 Refactor exc_callback invocation
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 > >>> 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 th
Re: [Freeipa-devel] [PATCH] 76 Refactor exc_callback invocation
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
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
Re: [Freeipa-devel] [PATCH] 76 Refactor exc_callback invocation
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
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
-
Re: [Freeipa-devel] [PATCH] 76 Refactor exc_callback invocation
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
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:
