[Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())

[Petr Viktorin]

On 07/30/2013 08:05 PM, Nathaniel McCallum wrote:
> On Tue, 2013-07-30 at 18:58 +0200, Petr Viktorin wrote:
>> On 07/30/2013 05:16 PM, Nathaniel McCallum wrote:
>>>  From 371e080fb6a86da6f0ec574c8cbc0d8d06c18c99 Mon Sep 17 00:00:00 2001
>>> From: Nathaniel McCallum<npmccallum at redhat.com>
>>> Date: Tue, 30 Jul 2013 11:13:55 -0400
>>> Subject: [PATCH] Return klass in api.register()
>>>
>>> This permits the use of api.register as a decorator.
>>
>> Thanks for the patch.
>> Was there any discussion/reason for this? Can you file a ticket?
>> I'm not opposed, I agree this would allow less typing for plugin authors.
>
> There was not really any discussion. I just simply noticed that a single
> line change would make plugins a bit more pythonic. I mentioned it to
> rcrit and he didn't see any problem with it. So I wrote the patch.
>
> Expressing registration as a decorator has numerous advantages,
> including being less error prone and easier to read. But typing less is
> probably a good enough reason. :)
>
> In my mind, this can be merged now since it doesn't break any backwards
> compatibility and enables a syntax that is easier to modify in the
> future. It is the migration from a function to a decorator that is where
> all the work lies. Once you have decorators, you can fairly easily do a
> sed replacement (or maintain backwards compatibility if you like).

The patch is backwards compatible, but the new syntax is not forward 
compatible. You can't easily run sed on third-party plugins -- we want 
to switch just once.
We should switch to supporting both the old and new syntax. An 
intermediate syntax that's better than the old one but not compatible 
with future plans isn't worth it.

I can make a patch that adds my proposed syntax but keeps the old 
behavior. I'd like to get the design done first though.
(And of course, the old syntax and functionality would still be supported)


>> I also think the plugin registration API needs a larger redesign, though
>> I haven't shared my thoughts on it lately as there are more pressing
>> things to do. I'll correct that now that there's (sort of) a thread on
>> the topic :)
>>
>> Basically, my gripe is that plugin registration only works on the one
>> global API object, ipalib.api. If you create a different API object, you
>> can't register plugins on it. Different API object would be useful for
>> testing, or for running IPA commands on another master (there's an ugly
>> hack in ipa-replica-install that does that).
>>
>> A rough sketch of what I had in mind would be:
>>
>> plugin.py:
>>       # the "register" decorator will collect registered items;
>>       # it must be named "register" so the loading routine finds it
>>       register = plugable.Registry()
>>
>>       @register
>>       class obj_mod(LDAPUpdate):
>>           ....
>>
>> plugable.py:
>>       class Registry:
>>           def __call__(self, item):
>>               self.items.append(item)
>>
>>       class API:
>>           def load_plugin_package(self, package_name):
>>               plugin = __import__(package_name).register
>>               for item in register.items:
>>                   self.register(item)
>>
>> Related ticket for this: https://fedorahosted.org/freeipa/ticket/3090
>>
>>
>> Maybe if we're giving an incentive for plugin authors to rewrite
>> plugins, we should give them an API that's at least compatible with the
>> above goals, so they won't have to rewrite again some time later.
>
> Having a registration collector as an independent object makes a lot of
> sense. I also prefer type-safety to strings.
>
> Make sure you make the decorator a callable though. This permits you to
> easily add options later without breaking backwards compatibility.
>
> You want:
> @register()
> class foo:
>    pass
>
> Not:
> @register
> class foo:
>    pass

Fair point.

So we're at:
     from ipalib.plugable import Registry

     register = Registry()

     @register()
     class object_mod(...)
         ...

> Also, you can avoid the creation of a registry object for each plugin.

But that is exactly what I want to do! I *want* a plugin module to just 
have a list of things it exports, and leave it to the consumer to load 
them if it wants to.

> You can essentially do module cache poisoning to force an alternative
> registry. This saves some typing and keeps the alternate code in the
> same scope as the tests for that alternate path...

Too much magic. I would really rather have things work as they normally 
do Python. Module cache poisoning is pretty tough on the reader.
The extra line that you have to type keeps things clear and tells the 
reader where to look to see how things are done.

I don't understand what exactly you mean by "alternate code" here?

> Lastly, you could use class hierarchy introspection to avoid
> registration altogether. This has the advantage of some natural type
> safety.

Did you mean to look which classes in a module are Plugin subclasses? 
That would mean all plugins now have to be defined as top-level items in 
a module -- an unnecessary restriction. Doing anything else than a 
traditional plugin module would get unnecessarily complicated (e.g. 
creating plugins dynamically -- who knows what we'll need in the future?).
Type safety is enforced by the API already.

Or did you mean something like looking at Plugin.__subclasses__()? 
Assuming that all classes whose module happened to be loaded up to now 
need to be included in the API is extremely fragile. (This is another 
reason why the current registration sucks.)


Let's just keep things explicit, please.


-- 
Petr³