Merge into py3 : discovery : Code : Gwibber

Status:	Merged
Merged at revision:	1416
Proposed branch:	lp:~robru/gwibber/discovery
Merge into:	lp:~barry/gwibber/py3
Diff against target:	154 lines (+35/-59) 4 files modified gwibber/gwibber/protocols/flickr.py (+1/-1) gwibber/gwibber/tests/test_protocols.py (+11/-15) gwibber/gwibber/utils/modules.py (+0/-43) gwibber/gwibber/utils/protocol.py (+23/-0)
To merge this branch:	bzr merge lp:~robru/gwibber/discovery
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Barry Warsaw		2012-09-06	Pending
Review via email: mp+122995@code.launchpad.net

Description of the change

Ugh, today was a disaster :-/

So what I've done here is merged find_all_protocols and Base protocol class into gwibber.utils.protocol. I've also done the bare minimum to convert find_all_protocols into ProtocolManager.find_all. I ran into a number of seemingly intractable problems:

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. 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).

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. 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.

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.

Thanks!

lp:~robru/gwibber/discovery updated on 2012-09-06

1419. By Robert Bruce Park on 2012-09-06: Make ProtocolManager a dict mapping protocol names to their classes.

Revision history for this message

Robert Bruce Park (robru) wrote on 2012-09-06:

#

Barry, I can't figure out why the tests aren't passing here, it works successfully when I access it from an interactive python session. Something about the unittest environment is breaking something else that's going on, but I can't figure out what because I haven't changed much from your code that did work, the biggest change is that it's executing the code immediately instead of as a generator.

So sorry for mp'ing broken code, hopefully there's something obvious that I've overlooked because I'm exhausted. Please have a look!

lp:~robru/gwibber/discovery updated on 2012-09-06

1420. By Robert Bruce Park on 2012-09-06: Drop @singleton decorator to make tests pass.

Revision history for this message

Barry Warsaw (barry) wrote on 2012-09-06:

#

Download full text (9.3 KiB)

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'
attribut...

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.

Gwibber

Merge lp:~robru/gwibber/discovery into lp:~barry/gwibber/py3

Commit message

Description of the change

Preview Diff

Subscribers

 === 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:16:41 +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
  log = logging.getLogger('gwibber.service')
 === 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:16:41 +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
++
  try:
      # Python 3.3
@@ -43,20 +43,16 @@
      """Test the protocol finder."""
      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)
++        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)
++        for protocol_class in manager.values():
              self.assertNotEqual(protocol_class, Base)
              self.assertIsNotNone(protocol_class.name)
 === removed file 'gwibber/gwibber/utils/modules.py'
 --- gwibber/gwibber/utils/modules.py	2012-09-05 19:22:46 +0000
 +++ gwibber/gwibber/utils/modules.py	1970-01-01 00:00:00 +0000
@@ -1,43 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""Utilities for finding things in modules."""
--
--__all__ = [
--    'find_all_protocols',
--    ]
--
--
--import os
--import inspect
--import importlib
--
--from pkg_resources import resource_listdir
--
--from gwibber.protocols.base import Base
--
--
--def find_all_protocols():
--    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.  Be sure
--        # not to return the base protocol class itself!
--        for name, value in inspect.getmembers(module, inspect.isclass):
--            if issubclass(value, Base) and value is not Base:
--                yield value
 === 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:16:41 +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):
++        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)