[Elisa-commits] [MERGE] Capabilities and metadata manager

[Benjamin Kampmann]

>=== added file 'elisa-core/elisa/base_components/capability.py'
>--- elisa-core/elisa/base_components/capability.py     1970-01-01 00:00:00 +0000
>+++ elisa-core/elisa/base_components/capability.py     2008-04-24 15:53:33 +0000
>       [...]
>+class Capability(Component):
>+
>+    """
>       [...]
>+    @ivar provider: the provider this capability belongs to
>+    @type provider: L{elisa.base_components.resource_provider.ResourceProvider}
>+    """
I was thinking about this and came to the conclusion that we don't need to force
this here. Just drop it.



>+    # When writing a new capability, don't forget that the provider needs to be
>+    # set manually after the initialize() method has been called.
>+
>+    def initialize(self):
>+        """
>+        Initialize the component.
>+
>+        This method is called by C{Component.create} to finish the
>+        initialization of a component. Don't forget that a capability needs a
>+        'parent' provider, which needs to be set after calling this method.
>+
>+        @return: a deferred called when a component is fully initialized
>+        @rtype: L{twisted.internet.defer.Deferred}
>+        """
>+        return defer.succeed(self)

I think this whole part is as well very useless as there is already an initialize in
L{elisa.core.component.Component}. Just drop it.

General remark: This looks like an Interface to me. The same does the Frontend as
we have it now and the Controller as it is going to be. They don't contain any logic
or methods that needs to be shared anymore but a docstrings with good documentation.

Maybe we should think about renaming the base_components-part into a 'interfaces'
module (maybe directly putting the classes into this module as I don't see why they
need to be in different modules. Hint to twisted ;) ).


>=== added file 'elisa-core/elisa/base_components/metadata_capability.py'
>--- elisa-core/elisa/base_components/metadata_capability.py    1970-01-01 00:00:00 +0000
>+++ elisa-core/elisa/base_components/metadata_capability.py    2008-04-24 16:12:10 +0000
>       [...]
>+"""
>+Capabilities are additional services advertised by resource providers, such as
>+metadata retrieval or advanced search facilities.
>+"""
Copy-Paste? This should tell me something about THIS capability, or ;) ?

[...]
>+    def able_to_handle(self, model):
>+        """
>+        Test whether the capability is able to handle a metadata request on the
>+        given type of model.
>+
Sounds like you should only check with 'isinstance'. Maybe we can add something like
"and is able to fill in the missing data".


[...]
>+    def get_metadata(self, model):
>+        """
>+        Try to retrieve metadata and populate the model with it.
>+
>+        This method is non-blocking: it returns a deferred that will be fired
>+        when the metadata retrieval is complete (be it a success or a failure).
>+
This comment is not needed as you already say it in the @return statement


>=== modified file 'elisa-core/elisa/core/metadata_manager.py'
>--- elisa-core/elisa/core/metadata_manager.py  2008-04-01 10:21:27 +0000
>+++ elisa-core/elisa/core/metadata_manager.py  2008-04-24 16:12:10 +0000
>@@ -15,127 +15,111 @@
> # for details on that license.
> 
> """
>-Metadata parsing of medias using metadata_provider components
>+Metadata handling via a metadata manager against which resource providers
>+register metadata capabilities.
> """
This is an architecture description of what happens and not needed for the
metadata manager itself. As long as the API is fine the manager could also
work with full blown providers ;) . A short
"Metadata requesting through a simply API" would be enough :)

>+    """
>+    The MetadataManager holds references to all the resource providers that
>+    registered against it claiming that they can provide metadata retrieval
>+    capabilities.
>+    """
I really hope it does not hold the references to resource providers ;) . This
is a very confusing description. I would go the simple way and say that you
can ask this Manager for metadata for a given model and it goes through the
components in a certain way (ordered by rank).

>    def __init__(self):
>        [...]
>+        """
>+        Initialize the metadata manager.
>+        """
>+        Manager.__init__(self)
Useless. Just drop this method.

>+    def register_component(self, capability):
>+        """
>+        Register a new metadata capability.
>+
>+        The internal list of capabilities is always kept sorted by decreasing
>+        rank.
>+
>+        @param capability: an instance of a metadata capability to register
>+        @type capability:
>L{elisa.base_components.metadata_capability.MetadataCapability}
>+        """
For the whole method (also internally):
Even if we call the component in this case a 'Capability' I would stick to the
name "component" everywhere as it is very inconsistence to call
'register_component' with a 'capability' to 'Register a new metadata capability'.


>+        if not self.component_registered(capability):
this is not existing anymore. As you said you did the latest update to rest,
your tests are probably failing now ;)

>+            self.debug("Registering capability %s" % str(capability))
>+            for index in xrange(len(self._components)):
>+                if capability.rank > self._components[index].rank:
>+                    # Insert the capability at the right position in the list
>+                    self._components.insert(index, capability)
>+                    return
>+            # Append the capability to the end of the list, lowest rank
>+            self._components.append(capability)
>+        else:
>+            self.debug("Capability %s already registered" % str(capability))
>+            raise AlreadyRegistered()

You could also have one nested part less by refactoring the method like this:
    
    if component in self._components:
        raise AlreadyRegistered()

    for index ...

>+    def get_metadata(self, request_callback, model):
I would change the order: first the model (as it stays in foreground) and then
callback. The name of it is confusing. I would call it check_callback or
something.

    [...]
>+        @param request_callback: a callback that determines whether the
>+                                 requested metadata has been successfully
>+                                 retrieved
>+        @type request_callback:  a callback function that takes the model as
>+                                 parameter and returns a C{bool}
As this doc is not very clear as well. I would change it to something more
explicit.

    @param  check_callback: gets called after each component.get_metadata was
                            called and has answered. The first (and only)
                            parameter is the model itself. The callback should
                            check whether we can stop processing this model as
                            all desired values are set (by returning True) or
                            the manager should go on processing the model.
    @type check_callback:   callable

>+        @return:                 a deferred fired when the metadata retrieval
>+                                 is complete
>+        @rtype:                  L{elisa.core.utils.defer.Deferred}
>+        """
Idea: should we maybe fire a callback in the case the check_callback tells us
that we can stop and return the model as the result and fire an errback if we
reached the end of the chain without the check_callback telling us that
everything is set?

>+        # Not optimal (we're going twice through the list), but so much easier
>+        # to write...
>+        able_capabilities = [capability for capability in self._components if \
>+                             capability.able_to_handle(model)]
>+        index = -1
>+
>+        def one_capability_tried(get_result, index):
>+            """
>+            This callback is called when one capability has been tried, it is
>+            used to decide whether to stop or continue by hitting the remaining
>+            capabilities.
>+
>+            @param index: the index of the capability that has been tried in
>+                          self._components
>+            @type index:  an unsigned C{int}
>+            """
:)) That is cute. You only need to put doc strings in all *public* methods but
not in nested ones ;). Don't overdoc, dude...

>+            res_dfr = defer.succeed(None)
>+            index += 1
>+            if (not request_callback(model)) and \
>+                (index < len(able_capabilities)):
>+                # Try the next capability able to handle this model
>+                next_capability = able_capabilities[index]
>+                self.debug('Trying capability %d/%d' % \
>+                           (index + 1, len(able_capabilities)))
>+                res_dfr = next_capability.get_metadata(request_callback, model)
>+                res_dfr.addCallbacks(one_capability_tried,
>+                                     one_capability_tried,
>+                                     callbackArgs=(index,),
>+                                     errbackArgs=(index,))
>+            return res_dfr
>+
>+        result_dfr = defer.Deferred()
>+        try_dfr = one_capability_tried(defer.succeed(None), index)
>+        try_dfr.chainDeferred(result_dfr)
>+        return result_dfr

Hm... Maybe you should use a twisted coiterator over here (take a look into
the manager.load_components part). Alessandro is it easily doable to break in
such a coiterator loop and return the model when the check tells us to?

    [...]

>=== modified file 'elisa-core/elisa/core/tests/test_metadata_manager.py'
>--- elisa-core/elisa/core/tests/test_metadata_manager.py       2008-04-01 10:21:27 +0000
>+++ elisa-core/elisa/core/tests/test_metadata_manager.py       2008-04-24 15:53:33 +0000
>
>+from elisa.core.metadata_manager import MetadataManager
>+
>+from twisted.internet import defer
>+
>+class FlexibleMeta(object):
>+    path = "" # remove this as soon as the manager does not need it anymore

The manager got merged. You can remove it now

    [...]

The tests itself look good so far. Could you please add one that shows what
happens the check_callback is not a callable?

As the tests are there and are good my remarks could be done without having
the need for another review:
bb:tweak

benjamin