Hi Robert,
Thanks again for all the awesome work on the Python 3 port of Gwibber. Here
are some detailed comments on the Accounts branch.
=== added file 'gwibber/gwibber/tests/test_account.py'
--- gwibber/gwibber/tests/test_account.py 1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/tests/test_account.py 2012-08-28 21:31:33 +0000
> @@ -0,0 +1,100 @@
> +# 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 .
> +
> +"""Test the Account class."""
> +
> +__all__ = [
> + 'TestAccount',
> + ]
> +
> +
> +import unittest
> +
> +from threading import Lock
> +
> +from gwibber.utils.account import Account
> +
> +try:
> + # Python 3.3
> + from unittest import mock
> +except ImportError:
> + import mock
> +
> +
> +class TestAccount(unittest.TestCase):
> + """Test Account class."""
> +
> + def test_basic_api(self):
> + fake_account_service = mock.MagicMock()
> + fake_account_service.get_account().get_settings_iter.return_value = iter([(True, 'key', 'val')])
PEP 8 recommends a maximum line length of 79 characters.
http://www.python.org/dev/peps/pep-0008/#maximum-line-length
There's ways to rewrite this code to fit, but I have another suggestion in a
moment..
> + fake_account_service.get_account().id = 'fake_id'
> + fake_account_service.get_service().get_name.return_value = 'fake_service'
> +
> + account = Account(fake_account_service)
I'm not actually sure you need to mock this to get decent tests. Since you're
passing account_service into the constructor, the test module could just
create a simple class that provides the required API. Instantiate that and
pass it into the constructor, then have it respond the way you want it to to
the various calls.
> + fake_account_service.get_auth_data.assert_called_once_with()
> +
> + fake_account_service.connect.assert_called_once_with('changed',
> + account.on_account_changed, fake_account_service.get_account())
> +
> + account['name'] = 'Fred'
> + self.assertEqual(account['name'], 'Fred')
> +
> + self.assertTrue('auth' in account)
> + self.assertTrue('id' in account['auth'])
> + self.assertTrue('method' in account['auth'])
> + self.assertTrue('mechanism' in account['auth'])
> + self.assertTrue('parameters' in account['auth'])
> + self.assertEqual(len(account['auth']), 4)
> +
> + self.assertTrue('service' in account)
> +
> + self.assertTrue('id' in account)
If you do create a fake, unmocked account_service object, then you won't need
to do the assertTrues here. You could actually test that the expected values
exist with assertEqual. That'll be easier to debug if some day there are
failures.
> + self.assertEqual(account['id'], 'fake_id/fake_service')
> + self.assertEqual(account.global_id, 'fake_id')
> +
> + self.assertFalse('foobar' in account)
> + with self.assertRaises(KeyError):
> + account['foobar']
> +
> + self.assertTrue(account.login_lock.acquire())
> +
> + fake_account_service.get_enabled.return_value = True
> + self.assertTrue(account.enabled())
> + fake_account_service.get_enabled.return_value = False
> + self.assertFalse(account.enabled())
Also, I typically prefer really tight, focused tests for each bit of code
you're testing. That way if you get a failure because the
login_lock.acquire() returned False, you'll know immediately that the test
didn't fail because, say account['id'] had the wrong value.
You wouldn't need to test each individual key in account['auth'], because
those are logically the same bit of code. But this test could be split up
into several smaller tests that each flex a single logical chunk of the
constructor.
> +
> + def test_account_changed(self):
> + fake_account_service = mock.MagicMock()
> + fake_account_service.get_account().get_settings_iter.return_value = iter(
> + [(True, 'key', 'val'), (True, 'setting', 'foobar')])
> +
> + account = Account(fake_account_service)
> + self.assertEqual(account['key'], 'val')
> + self.assertEqual(account['setting'], 'foobar')
> +
> + fake_account_service.get_account().get_settings_iter.return_value = iter(
> + [(True, 'new', 'stuff'), (True, 'gets', 'added'),
> + (False, 'not_gwibber/wrong', 'stuff is ignored')])
> +
> + # I know this method has a goofy signature, but it's a signal callback,
> + # so it never actually gets called like this. It was necessary here
> + # because the MagicMock isn't magical enough to emit signals for us...
> + account.on_account_changed(
> + account.account_service,
> + account.account_service.get_account())
> +
> + self.assertEqual(account['new'], 'stuff')
> + self.assertEqual(account['gets'], 'added')
> + self.assertFalse('wrong' in account)
> +
=== added file 'gwibber/gwibber/utils/account.py'
--- gwibber/gwibber/utils/account.py 1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/utils/account.py 2012-08-28 21:31:33 +0000
> @@ -0,0 +1,57 @@
> +# 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 .
> +
> +
> +from threading import Lock
> +
> +
> +class Account(dict):
> + def __init__(self, account_service):
> + self.account_service = account_service
> +
> + auth_data = account_service.get_auth_data()
> + self['auth'] = {
> + 'id': auth_data.get_credentials_id(),
> + 'method': auth_data.get_method(),
> + 'mechanism': auth_data.get_mechanism(),
> + 'parameters': auth_data.get_parameters(),
> + }
An alternative style I sometimes like, which works if all the keys are Python
identifiers (as is the case here), is something like this:
self['auth'] = dict(
id = auth_data.get_credentials_id(),
method = auth_data.get_method(),
mechanism = auth_data.get_mechanism(),
parameters = auth_data.get_parameters(),
)
IOW, use keyword arguments to the dict() type.
> +
> + account = account_service.get_account()
> + self.global_id = account.id
> + service_name = account_service.get_service().get_name()
> + self['id'] = '%s/%s' % (self.global_id, service_name)
I generally prefer str.format() over %s-interpolation these days, e.g.
self['id'] = '{}/{}'.format(self.global_id, service_name)
I find it generally more readable, and there are often better options for
conforming to PEP 8 line lengths.
> +
> + # the provider name in libaccounts should match the name of our plugin
PEP 8 recommends full sentences in comments, i.e. capitalize and end in a
period.
> + self['service'] = account.get_provider_name()
> +
> + account_service.connect('changed', self.on_account_changed, account)
> + self.on_account_changed(account_service, account)
> +
> + # this is used to prevent multiple simultaneous login attempts
> + self.login_lock = Lock()
> +
> + def on_account_changed(self, account_service, account):
> + self.update(
> + [(key, value)
> + for (success, key, value)
> + in account.get_settings_iter('gwibber/')
> + if success])
You don't need a list comprehension here because a generator comprehension
will work just fine. There's no need to create a temporary concrete list that
just gets thrown away. Also, stylistically, I'd probably format this a bit
differently, e.g.
self.update(
(key, value)
for (success, key, value) in account.get_settings_iter('gwibber/')
if success)
> +
> + def enabled(self):
> + return self.account_service.get_enabled()
What do you think about using a property here? E.g. adding
@property
then using it like: `if account.enabled` instead of having to call it.
> +
> + def __eq__(self, other):
> + return self.account_service == other.account_service
> +
=== modified file 'gwibber/microblog/dispatcher.py'
--- gwibber/microblog/dispatcher.py 2012-08-27 19:39:58 +0000
+++ gwibber/microblog/dispatcher.py 2012-08-28 21:31:33 +0000
> @@ -138,6 +138,7 @@
> logger.debug("%s Finished operation (%s)" % (logtext, str(self.t_runtime)))
>
>
> +<<<<<<< TREE
> class Account(collections.MutableMapping):
> def __init__(self, account_service):
> self._account_service = account_service
> @@ -204,6 +205,8 @@
> return iter(self._dict.items())
>
>
> +=======
> +>>>>>>> MERGE-SOURCE
> class OperationCollector:
> def __init__(self, dispatcher):
> self.dispatcher = dispatcher
> @@ -476,7 +479,7 @@
> def on_account_deleted(self, account_manager, account_id):
> # Delete streams associated with the user that was deleted
> for account in self._accounts:
> - if account.global_id() != account_id: continue
> + if account.global_id != account_id: continue
> logger.debug("Deleting account %s" % account["id"])
> self._accounts.remove(account)
> try:
I'm getting merge conflicts with trunk now. This shows up in the merge
proposal too.
Anyway, since you're now working on the nastiest part of the code base, I'd be
happy to make the corrections I suggest and merge to trunk. I'd hate to have
you lose good momentum on the dispatcher rewrite.