On 12-08-28 05:04 PM, Barry Warsaw wrote:
>> +        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

Yeah, sorry. Somewhere along the line I fell into the bad habit of 
regarding my tests as just "test code" and not "real code" and thus I've 
become less stringent about adhering to PEP8 in the tests.

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

I had actually written a fake class originally, and there were dozens of 
lines of stub methods on it and I found it was actually quite 
cumbersome. The mock only required a few lines worth of return_value 
defintions in order to be serviceable, so I definitely prefer the mock.

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

But the beauty of unittest is that it tells you precisely which test 
failed and why, unlike using bare 'assert' statements. I understand this 
test is a bit long, I guess I'll leave it to you to split up.

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

The other reason that it's all crammed into one test is that I didn't 
want to have to duplicate the mock creation code needlessly. I guess it 
could be promoted to a module global and then referenced from different 
tests if you cared to tackle that though.

>> +        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(),
>              )

Done.

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

Done.

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

Sorry.

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

Well, the last time I was profiling code, it turned out that generator 
expressions were actually significantly slower than list comprehensions 
(though admittedly this was in py2.7, I'm not sure if it's changed since 
then). Generators have the benefit of not using up a lot of RAM, but 
they do so by invoking python functions repeatedly, which is a well 
known way to do things very slowly (function invocation is known to have 
a high overhead). So here you just have to choose whether you want to be 
wasteful of CPU time, or of RAM. On my 8GB machine I typically prefer to 
waste RAM ;-)

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

I find the wildly differing line lengths to be unattractive here, 
personally.

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

I'm indifferent, because you're still invoking a method when you access 
that attribute, so it's not like you're saving any processing time. It's 
purely stylistic. If you're going to change this make sure you grep 
through and change everything else that calls enabled(), too.

> I'm getting merge conflicts with trunk now.  This shows up in the merge
> proposal too.

I'm not sure why that happened. The conflict is resolved by deleting the 
Account class out of dispatcher.py. Or not, considering we're going to 
toss that file soon(ish) anyway.

Anyways, I've pushed some of your changes, feel free to do some of the 
other ones if you prefer. Thanks for the review!