Code review comment for lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209

Revision history for this message
Michael Vogt (mvo) wrote :

On Mon, Sep 17, 2012 at 09:55:23PM -0000, Gary Lasker wrote:
> Ok, I added the check for the launcher queue to be properly emptied for the various types of transaction end cases. The error case, where a "transaction-stopped" signal is emitted, needed a fix to clear the queue so I've added that as well.
>
> Note that clearing the queue isn't strictly required as an item left there will have no ill effect (it's only functional when the corresponding package completes a successful install, and in this case it will "just word". Plus, it's not likely that many items would end up queued in here. Neverthess, it's clean and much nicer to clear it out for all cases, so it does that now.

Thanks for the updated test and code. I agree that while unlikely to
have ill effects its a good idea to clean the queue and the test will
ensure that this is the case. I like that :)

The diff is fine, the only nitpick I have is that in lines 132-138 and
161-168 the tests sets "self.expected_{pkgname,launcher_info}". Given
that the test is about not sending the signal it seems like this is
not needed, is that correct?

Cheers,
 Michael

« Back to merge proposal