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.