Re: [Freeipa-devel] [PATCH] 0048 Rework the CallbackInterface
On Wed, 2012-05-30 at 15:39 +0200, Petr Viktorin wrote: > On 05/15/2012 10:25 AM, Petr Viktorin wrote: > > On 05/10/2012 02:20 PM, Petr Viktorin wrote: > >> While investigating ticket 2674, I found several problems with our > >> implementation of the CallbackInterface — it required complicated > >> calling code, and would subtly break if command classes were > >> instantiated in different ways than they are currently. > >> > >> Here's my fix. See commit message for details. > >> > > > > Rebased to current master > > > > Rebased again. This is certainly an improvement of our callback interface. No issue found, unit tests clean. ACK, pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0048 Rework the CallbackInterface
On 05/15/2012 10:25 AM, Petr Viktorin wrote: On 05/10/2012 02:20 PM, Petr Viktorin wrote: While investigating ticket 2674, I found several problems with our implementation of the CallbackInterface — it required complicated calling code, and would subtly break if command classes were instantiated in different ways than they are currently. Here's my fix. See commit message for details. Rebased to current master Rebased again. -- Petr³ From 59e024220c335be0fa262560dbb7ab2e60420373 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 25 Apr 2012 10:31:10 -0400 Subject: [PATCH] Rework the CallbackInterface Fix several problems with the callback interface: - Automatically registered callbacks (i.e. methods named exc_callback, pre_callback etc) were registered on every instantiation. Fix: Do not register callbacks in __init__; instead return the method when asked for it. - The calling code had to distinguish between bound methods and plain functions by checking the 'im_self' attribute. Fix: Always return the "default" callback as an unbound method. Registered callbacks now always take the extra `self` argument, whether they happen to be bound methods or not. Calling code now always needs to pass the `self` argument. - Did not work well with inheritance: due to the fact that Python looks up missing attributes in superclasses, callbacks could get attached to a superclass if it was instantiated early enough. * Fix: Instead of attribute lookup, use a dictionary with class keys. - The interface included the callback types, which are LDAP-specific. Fix: Create generic register_callback and get_callback mehods, move LDAP-specific code to BaseLDAPCommand Update code that calls the callbacks. Add tests. Remove lint exceptions for CallbackInterface. * https://fedorahosted.org/freeipa/ticket/2674 --- ipalib/cli.py |9 +- ipalib/plugins/baseldap.py| 354 +++-- make-lint |2 - tests/test_xmlrpc/test_baseldap_plugin.py | 95 +++- 4 files changed, 239 insertions(+), 221 deletions(-) diff --git a/ipalib/cli.py b/ipalib/cli.py index 8279345a909ac2e3b764d1f92cfc45ed679b9204..d53e6cd403947ad8cdce1d20cb692db10a0c3dd5 100644 --- a/ipalib/cli.py +++ b/ipalib/cli.py @@ -1195,8 +1195,13 @@ def prompt_interactively(self, cmd, kw): param.label, param.confirm ) -for callback in getattr(cmd, 'INTERACTIVE_PROMPT_CALLBACKS', []): -callback(kw) +try: +callbacks = cmd.get_callbacks('interactive_prompt') +except AttributeError: +pass +else: +for callback in callbacks: +callback(cmd, kw) def load_files(self, cmd, kw): """ diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 93852a2dd8bf4dd89d7cda780898f96b81f648c4..e2e75e104b49e80dcc755aaec2ecbfcdf45a601f 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -690,93 +690,57 @@ def _check_limit_object_class(attributes, attrs, allow_only): if len(limitattrs) > 0 and allow_only: raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=limitattrs[0])) -class CallbackInterface(Method): -""" -Callback registration interface -""" -def __init__(self): -#pylint: disable=E1003 -if not hasattr(self.__class__, 'PRE_CALLBACKS'): -self.__class__.PRE_CALLBACKS = [] -if not hasattr(self.__class__, 'POST_CALLBACKS'): -self.__class__.POST_CALLBACKS = [] -if not hasattr(self.__class__, 'EXC_CALLBACKS'): -self.__class__.EXC_CALLBACKS = [] -if not hasattr(self.__class__, 'INTERACTIVE_PROMPT_CALLBACKS'): -self.__class__.INTERACTIVE_PROMPT_CALLBACKS = [] -if hasattr(self, 'pre_callback'): -self.register_pre_callback(self.pre_callback, True) -if hasattr(self, 'post_callback'): -self.register_post_callback(self.post_callback, True) -if hasattr(self, 'exc_callback'): -self.register_exc_callback(self.exc_callback, True) -if hasattr(self, 'interactive_prompt_callback'): -self.register_interactive_prompt_callback( -self.interactive_prompt_callback, True) #pylint: disable=E1101 -super(Method, self).__init__() -@classmethod -def register_pre_callback(klass, callback, first=False): -assert callable(callback) -if not hasattr(klass, 'PRE_CALLBACKS'): -klass.PRE_CALLBACKS = [] -if first: -klass.PRE_CALLBACKS.insert(0, callback) -else: -klass.PRE_CALLBACKS.append(callback) - -@classmethod -def register_post_callback(klass, callback, first=False): -assert callable(callback) -if not hasattr
Re: [Freeipa-devel] [PATCH] 0048 Rework the CallbackInterface
On 05/10/2012 02:20 PM, Petr Viktorin wrote: While investigating ticket 2674, I found several problems with our implementation of the CallbackInterface — it required complicated calling code, and would subtly break if command classes were instantiated in different ways than they are currently. Here's my fix. See commit message for details. Rebased to current master -- Petr³ From e6e4c0b64cd5c7fc2c80cbfe1c4fa9979b51bc59 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 25 Apr 2012 10:31:10 -0400 Subject: [PATCH] Rework the CallbackInterface Fix several problems with the callback interface: - Automatically registered callbacks (i.e. methods named exc_callback, pre_callback etc) were registered on every instantiation. Fix: Do not register callbacks in __init__; instead return the method when asked for it. - The calling code had to distinguish between bound methods and plain functions by checking the 'im_self' attribute. Fix: Always return the "default" callback as an unbound method. Registered callbacks now always take the extra `self` argument, whether they happen to be bound methods or not. Calling code now always needs to pass the `self` argument. - Did not work well with inheritance: due to the fact that Python looks up missing attributes in superclasses, callbacks could get attached to a superclass if it was instantiated early enough. * Fix: Instead of attribute lookup, use a dictionary with class keys. - The interface included the callback types, which are LDAP-specific. Fix: Create generic register_callback and get_callback mehods, move LDAP-specific code to BaseLDAPCommand Update code that calls the callbacks. Add tests. Remove lint exceptions for CallbackInterface. * https://fedorahosted.org/freeipa/ticket/2674 --- ipalib/cli.py |9 +- ipalib/plugins/baseldap.py| 354 +++-- make-lint |2 - tests/test_xmlrpc/test_baseldap_plugin.py | 95 +++- 4 files changed, 239 insertions(+), 221 deletions(-) diff --git a/ipalib/cli.py b/ipalib/cli.py index 8279345a909ac2e3b764d1f92cfc45ed679b9204..d53e6cd403947ad8cdce1d20cb692db10a0c3dd5 100644 --- a/ipalib/cli.py +++ b/ipalib/cli.py @@ -1195,8 +1195,13 @@ def prompt_interactively(self, cmd, kw): param.label, param.confirm ) -for callback in getattr(cmd, 'INTERACTIVE_PROMPT_CALLBACKS', []): -callback(kw) +try: +callbacks = cmd.get_callbacks('interactive_prompt') +except AttributeError: +pass +else: +for callback in callbacks: +callback(cmd, kw) def load_files(self, cmd, kw): """ diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 2851f0f270d9e2bdba4780cc7bf308a76e180fd2..dd5c1411b2232522061955aa8aa8b7f9ff7f9c16 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -690,93 +690,57 @@ def _check_limit_object_class(attributes, attrs, allow_only): if len(limitattrs) > 0 and allow_only: raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=limitattrs[0])) + class CallbackInterface(Method): -""" -Callback registration interface -""" -def __init__(self): -#pylint: disable=E1003 -if not hasattr(self.__class__, 'PRE_CALLBACKS'): -self.__class__.PRE_CALLBACKS = [] -if not hasattr(self.__class__, 'POST_CALLBACKS'): -self.__class__.POST_CALLBACKS = [] -if not hasattr(self.__class__, 'EXC_CALLBACKS'): -self.__class__.EXC_CALLBACKS = [] -if not hasattr(self.__class__, 'INTERACTIVE_PROMPT_CALLBACKS'): -self.__class__.INTERACTIVE_PROMPT_CALLBACKS = [] -if hasattr(self, 'pre_callback'): -self.register_pre_callback(self.pre_callback, True) -if hasattr(self, 'post_callback'): -self.register_post_callback(self.post_callback, True) -if hasattr(self, 'exc_callback'): -self.register_exc_callback(self.exc_callback, True) -if hasattr(self, 'interactive_prompt_callback'): -self.register_interactive_prompt_callback( -self.interactive_prompt_callback, True) #pylint: disable=E1101 -super(Method, self).__init__() - -@classmethod -def register_pre_callback(klass, callback, first=False): -assert callable(callback) -if not hasattr(klass, 'PRE_CALLBACKS'): -klass.PRE_CALLBACKS = [] -if first: -klass.PRE_CALLBACKS.insert(0, callback) -else: -klass.PRE_CALLBACKS.append(callback) - -@classmethod -def register_post_callback(klass, callback, first=False): -assert callable(callback) -if not hasattr(klass, 'POST_CALLBACKS'): -klass.POST_CALLBACKS =
[Freeipa-devel] [PATCH] 0048 Rework the CallbackInterface
While investigating ticket 2674, I found several problems with our implementation of the CallbackInterface — it required complicated calling code, and would subtly break if command classes were instantiated in different ways than they are currently. Here's my fix. See commit message for details. -- Petr³ From 20036ab7b06f00380670243c49b3959983f39320 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 25 Apr 2012 10:31:10 -0400 Subject: [PATCH] Rework the CallbackInterface Fix several problems with the callback interface: - Automatically registered callbacks (i.e. methods named exc_callback, pre_callback etc) were registered on every instantiation. Fix: Do not register callbacks in __init__; instead return the method when asked for it. - The calling code had to distinguish between bound methods and plain functions by checking the 'im_self' attribute. Fix: Always return the "default" callback as an unbound method. Registered callbacks now always take the extra `self` argument, whether they happen to be bound methods or not. Calling code now always needs to pass the `self` argument. - Did not work well with inheritance: due to the fact that Python looks up missing attributes in superclasses, callbacks could get attached to a superclass if it was instantiated early enough. * Fix: Instead of attribute lookup, use a dictionary with class keys. - The interface included the callback types, which are LDAP-specific. Fix: Create generic register_callback and get_callback mehods, move LDAP-specific code to BaseLDAPCommand Update code that calls the callbacks. Add tests. Remove lint exceptions for CallbackInterface. * https://fedorahosted.org/freeipa/ticket/2674 --- ipalib/cli.py |9 +- ipalib/plugins/baseldap.py| 354 +++-- make-lint |2 - tests/test_xmlrpc/test_baseldap_plugin.py | 95 +++- 4 files changed, 239 insertions(+), 221 deletions(-) diff --git a/ipalib/cli.py b/ipalib/cli.py index 8279345a909ac2e3b764d1f92cfc45ed679b9204..d53e6cd403947ad8cdce1d20cb692db10a0c3dd5 100644 --- a/ipalib/cli.py +++ b/ipalib/cli.py @@ -1195,8 +1195,13 @@ def prompt_interactively(self, cmd, kw): param.label, param.confirm ) -for callback in getattr(cmd, 'INTERACTIVE_PROMPT_CALLBACKS', []): -callback(kw) +try: +callbacks = cmd.get_callbacks('interactive_prompt') +except AttributeError: +pass +else: +for callback in callbacks: +callback(cmd, kw) def load_files(self, cmd, kw): """ diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 85a81723175f38f10c711530971f173a54f1150a..3e1c51357eadb7b6a3f3d7fa900e97ee45267c1a 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -679,93 +679,57 @@ def _check_limit_object_class(attributes, attrs, allow_only): if len(limitattrs) > 0 and allow_only: raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=limitattrs[0])) + class CallbackInterface(Method): -""" -Callback registration interface -""" -def __init__(self): -#pylint: disable=E1003 -if not hasattr(self.__class__, 'PRE_CALLBACKS'): -self.__class__.PRE_CALLBACKS = [] -if not hasattr(self.__class__, 'POST_CALLBACKS'): -self.__class__.POST_CALLBACKS = [] -if not hasattr(self.__class__, 'EXC_CALLBACKS'): -self.__class__.EXC_CALLBACKS = [] -if not hasattr(self.__class__, 'INTERACTIVE_PROMPT_CALLBACKS'): -self.__class__.INTERACTIVE_PROMPT_CALLBACKS = [] -if hasattr(self, 'pre_callback'): -self.register_pre_callback(self.pre_callback, True) -if hasattr(self, 'post_callback'): -self.register_post_callback(self.post_callback, True) -if hasattr(self, 'exc_callback'): -self.register_exc_callback(self.exc_callback, True) -if hasattr(self, 'interactive_prompt_callback'): -self.register_interactive_prompt_callback( -self.interactive_prompt_callback, True) #pylint: disable=E1101 -super(Method, self).__init__() - -@classmethod -def register_pre_callback(klass, callback, first=False): -assert callable(callback) -if not hasattr(klass, 'PRE_CALLBACKS'): -klass.PRE_CALLBACKS = [] -if first: -klass.PRE_CALLBACKS.insert(0, callback) -else: -klass.PRE_CALLBACKS.append(callback) - -@classmethod -def register_post_callback(klass, callback, first=False): -assert callable(callback) -if not hasattr(klass, 'POST_CALLBACKS'): -klass.POST_CALLBACKS = [] -if first: -klass.POST_CALLBACKS.insert(0, callba