Code review comment for lp:~robru/gwibber/discovery

On Sep 06, 2012, at 03:22 AM, Robert Bruce Park wrote:

>Ugh, today was a disaster :-/

Bummer! Today will surely be better.

>1. Despite Base class no longer living in protocols directory,
>inspect.getmembers is still managing to yield it from find_all. My guess
>would be that this is happening because the flickr plugin imports Base into
>it's own namespace, so inspect finds it in that namespace and then yields
>it.

Yep, that's exactly what's happening.

>So the 'value is not Base' test is still necessary, defeating much of what I
>set out to do today. The only way I can see around this is to give up on the
>idea of importing all classes, and just import the class that matches the
>filename (or just live with it as-is, that's probably not so bad either).

Actually, I realize that instead of iterating over all members of the module,
you should just iterate over the names in the module's __all__. That's the
intent of __all__ anyway, to explicitly export the public members of a module.
Of course it means that __all__ has to be kept up-to-date, but I think that's
a good idea anyway.

The loop will still have to do the subclass check, but it won't need to check
for non-Base-y-ness.

>2. My idea that the ProtocolManager would actually instantiate all plugins in
>it's __init__ method is another utter failure, because the plugins need
>accounts in order to be instantiated, and the PluginManager doesn't have any
>access to the accounts in order to actually do that.

We discussed this a bit in IRC. I definitely need to read up on libaccounts,
but I think we've decided that it's possible to have multiple instances of the
protocol classes with different account information. E.g. you might have two
Twitter accounts. So yeah, ProtocolManager can only find and record the
protocol classes, though depending on the outcome of future designs, it could
possibly provide an API for more convenient instantiation of the classes. I
think we're agreed that there need be only one global "account manager" per
gwibber-service.

We'll cross that bridge later though.

>So the best we're going to get from this is a dict mapping basenames to
>classes (not instances), which I am about to implement. It's going to break
>your elegant generator, but unfortunately that is necessary because the other
>bits of gwibber are going to need to do 'ProtocolManager["flickr"]' and not
>'for p in ProtocolManager.find_all(): if str(p) != "flickr": continue' or
>some other heinous mess.

I'm also not psyched about having ProtocolManager be a subclass of dict. In
general, I don't like code that does that just to provide a getitem API to
clients, but the class itself isn't intended to be used everywhere a dict is
to be used. It seems like an inappropriate use of inheritance. We use it in
the Account class (which I still don't like, but oh well), but I don't think
we should use that for ProtocolManager.

Or to put it in OOP parlance: a ProtocolManager *has a* dict, but not *is a*
dict. It's generally frowned upon to use inheritance for *has a*
relationships. I'd like to rewrite that for the merge.

One other thing I've changed. The protocol classes themselves have a 'name'
attribute and I think that's a safer key to use in the dictionary than the
class name. Thus I've rewritten it to use cls.name.lower() as the key into
the dictionary.

>Anyway, please review & merge this, and then we can discuss how to proceed
>with regards to the dispatcher interacting with plugins. I'm thinking that
>the Account class should contain a reference to the instance of the protocol
>that it belongs to, but in particular we need to figure out what the lifetime
>and scope of protocol instances is going be. Eg, can we design it so that the
>instance that represents each acount is persistent in memory (similar to my
>@memoize magic), or is it ok to have multiple different instances of the
>protocol class representing a given account (eg, if two different threads
>happen to instantiate their own Flickr plugin for the same account, is that
>ok, or is that going to conflict with each other). I guess what I'm really
>trying to say is that we need to audit this code for thread safety.

Let's come back to this question. It may require a rethink about the basic
design of Account and the Base subclasses. But for now, as we discussed on
IRC, we're going to keep the ProtocolManager class strictly focused on finding
and exporting the protocol classes.

A review of the code follows.

=== modified file 'gwibber/gwibber/protocols/flickr.py'
--- gwibber/gwibber/protocols/flickr.py 2012-09-05 22:09:44 +0000
+++ gwibber/gwibber/protocols/flickr.py 2012-09-06 15:18:56 +0000
> @@ -23,11 +23,11 @@
> import logging
>
> from gwibber.errors import AuthorizationError
> -from gwibber.protocols.base import Base
> from gwibber.utils.authentication import Authentication
> from gwibber.utils.download import get_json
> from gwibber.utils.results import Results
> from gwibber.utils.time import parsetime
> +from gwibber.utils.protocol import Base

FWIW, these from-imports should be alphabetized. The style guide I work from,
which has been formed from many years of discussion with other Pythonistas is
here:

http://tinyurl.com/bmua4ok

=== modified file 'gwibber/gwibber/tests/test_protocols.py'
--- gwibber/gwibber/tests/test_protocols.py 2012-09-05 22:09:44 +0000
+++ gwibber/gwibber/tests/test_protocols.py 2012-09-06 15:18:56 +0000
> @@ -25,12 +25,12 @@
> import unittest
> import threading
>
> -from gwibber.errors import AuthorizationError
> -from gwibber.protocols.base import Base
> +from gwibber.testing.mocks import FakeData
> from gwibber.protocols.flickr import Flickr
> -from gwibber.testing.mocks import FakeData
> from gwibber.utils.logging import initialize
> -from gwibber.utils.modules import find_all_protocols
> +from gwibber.errors import AuthorizationError
> +from gwibber.utils.protocol import Base, ProtocolManager
> +

And here, but I've fixed both sorts in the merge.

>
> try:
> # Python 3.3
> @@ -43,20 +43,16 @@
> """Test the protocol finder."""
>

Probably best to rename the test class to TestProtocolManager. I also think
we might as well add a setUp() that creates the manager instance.

> def test_find_all_protocols(self):
> - # Search for and return all of the discovered classes implementing the
> - # known protocols.
> - flickr_class = None
> - for protocol_class in find_all_protocols():
> - if protocol_class.name == 'Flickr':
> - flickr_class = protocol_class
> - break
> - else:
> - raise AssertionError('Flickr protocol class not found')
> - self.assertEqual(flickr_class, Flickr)
> + # ProtocolManager knows them all.
> + manager = ProtocolManager()
> + self.assertTrue('flickr' in manager)

Python 3's unittest.TestCase is *so* much nicer than Python 2.6's (and to some
extent even 2.7's). E.g. better to use self.assertIn() here.

> + self.assertEqual(manager['flickr'], Flickr)
>
> def test_doesnt_find_base(self):
> # Make sure that the base protocol class isn't returned.
> - for protocol_class in find_all_protocols():
> + manager = ProtocolManager()
> + self.assertFalse('base' in manager)

We can use assertNotIn() here.

> + for protocol_class in manager.values():
> self.assertNotEqual(protocol_class, Base)
> self.assertIsNotNone(protocol_class.name)

=== renamed file 'gwibber/gwibber/protocols/base.py' => 'gwibber/gwibber/utils/protocol.py'
--- gwibber/gwibber/protocols/base.py 2012-09-05 14:18:12 +0000
+++ gwibber/gwibber/utils/protocol.py 2012-09-06 15:18:56 +0000
> @@ -16,9 +16,16 @@
>
> __all__ = [
> 'Base',
> + 'ProtocolManager',
> ]
>
>
> +import os
> +import inspect
> +import importlib
> +
> +from pkg_resources import resource_listdir
> +
> from threading import Thread
>
>
> @@ -89,3 +96,19 @@
> def _locked_login(self, old_token):
> """Login operation stub. This must be implemented by subclasses."""
> raise NotImplementedError
> +
> +
> +class ProtocolManager(dict):
> + """Discover and instantiate all protocols."""
> + def __init__(self):

Style nit: single blank line after the class docstring.

> + for filename in resource_listdir('gwibber', 'protocols'):
> + basename, extension = os.path.splitext(filename)
> + if extension != '.py':
> + continue
> + module_path = 'gwibber.protocols.' + basename
> + module = importlib.import_module(module_path)
> + # Now, look at all the names in the just imported module. Return all
> + # class objects that inherit from the Base protocol class.
> + self.update((name.lower(), obj) for (name, obj)
> + in inspect.getmembers(module, inspect.isclass)
> + if issubclass(obj, Base) and obj is not Base)

So yeah, I think giving ProtocolManager a .protocols attribute which is the
dictionary mapping protocol names to their classes, is a better way to go.
I've also split discovery off into a separate method (actually, property for
the heck of it), and I've rewritten it to only consult the module's __all__
attribute. It's a bit more verbose because it's better off written as a
traditional for-loop, but I think it's more correct.