Gwibber

Merge lp:~robru/gwibber/account-manager into lp:~barry/gwibber/py3

account-manager
Merge into py3

Proposed by Robert Bruce Park on 2012-09-07

Status:	Merged
Merged at revision:	1422
Proposed branch:	lp:~robru/gwibber/account-manager
Merge into:	lp:~barry/gwibber/py3
Diff against target:	221 lines (+118/-22) 3 files modified gwibber/Makefile (+10/-0) gwibber/gwibber/tests/test_account.py (+53/-17) gwibber/gwibber/utils/account.py (+55/-5)
To merge this branch:	bzr merge lp:~robru/gwibber/account-manager
Related bugs:	Link a bug report

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

Description of the change

This branch adds AccountManager class in account.py, with code largely stolen directly from the old dispatcher. Some basic tests are added that cover its interaction with libaccounts and also with our streams.

Note that the AccountManager is not actually used anywhere yet, the intention is that it will be instantiated by the Dispatcher once that's in place. The reason for this is that it needs a callback into the Dispatcher's refresh() method in order to add new streams when new accounts have been added by libaccounts.

And just for fun, I've introduced a Makefile that makes it easier to invoke the testsuite, with or without virtualenv.

Revision history for this message

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

Download full text (14.5 KiB)

On Sep 07, 2012, at 07:16 PM, Robert Bruce Park wrote:

>This branch adds AccountManager class in account.py, with code largely stolen
>directly from the old dispatcher. Some basic tests are added that cover its
>interaction with libaccounts and also with our streams.

Nice.

>Note that the AccountManager is not actually used anywhere yet, the intention
>is that it will be instantiated by the Dispatcher once that's in place. The
>reason for this is that it needs a callback into the Dispatcher's refresh()
>method in order to add new streams when new accounts have been added by
>libaccounts.

Sounds good.

>And just for fun, I've introduced a Makefile that makes it easier to invoke
>the testsuite, with or without virtualenv.

Very convenient!

Okay, code review coming up. Just in general, I added a bunch of comments
throughout based on my understanding of the changes. Once your branch is
merged please take a look to make sure I'm not mistaken.

=== added file 'gwibber/Makefile'
--- gwibber/Makefile 1970-01-01 00:00:00 +0000
+++ gwibber/Makefile 2012-09-07 19:15:22 +0000
> @@ -0,0 +1,10 @@
> +check:
> + python3 -m unittest discover -vv
> +
> +install:
> + python3 setup.py install
> +
> +check_all:
> + virtualenv --system-site-packages -p python3 /tmp/gwib
> + /tmp/gwib/bin/python3 setup.py install
> + /tmp/gwib/bin/python3 -m unittest discover -vv

This is great. I'm just going to add a standard copyright notice to the top
of the file.

=== modified file 'gwibber/gwibber/tests/test_account.py'
--- gwibber/gwibber/tests/test_account.py 2012-09-06 20:48:07 +0000
+++ gwibber/gwibber/tests/test_account.py 2012-09-07 19:15:22 +0000
> @@ -21,9 +21,12 @@
>
> import unittest
>
> +from gi.repository import Accounts
> +

Turns out this import isn't used, so I've removed it. Have you seen the
pyflakes tool? It's a nice little tool that finds some common mistakes in
Python code. It's not as thorough as others like pylint, but also doesn't
have the vast number of false positives that pylint can produce by default
(and which are a pita to suppress). Of course pylint integrates nicely with
Emacs so when it complains, it paints the line with a red background so it's
easy to see. It also displays the number of problems it finds in the
modeline.

For me, pyflakes strikes a nice balance for a static Python checker that gives
you useful warnings without bombarding you with false positives. A lot of
folks also like the pep8 tool, but since I co-authored that PEP, I tend to
have it's rules engraved in my brain, as you've no doubt unfortunately
observed in my curmudgeonly reviews. :)

> from gwibber.errors import UnsupportedProtocolError
> from gwibber.protocols.flickr import Flickr
> -from gwibber.utils.account import Account
> +from gwibber.utils.account import Account, AccountManager
> +
>
> try:
> # Python 3.3
> @@ -32,6 +35,30 @@
> import mock
>
>
> +accounts_manager = mock.Mock()
> +accounts_manager.new_for_service_type(
> + 'microblogging').get_enabled_account_services.return_value = []
> +
> +
> +class FakeAccount(dict):
> + global_id = 'some fake id'
> + def __init__(self, service):
> + self['id'] = 'faker/tha...

On Sep 07, 2012, at 07:16 PM, Robert Bruce Park wrote:

Nice.

Sounds good.

>And just for fun, I've introduced a Makefile that makes it easier to invoke
>the testsuite, with or without virtualenv.

Very convenient!

Okay, code review coming up.  Just in general, I added a bunch of comments
throughout based on my understanding of the changes.  Once your branch is
merged please take a look to make sure I'm not mistaken.

=== added file 'gwibber/Makefile'
--- gwibber/Makefile	1970-01-01 00:00:00 +0000
+++ gwibber/Makefile	2012-09-07 19:15:22 +0000
> @@ -0,0 +1,10 @@
> +check:
> +	python3 -m unittest discover -vv
> +
> +install:
> +	python3 setup.py install
> +
> +check_all:
> +	virtualenv --system-site-packages -p python3 /tmp/gwib
> +	/tmp/gwib/bin/python3 setup.py install
> +	/tmp/gwib/bin/python3 -m unittest discover -vv

This is great.  I'm just going to add a standard copyright notice to the top
of the file.

=== modified file 'gwibber/gwibber/tests/test_account.py'
--- gwibber/gwibber/tests/test_account.py	2012-09-06 20:48:07 +0000
+++ gwibber/gwibber/tests/test_account.py	2012-09-07 19:15:22 +0000
> @@ -21,9 +21,12 @@
>  
>  import unittest
>  
> +from gi.repository import Accounts
> +

Turns out this import isn't used, so I've removed it.  Have you seen the
pyflakes tool?  It's a nice little tool that finds some common mistakes in
Python code.  It's not as thorough as others like pylint, but also doesn't
have the vast number of false positives that pylint can produce by default
(and which are a pita to suppress).  Of course pylint integrates nicely with
Emacs so when it complains, it paints the line with a red background so it's
easy to see.  It also displays the number of problems it finds in the
modeline.

For me, pyflakes strikes a nice balance for a static Python checker that gives
you useful warnings without bombarding you with false positives.  A lot of
folks also like the pep8 tool, but since I co-authored that PEP, I tend to
have it's rules engraved in my brain, as you've no doubt unfortunately
observed in my curmudgeonly reviews. :)

>  from gwibber.errors import UnsupportedProtocolError
>  from gwibber.protocols.flickr import Flickr
> -from gwibber.utils.account import Account
> +from gwibber.utils.account import Account, AccountManager
> +
>  
>  try:
>      # Python 3.3
> @@ -32,6 +35,30 @@
>      import mock
>  
>  
> +accounts_manager = mock.Mock()
> +accounts_manager.new_for_service_type(
> +    'microblogging').get_enabled_account_services.return_value = []
> +
> +
> +class FakeAccount(dict):
> +    global_id = 'some fake id'
> +    def __init__(self, service):
> +        self['id'] = 'faker/than fake'

As it turns out, I just added a testing helper for fake accounts, so I rolled
this change into there.

> +
> +
> +class SettingsIterMock:
> +    """Mimick the weird libaccounts get_settings_iter."""
> +
> +    def __init__(self):
> +        self.items = [(True, 'key', 'value')]
> +
> +    def next(self):
> +        if self.items:
> +            return self.items.pop()
> +        else:
> +            return (False, 'dog', 'four')

Yes, this looks like a much better model of AccountSettingsIter, except for
that as I observe it, when the iterator is exhausted, (False, None, None) is
actually returned.  Not that it matters since the second and third items will
be ignored, but see some commentary below.

I like this, so I moved it to the helpers module.

> +
> +
>  class TestAccount(unittest.TestCase):
>      """Test Account class."""
>  
> @@ -54,9 +81,7 @@
>                  'get_parameters.return_value': 'fake parameters',
>                  }),
>              'get_account.return_value': mock.Mock(**{
> -                'get_settings_iter.return_value': [
> -                    (True, 'key', 'value'),
> -                    ],
> +                'get_settings_iter.return_value': SettingsIterMock(),
>                  'id': 'fake_id',
>                  'get_provider_name.return_value': 'flickr',
>                  }),
> @@ -111,20 +136,14 @@
>          # received, the callback is called and the account is updated.
>          #
>          # Start by simulating a change in the account service.
> -        iter = self.account_service.get_account().get_settings_iter
> -        iter.return_value = [
> +        other_iter = SettingsIterMock()
> +        other_iter.items = [
>              (True, 'ant', 'one'),
>              (True, 'bee', 'two'),
>              (True, 'cat', 'three'),
> -            # The semantics of get_settings_iter() is that it will stop
> -            # returning values on the first False in item[0] it encounters.
> -            # Since the mock doesn't completely mimic this behavior, all we
> -            # can do is test that False values don't make it into the
> -            # account.  IOW, it should be impossible for True values to follow
> -            # False ones, but that's behavior in get_settings_iter() and we
> -            # can't really (nor should we) test that.
> -            (False, 'dog', 'four'),
>              ]
> +        iter = self.account_service.get_account().get_settings_iter
> +        iter.return_value = other_iter
>          # Check that the signal has been connected.
>          self.assertEqual(self._callback_signal, 'changed')
>          # Check that the account is the object we expect it to be.
> @@ -155,9 +174,7 @@
>          # other mock service has to at least support the basic required API.
>          other = Account(mock.Mock(**{
>              'get_account.return_value': mock.Mock(**{
> -                'get_settings_iter.return_value': [
> -                    (True, 'key', 'value'),
> -                    ],
> +                'get_settings_iter.return_value': SettingsIterMock(),
>                      # It's okay if the provider names are the same; the test
>                      # is for whether the account services are the same or not,
>                      # and in this test, they'll be different mock instances.
> @@ -165,3 +182,22 @@
>                  }),
>              }))
>          self.assertNotEqual(self.account, other)
> +
> +    @mock.patch('gi.repository.Accounts.Manager', accounts_manager)
> +    @mock.patch('gwibber.utils.account.Account', FakeAccount)
> +    def test_account_manager(self):

I wonder if it make sense to move this into a separate class, though still in
test_accounts.py?  I don't think this test uses any of the mocks created in
the setUp().

If I create a TestAccountManager class and move the mock decorators up to it,
everything still works.

I think I'll also split this test into smaller individual tests.  Yeah, I
know, I know... ;)

> +        mock_stream = mock.Mock()
> +        mock_stream.List.return_value = \
> +          '[{"account":"faker/than fake","id":"foo"}]'

I changed this to actually do the json.dumps() since that would be easier to
change or cargo-cult in the future.  Side benefit: no backslash. :)

> +        account_service = mock.Mock()
> +
> +        manager = AccountManager(lambda:12345, mock_stream)
> +        self.assertEqual(manager.refresh(), 12345)
> +
> +        manager.add_new_account(account_service)
> +        self.assertIn('some fake id', manager.accounts)
> +
> +        manager.on_account_deleted(accounts_manager, 'some fake id')
> +        self.assertNotIn('some fake id', manager.accounts)
> +        mock_stream.List.assert_called_once_with()
> +        mock_stream.Delete.assert_called_once_with('foo')

=== modified file 'gwibber/gwibber/utils/account.py'
--- gwibber/gwibber/utils/account.py	2012-09-06 20:48:07 +0000
+++ gwibber/gwibber/utils/account.py	2012-09-07 19:15:22 +0000
> @@ -16,18 +16,63 @@
>  
>  __all__ = [
>      'Account',
> +    'AccountManager',
>      ]
>  
>  
> +import json
> +import logging
> +log = logging.getLogger('gwibber.accounts')
> +
>  from threading import Lock
>  
> +from gi.repository import Accounts
> +
>  from gwibber.errors import UnsupportedProtocolError
> -# TODO later on we'll get the global ProtocolManager instance from the
> -# Dispatcher when it exists, but for now we need to import it here.
>  from gwibber.utils.protocol import ProtocolManager
> +

Looks good; I'm just going to do some rearranging of lines to match my
mysterious but pedantic style. ;)

>  _manager = ProtocolManager()
>  
>  
> +class AccountManager:
> +    """Manage the accounts that we know about."""
> +
> +    def __init__(self, refresh_callback, streams):
> +        self.refresh = refresh_callback
> +        self.streams = streams
> +        self.accounts = {}

It seems to me that all three of these attributes can be non-public
(i.e. prefixed with a single underscore).  I don't think any of them must be
accessed from outside the class, so I'll make that change when I merge it.

I see that self.accounts is accessed by the test, but I think it's fine if
that uses a non-public attribute.  (Generally, tests can be a bit more
voyeuristic in peeking into a class's non-public API.)

I think it reads better if self.refresh is self._callback.

I also see that self.refresh is called from test_account_manager() but I'm not
sure that tests anything useful.  Basically, it's testing something that's
obvious: calling a function should return whatever that function returns!
I added some additional checks that the callback is getting called at the
right times; let me know what you think.

> +
> +        manager = Accounts.Manager.new_for_service_type('microblogging')
> +        manager.connect('enabled-event', self.on_enabled_event)
> +        manager.connect('account-deleted', self.on_account_deleted)

I also think these two callback methods don't need to be public either, so
I'll change them to single-underscore-prefixed names.

> +        for account_service in manager.get_enabled_account_services():
> +            self.add_new_account(account_service)
> +            log.info('Found {} accounts'.format(len(self.accounts)))

Don't you want that log message to happen after the loop?  Otherwise, let's
say you add 3 accounts.  You'd see the following log messages:

Found 1 accounts
Found 2 accounts
Found 3 accounts

I've moved it to outside the loop and slightly changed the message to be
grammatically correct even when there's just one account added.

> +
> +    def on_enabled_event(self, manager, account_id):
> +        account = manager.get_account(account_id)
> +        for service in account.list_services():
> +            account_service = Accounts.AccountService.new(account, service)
> +            if account_service.get_enabled():
> +                self.add_new_account(account_service)
> +                self.refresh()

Do you really want the refresh callback to potentially get called every time
through the loop, for each enabled account service?  That seems excessive; if
we find 10 enabled account services, refresh will get called 10 times.  The
callback doesn't get any clue as to what service got added, so wouldn't it
make more sense to call refresh just once after the loop completes?  Even if
no enabled services were found, I still think you should call the refresh once
(hopefully it can't hurt).

I'll move that to outside the loop, but let me know if you disagree.

Also, for symmetry, I added a debug log call.

> +
> +    def on_account_deleted(self, manager, account_id):
> +        log.debug('Deleting account ' + account_id)

I changed this to:

log.debug('Deleting account {}', account_id)

> +        account = self.accounts.pop(account_id)

It's possible that account_id has never been seen before, in which case a
KeyError will get raised.  I ended up catching that exception, logging, and
returning.

> +        try:
> +            for stream in json.loads(self.streams.List()):
> +                if stream['account'] == account['id']:
> +                    self.streams.Delete(stream['id'])
> +        except:
> +            # TODO Identify what actually gets raised so we can catch
> +            # it properly.
> +            raise

Hmm, this makes me uncomfortable.  It also seems unhelpful since it's
effectively the same as not catching the exception at all.  It might be
different if you logged the exception or something, but I think it's just as
well to let any exception propagate, and then evaluate how to react to them
once you've seen one in the wild.

Also, shouldn't the refresh callback get called when an account is deleted?  I
added that, but let me know if you disagree.

> +
> +    def add_new_account(self, account_service):
> +        new_account = Account(account_service)
> +        self.accounts[new_account.global_id] = new_account
> +
>  class Account(dict):
>      """A thin wrapper around libaccounts API."""
>  
> @@ -60,9 +105,14 @@
>  
>      def on_account_changed(self, account_service, account):
>          settings = account.get_settings_iter('gwibber/')
> -        self.update((key, value)
> -                    for (success, key, value) in settings
> -                    if success)
> +        # This is a little bit ugly, but necessary until we get around
> +        # to making the libaccounts API a little bit nicer.
> +        while True:
> +            success, key, value = settings.next()
> +            if success:
> +                self[key] = value
> +            else:
> +                break

Yep, it's a bummer that AccountSettingIter doesn't have an override that
produces an actual Python iterable.  But we can have some fun with Python to
make it work like an iterator.  The trick is 2-arg built-in iter() and
observing that g_s_i returns (False, None, None) when it's exhausted.  So we
can actually just change one line of the original to:

settings = iter(account.get_settings_iter('gwibber/'),
                        # Sentinel - return when iterator is exhausted.
                        (False, None, None))

One problem that I can see is that afaict, the second and third items appear
to be undocumented when the first is False, so we're crossing our fingers
about the sentinel.

Is it worth it?  Is it more readable?  Probably not, so I'll keep your change.
But let's keep that in mind when we submit a proposal to improve the
override. :)

>  
>      def enabled(self):
>          return self.account_service.get_enabled()

Revision history for this message

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

Download full text (13.0 KiB)

On 12-09-07 04:52 PM, Barry Warsaw wrote:
>> +check_all:
>> + virtualenv --system-site-packages -p python3 /tmp/gwib
>> + /tmp/gwib/bin/python3 setup.py install
>> + /tmp/gwib/bin/python3 -m unittest discover -vv
>
> This is great. I'm just going to add a standard copyright notice to the top
> of the file.

I'm glad you like it. I got sick of grepping through #gwibber logs to
try and figure out how to re-start the venv every day ;-)

>> +from gi.repository import Accounts
>> +
>
> Turns out this import isn't used, so I've removed it.

Oh, oops. I thought it was necessary in order for mock.patch to patch
over it.

> Have you seen the
> pyflakes tool? It's a nice little tool that finds some common mistakes in
> Python code. It's not as thorough as others like pylint, but also doesn't
> have the vast number of false positives that pylint can produce by default
> (and which are a pita to suppress). Of course pylint integrates nicely with
> Emacs so when it complains, it paints the line with a red background so it's
> easy to see. It also displays the number of problems it finds in the
> modeline.

Never heard of pyflakes, but I have used pylint on past projects. I know
all too well what you mean about suppressing pylint false positives:

https://github.com/robru/gottengeography/blob/gexiv2/delinter.sh

;-)

> For me, pyflakes strikes a nice balance for a static Python checker that gives
> you useful warnings without bombarding you with false positives. A lot of
> folks also like the pep8 tool, but since I co-authored that PEP, I tend to
> have it's rules engraved in my brain, as you've no doubt unfortunately
> observed in my curmudgeonly reviews. :)

Whoa, I actually was not aware that you co-authored PEP8. I had no idea
I was working with such a prominent figure in the Python community. I'm
honored! ;-)

>> +class FakeAccount(dict):
>> + global_id = 'some fake id'
>> + def __init__(self, service):
>> + self['id'] = 'faker/than fake'
>
> As it turns out, I just added a testing helper for fake accounts, so I rolled
> this change into there.

Fair. This one was necessary because, as it turns out, mock doesn't
support subscripting syntax at all. I was quite surprised, considering
how magical it tends to be with everything else. you'd think they could
have written a __setitem__ that was just as magical, but I guess not.
Who knows.

>> +class SettingsIterMock:
>> + """Mimick the weird libaccounts get_settings_iter."""
>> +
>> + def __init__(self):
>> + self.items = [(True, 'key', 'value')]
>> +
>> + def next(self):
>> + if self.items:
>> + return self.items.pop()
>> + else:
>> + return (False, 'dog', 'four')
>
> Yes, this looks like a much better model of AccountSettingsIter, except for
> that as I observe it, when the iterator is exhausted, (False, None, None) is
> actually returned. Not that it matters since the second and third items will
> be ignored, but see some commentary below.

Yes, it is (False, None, None), I just copied the 'dog', 'four' stuff in
keeping with the theme of the test that you'd written.

>> + @mock.patch('gi.repository.Accounts.Manager', acc...

On 12-09-07 04:52 PM, Barry Warsaw wrote:
>> +check_all:
>> +	virtualenv --system-site-packages -p python3 /tmp/gwib
>> +	/tmp/gwib/bin/python3 setup.py install
>> +	/tmp/gwib/bin/python3 -m unittest discover -vv
>
> This is great.  I'm just going to add a standard copyright notice to the top
> of the file.

I'm glad you like it. I got sick of grepping through #gwibber logs to 
try and figure out how to re-start the venv every day ;-)

>> +from gi.repository import Accounts
>> +
>
> Turns out this import isn't used, so I've removed it.

Oh, oops. I thought it was necessary in order for mock.patch to patch 
over it.

>  Have you seen the
> pyflakes tool?  It's a nice little tool that finds some common mistakes in
> Python code.  It's not as thorough as others like pylint, but also doesn't
> have the vast number of false positives that pylint can produce by default
> (and which are a pita to suppress).  Of course pylint integrates nicely with
> Emacs so when it complains, it paints the line with a red background so it's
> easy to see.  It also displays the number of problems it finds in the
> modeline.

Never heard of pyflakes, but I have used pylint on past projects. I know 
all too well what you mean about suppressing pylint false positives:

https://github.com/robru/gottengeography/blob/gexiv2/delinter.sh

;-)

> For me, pyflakes strikes a nice balance for a static Python checker that gives
> you useful warnings without bombarding you with false positives.  A lot of
> folks also like the pep8 tool, but since I co-authored that PEP, I tend to
> have it's rules engraved in my brain, as you've no doubt unfortunately
> observed in my curmudgeonly reviews. :)

Whoa, I actually was not aware that you co-authored PEP8. I had no idea 
I was working with such a prominent figure in the Python community. I'm 
honored! ;-)

>> +class FakeAccount(dict):
>> +    global_id = 'some fake id'
>> +    def __init__(self, service):
>> +        self['id'] = 'faker/than fake'
>
> As it turns out, I just added a testing helper for fake accounts, so I rolled
> this change into there.

Fair. This one was necessary because, as it turns out, mock doesn't 
support subscripting syntax at all. I was quite surprised, considering 
how magical it tends to be with everything else. you'd think they could 
have written a __setitem__ that was just as magical, but I guess not. 
Who knows.

>> +class SettingsIterMock:
>> +    """Mimick the weird libaccounts get_settings_iter."""
>> +
>> +    def __init__(self):
>> +        self.items = [(True, 'key', 'value')]
>> +
>> +    def next(self):
>> +        if self.items:
>> +            return self.items.pop()
>> +        else:
>> +            return (False, 'dog', 'four')
>
> Yes, this looks like a much better model of AccountSettingsIter, except for
> that as I observe it, when the iterator is exhausted, (False, None, None) is
> actually returned.  Not that it matters since the second and third items will
> be ignored, but see some commentary below.

Yes, it is (False, None, None), I just copied the 'dog', 'four' stuff in 
keeping with the theme of the test that you'd written.

>> +    @mock.patch('gi.repository.Accounts.Manager', accounts_manager)
>> +    @mock.patch('gwibber.utils.account.Account', FakeAccount)
>> +    def test_account_manager(self):
>
> I wonder if it make sense to move this into a separate class, though still in
> test_accounts.py?  I don't think this test uses any of the mocks created in
> the setUp().
>
> If I create a TestAccountManager class and move the mock decorators up to it,
> everything still works.
>
> I think I'll also split this test into smaller individual tests.  Yeah, I
> know, I know... ;)

At some point I just have to wonder if you're inflating the test 
numbers... it's as if your boss is going to run 'make check' and then 
pay you a large fixed amount per test that is reported ;-)

>> +        mock_stream = mock.Mock()
>> +        mock_stream.List.return_value = \
>> +          '[{"account":"faker/than fake","id":"foo"}]'
>
> I changed this to actually do the json.dumps() since that would be easier to
> change or cargo-cult in the future.  Side benefit: no backslash. :)

Bonus ;-)

>> +class AccountManager:
>> +    """Manage the accounts that we know about."""
>> +
>> +    def __init__(self, refresh_callback, streams):
>> +        self.refresh = refresh_callback
>> +        self.streams = streams
>> +        self.accounts = {}
>
> It seems to me that all three of these attributes can be non-public
> (i.e. prefixed with a single underscore).  I don't think any of them must be
> accessed from outside the class, so I'll make that change when I merge it.

NoooooOOOOOOOOOOOOOOOooooooooooooooooooooo! accounts dict is actually 
accessed externally, particularly accounts.get(), so please either 
re-publicize accounts, or give me something like 'self.get_account_by_id 
= self._accounts.get'

Actually the dispatcher is going to be iterating over accounts, 
remember? Because it uses account.protocol to actually access protocol 
methods. So it needs accounts dict public.

> I think it reads better if self.refresh is self._callback.

Fine. I just liked how 'self.refresh = refresh_callback' lined up so 
well with the method signature on the previous line. Usually when I see 
two lines have some content that lines up so perfectly, that's a sign 
that something is working well ;-)

> I also see that self.refresh is called from test_account_manager() but I'm not
> sure that tests anything useful.  Basically, it's testing something that's
> obvious: calling a function should return whatever that function returns!
> I added some additional checks that the callback is getting called at the
> right times; let me know what you think.

I'll look in a minute.

>> +        manager = Accounts.Manager.new_for_service_type('microblogging')
>> +        manager.connect('enabled-event', self.on_enabled_event)
>> +        manager.connect('account-deleted', self.on_account_deleted)
>
> I also think these two callback methods don't need to be public either, so
> I'll change them to single-underscore-prefixed names.

Yeah. I don't usually worry about whether something needs to be "public" 
or "private" because I find the underscore quite ugly. Only if the class 
is so huge that there are lots of methods does it start to make sense 
(to me) to talk about what's public and what's private. And also Python 
plays the privacy so loosy-goosy that I barely find it worth 
considering. Now if we were programming in Java I might care more about 
what was public and what was private ;-)

>> +        for account_service in manager.get_enabled_account_services():
>> +            self.add_new_account(account_service)
>> +            log.info('Found {} accounts'.format(len(self.accounts)))
>
> Don't you want that log message to happen after the loop?  Otherwise, let's
> say you add 3 accounts.  You'd see the following log messages:
>
> Found 1 accounts
> Found 2 accounts
> Found 3 accounts

Oh, duh. That's my underslept grogginess shining through today ;-)

> I've moved it to outside the loop and slightly changed the message to be
> grammatically correct even when there's just one account added.

Ok.

>> +    def on_enabled_event(self, manager, account_id):
>> +        account = manager.get_account(account_id)
>> +        for service in account.list_services():
>> +            account_service = Accounts.AccountService.new(account, service)
>> +            if account_service.get_enabled():
>> +                self.add_new_account(account_service)
>> +                self.refresh()
>
> Do you really want the refresh callback to potentially get called every time
> through the loop, for each enabled account service?  That seems excessive; if
> we find 10 enabled account services, refresh will get called 10 times.  The
> callback doesn't get any clue as to what service got added, so wouldn't it
> make more sense to call refresh just once after the loop completes?  Even if
> no enabled services were found, I still think you should call the refresh once
> (hopefully it can't hurt).
>
> I'll move that to outside the loop, but let me know if you disagree.

Nope, you're right. I'm totally asleep at the keyboard today.

>> +        account = self.accounts.pop(account_id)
>
> It's possible that account_id has never been seen before, in which case a
> KeyError will get raised.  I ended up catching that exception, logging, and
> returning.

Ok.

>> +        try:
>> +            for stream in json.loads(self.streams.List()):
>> +                if stream['account'] == account['id']:
>> +                    self.streams.Delete(stream['id'])
>> +        except:
>> +            # TODO Identify what actually gets raised so we can catch
>> +            # it properly.
>> +            raise
>
> Hmm, this makes me uncomfortable.  It also seems unhelpful since it's
> effectively the same as not catching the exception at all.  It might be
> different if you logged the exception or something, but I think it's just as
> well to let any exception propagate, and then evaluate how to react to them
> once you've seen one in the wild.

Yeah, this is one of those ugly things from the original dispatcher that 
I had no idea what to do with. The old dispatcher silently ignored the 
exception with a 'pass' and that *always* terrifies me, so I changed it 
to raise so that we'd have some idea of what the hell is going on. Like 
I said on IRC last week, people who write "try: foo(); except: pass" 
need to get run over by a bus.

> Also, shouldn't the refresh callback get called when an account is deleted?  I
> added that, but let me know if you disagree.

I was assuming that streams.Delete handled that, since the old 
dispatcher didn't do a refresh upon deletion.

>>       def on_account_changed(self, account_service, account):
>>           settings = account.get_settings_iter('gwibber/')
>> -        self.update((key, value)
>> -                    for (success, key, value) in settings
>> -                    if success)
>> +        # This is a little bit ugly, but necessary until we get around
>> +        # to making the libaccounts API a little bit nicer.
>> +        while True:
>> +            success, key, value = settings.next()
>> +            if success:
>> +                self[key] = value
>> +            else:
>> +                break
>
> Yep, it's a bummer that AccountSettingIter doesn't have an override that
> produces an actual Python iterable.

Before you get all down on me for dropping the sexy generator passed to 
dict.update, keep in mind that the *old* dispatcher accomplished this 
like so:

def on_account_changed(self, account_service, account):
     iterator = account.get_settings_iter(None)
     settings = []
     (ok, key, value) = iterator.next()
     while ok:
       settings.append((key, value))
       (ok, key, value) = iterator.next()

settings.sort()
     for (key, value) in settings:
       if key.startswith("gwibber"):
         self._dict[key.split("/")[1]] = value

So my "while True" loop is still a huge win in eliminating dain-bramage 
from the dispatcher.

> But we can have some fun with Python to
> make it work like an iterator.  The trick is 2-arg built-in iter() and
> observing that g_s_i returns (False, None, None) when it's exhausted.  So we
> can actually just change one line of the original to:
>
>          settings = iter(account.get_settings_iter('gwibber/'),
>                          # Sentinel - return when iterator is exhausted.
>                          (False, None, None))

I was not aware of this feature of iter(). The documentation[1] does not 
make it clear to me that this will actually help us, though.

[1] http://docs.python.org/py3k/library/functions.html#iter

That page says, "The iterator created in this case will call _object_ 
with no arguments for each call to its __next__() method;" I'm having 
some difficulty parsing this sentence, but I am interpreting it like so:

while True:
     object.__next__()
     result = object()
     if result == sentinel:
         break

That really doesn't do us any good, because object in our case isn't 
callable, but even worse, it doesn't have a __next__() method either, 
only next() (because this is C ugliness poking through the introspection 
layer, with no overrides to sex it up).

> Is it worth it?  Is it more readable?  Probably not, so I'll keep your change.
> But let's keep that in mind when we submit a proposal to improve the
> override. :)

Yes, for now we need to just keep it my way because that actually works 
with a live libaccounts session in the wild. Once we reassemble the 
pieces of gwibber with some integration tests, then we can look at 
hassling the libaccounts people to do a proper override (hell, I'll even 
write it, I just don't have time right now).

>>       def enabled(self):
>>           return self.account_service.get_enabled()

Lol, we should just change this bit to 'self.enabled = 
self.account_service.get_enabled' in the __init__ method. That would 
save us a whole layer of method invocation for nothing...

Alright barry, have a good weekend, I'll catch up with you on monday, 
maybe you'll even see an mp for the dispatcher.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barry Warsaw

Robert Bruce Park

 === added file 'gwibber/Makefile'
 --- gwibber/Makefile	1970-01-01 00:00:00 +0000
 +++ gwibber/Makefile	2012-09-07 19:15:22 +0000
@@ -0,0 +1,10 @@
++check:
++	python3 -m unittest discover -vv
++
++install:
++	python3 setup.py install
++
++check_all:
++	virtualenv --system-site-packages -p python3 /tmp/gwib
++	/tmp/gwib/bin/python3 setup.py install
++	/tmp/gwib/bin/python3 -m unittest discover -vv
 === modified file 'gwibber/gwibber/tests/test_account.py'
 --- gwibber/gwibber/tests/test_account.py	2012-09-06 20:48:07 +0000
 +++ gwibber/gwibber/tests/test_account.py	2012-09-07 19:15:22 +0000
@@ -21,9 +21,12 @@
  import unittest
++from gi.repository import Accounts
++
  from gwibber.errors import UnsupportedProtocolError
  from gwibber.protocols.flickr import Flickr
--from gwibber.utils.account import Account
++from gwibber.utils.account import Account, AccountManager
++
  try:
      # Python 3.3
@@ -32,6 +35,30 @@
      import mock
++accounts_manager = mock.Mock()
++accounts_manager.new_for_service_type(
++    'microblogging').get_enabled_account_services.return_value = []
++
++
++class FakeAccount(dict):
++    global_id = 'some fake id'
++    def __init__(self, service):
++        self['id'] = 'faker/than fake'
++
++
++class SettingsIterMock:
++    """Mimick the weird libaccounts get_settings_iter."""
++
++    def __init__(self):
++        self.items = [(True, 'key', 'value')]
++
++    def next(self):
++        if self.items:
++            return self.items.pop()
++        else:
++            return (False, 'dog', 'four')
++
++
  class TestAccount(unittest.TestCase):
      """Test Account class."""
@@ -54,9 +81,7 @@
                  'get_parameters.return_value': 'fake parameters',
                  }),
              'get_account.return_value': mock.Mock(**{
--                'get_settings_iter.return_value': [
--                    (True, 'key', 'value'),
--                    ],
++                'get_settings_iter.return_value': SettingsIterMock(),
                  'id': 'fake_id',
                  'get_provider_name.return_value': 'flickr',
                  }),
@@ -111,20 +136,14 @@
          # received, the callback is called and the account is updated.
+         #
          # Start by simulating a change in the account service.
--        iter = self.account_service.get_account().get_settings_iter
--        iter.return_value = [
++        other_iter = SettingsIterMock()
++        other_iter.items = [
              (True, 'ant', 'one'),
              (True, 'bee', 'two'),
              (True, 'cat', 'three'),
--            # The semantics of get_settings_iter() is that it will stop
--            # returning values on the first False in item[0] it encounters.
--            # Since the mock doesn't completely mimic this behavior, all we
--            # can do is test that False values don't make it into the
--            # account.  IOW, it should be impossible for True values to follow
--            # False ones, but that's behavior in get_settings_iter() and we
--            # can't really (nor should we) test that.
--            (False, 'dog', 'four'),
+             ]
++        iter = self.account_service.get_account().get_settings_iter
++        iter.return_value = other_iter
          # Check that the signal has been connected.
          self.assertEqual(self._callback_signal, 'changed')
          # Check that the account is the object we expect it to be.
@@ -155,9 +174,7 @@
          # other mock service has to at least support the basic required API.
          other = Account(mock.Mock(**{
              'get_account.return_value': mock.Mock(**{
--                'get_settings_iter.return_value': [
--                    (True, 'key', 'value'),
--                    ],
++                'get_settings_iter.return_value': SettingsIterMock(),
                      # It's okay if the provider names are the same; the test
                      # is for whether the account services are the same or not,
                      # and in this test, they'll be different mock instances.
@@ -165,3 +182,22 @@
                  }),
              }))
          self.assertNotEqual(self.account, other)
++
++    @mock.patch('gi.repository.Accounts.Manager', accounts_manager)
++    @mock.patch('gwibber.utils.account.Account', FakeAccount)
++    def test_account_manager(self):
++        mock_stream = mock.Mock()
++        mock_stream.List.return_value = \
++          '[{"account":"faker/than fake","id":"foo"}]'
++        account_service = mock.Mock()
++
++        manager = AccountManager(lambda:12345, mock_stream)
++        self.assertEqual(manager.refresh(), 12345)
++
++        manager.add_new_account(account_service)
++        self.assertIn('some fake id', manager.accounts)
++
++        manager.on_account_deleted(accounts_manager, 'some fake id')
++        self.assertNotIn('some fake id', manager.accounts)
++        mock_stream.List.assert_called_once_with()
++        mock_stream.Delete.assert_called_once_with('foo')
 === modified file 'gwibber/gwibber/utils/account.py'
 --- gwibber/gwibber/utils/account.py	2012-09-06 20:48:07 +0000
 +++ gwibber/gwibber/utils/account.py	2012-09-07 19:15:22 +0000
@@ -16,18 +16,63 @@
  __all__ = [
      'Account',
++    'AccountManager',
+     ]
++import json
++import logging
++log = logging.getLogger('gwibber.accounts')
++
  from threading import Lock
++from gi.repository import Accounts
++
  from gwibber.errors import UnsupportedProtocolError
--# TODO later on we'll get the global ProtocolManager instance from the
--# Dispatcher when it exists, but for now we need to import it here.
  from gwibber.utils.protocol import ProtocolManager
++
  _manager = ProtocolManager()
++class AccountManager:
++    """Manage the accounts that we know about."""
++
++    def __init__(self, refresh_callback, streams):
++        self.refresh = refresh_callback
++        self.streams = streams
++        self.accounts = {}
++
++        manager = Accounts.Manager.new_for_service_type('microblogging')
++        manager.connect('enabled-event', self.on_enabled_event)
++        manager.connect('account-deleted', self.on_account_deleted)
++        for account_service in manager.get_enabled_account_services():
++            self.add_new_account(account_service)
++            log.info('Found {} accounts'.format(len(self.accounts)))
++
++    def on_enabled_event(self, manager, account_id):
++        account = manager.get_account(account_id)
++        for service in account.list_services():
++            account_service = Accounts.AccountService.new(account, service)
++            if account_service.get_enabled():
++                self.add_new_account(account_service)
++                self.refresh()
++
++    def on_account_deleted(self, manager, account_id):
++        log.debug('Deleting account ' + account_id)
++        account = self.accounts.pop(account_id)
++        try:
++            for stream in json.loads(self.streams.List()):
++                if stream['account'] == account['id']:
++                    self.streams.Delete(stream['id'])
++        except:
++            # TODO Identify what actually gets raised so we can catch
++            # it properly.
++            raise
++
++    def add_new_account(self, account_service):
++        new_account = Account(account_service)
++        self.accounts[new_account.global_id] = new_account
++
  class Account(dict):
      """A thin wrapper around libaccounts API."""
@@ -60,9 +105,14 @@
      def on_account_changed(self, account_service, account):
          settings = account.get_settings_iter('gwibber/')
--        self.update((key, value)
--                    for (success, key, value) in settings
--                    if success)
++        # This is a little bit ugly, but necessary until we get around
++        # to making the libaccounts API a little bit nicer.
++        while True:
++            success, key, value = settings.next()
++            if success:
++                self[key] = value
++            else:
++                break
      def enabled(self):
          return self.account_service.get_enabled()

Gwibber

Merge lp:~robru/gwibber/account-manager into lp:~barry/gwibber/py3

Commit message

Description of the change

Preview Diff

Subscribers