[01:49] <thumper> mwhudson: could I talk through https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 with you?
[01:49] <thumper> mwhudson: it is very unexciting
[01:49] <thumper> mwhudson: the only major change is LPS vs YUI() in many places
[01:49] <thumper> mwhudson: I talked this through with mars and he is v.happy that someone else is taking something off his plate
[01:56] <mwhudson> thumper: okay
[01:56] <thumper> mwhudson: thanks
[01:56] <mwhudson> thumper: do you want to skype?
[01:56] <mwhudson> i should read the diff first in any case
[01:56] <thumper> sure
[01:56] <thumper> ping me if you want to cover anything
[02:38] <mwhudson> thumper: i'm trying to play with it but things keep getting in my way and then i get distracted
[02:38] <thumper> :)
[02:39] <thumper> I'll have another soon
[02:39] <thumper> normalizes the review_type in the merge proposal model code
[02:39] <thumper> so '  ' -> None
[02:40] <mwhudson> ah cool
[02:40] <thumper> waiting for the scanner on that one
[02:58] <mwhudson> thumper: the commit editor doesn't work for me any more in launchpad.dev :(
[02:58] <thumper> ??
[02:58] <thumper> what do you mean?
[02:59] <mwhudson> thumper: i click the pencil icon and it works like a link
[02:59] <thumper> :)
[02:59] <thumper> I know what that is
[02:59] <thumper> make clean build
[02:59] <thumper> yui 3.0
[02:59] <mwhudson> ok
[03:04] <mwhudson> another thing to delay this review :(
[03:08] <mwhudson> thumper: hm, i was merging it into devel :(
[03:18] <thumper> :(
[03:25] <mwhudson> ah now it works at last
[03:25] <mwhudson> complete with duff looking multi-line editor :-)
[03:26] <thumper> https://code.launchpad.net/~thumper/launchpad/set-empty-review-type-as-none/+merge/15302
[03:27] <thumper> mwhudson: yeah, I think deryck is looking at that one
[03:31] <mwhudson> thumper: in test_pending_review_registrant it's not super obvious to me that the merge_proposal.registrant is who's passed into nominateReviewer
[03:31] <thumper> hmm.. yeah
[03:31] <mwhudson> thumper: could you give makeProposalWithReviewer an optional "requester" argument do you think?
[03:32] <thumper> and default it?
[03:32] <thumper> sure
[03:32] <mwhudson> ywah
[03:32] <mwhudson> yes even :)
[03:32] <mwhudson> thanks
[03:33] <mwhudson> thumper: reviewed
[03:34] <thumper> ta
[07:12] <jtv> stub: I thought I could get mwhudson to review my testfix branch, but haven't heard from him in a bit... want to help out?
[07:12] <jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-489068/+merge/15309
[07:13] <stub> jtv: That shouldn't be an XXX - it should just be a standard comment
[07:13] <jtv> Fixing...
[07:14] <stub> jtv: Wouldn't the real fix be to include boto in the launchpad-developer-dependencies?
[07:15] <jtv> stub: well it's not used in the test, and probably shouldn't be
[07:16] <jtv> I can just imagine the potential for recursion if this test really fired up an EC2 image.  :)
[07:17] <stub> We try to keep the environments as similar as possible - this is just sweeping this drift between the environments under the carpet where it might bite someone later.
[07:17] <stub> (Which is understandable, as it is busted *now* and updating ec2 images seems to take days or longer)
[07:19] <jtv> in that case, I can file a bug for having a boto egg (or launchpad-dependency?)
[07:19] <stub> Yes. I'm wondering if that comment should be an XXX referencing that bug then.
[07:20] <jtv> Fair enough.
[07:21] <stub> approved
[07:21] <jtv> I'll make them separate comments for the two imports.
[07:21] <jtv> thanks
[09:41] <allenap> adeuring: Could you review my multithreaded checkwatches branch please? https://code.edge.launchpad.net/~allenap/launchpad/multithreaded-checkwatches/+merge/15283
[09:41] <adeuring> allenap: sure
[09:41] <allenap> adeuring: Thanks :)
[10:17] <adeuring> allenap: the pattern try: return func(...); finally: self._logout() in _interactionDecorator() looks a bit odd. It is not wrong, but jtv spotted a bug in a similar pattern in ec2test. Effectively, the "return" transforms the "Finally" into an "except", so I think it is better to use the latter
[10:18] <allenap> adeuring: I don't understand. I thought the thing in ec2test was a try...else?
[10:19] <adeuring> allenap: ouch, right. But let me test this "finally" nevertheless...
[10:20] <adeuring> allenap: OK, the "finally" code is indeed called! my bad... sorry for the noise
[10:21] <allenap> adeuring: Not at all, it's fine :)
[10:52] <adeuring> allenap: perhaps I misunderstood something aabout threading, so i may be wrong. but: updateBugTracker() calls self.txn.begin() and later self.txn.commit(). As I understand it, all worker threads share one "self" instance, so don't they begin/commit the same transaction?
[10:54] <allenap> adeuring: This was a fun thing for me to work through :) The transactions are stored as thread local vars. I think self.txn is just a reference to the transaction module.
[10:55] <adeuring> allenap: that's really interesting. Could you add a comment explaining this so that other people don't ask the same question as I did ;)?
[10:55] <allenap> adeuring: Sure, good idea.
[11:09] <adeuring> allenap: last question (hopefully...): do you know if calls like self.log.inf() or self.warning() are thread safe?
[11:09]  * adeuring is a bit paranoid regarding threading
[11:09] <allenap> adeuring: I don't know actually. I'll check.
[11:13] <allenap> adeuring: The logging module has been written to be thread-aware (though I've spotted a race-condition already). I think that's enough.
[11:14] <allenap> adeuring: What do you think?
[11:14] <adeuring> allenap: yeah, sure. Though it might be worth to file a bug about the race condition
[11:14] <allenap> adeuring: Yeah, definitely :)
[11:21] <adeuring> allenap: r=me
[14:23] <jtv> adeuring: got a review for you!  https://code.edge.launchpad.net/~jtv/launchpad/bug-487447/+merge/15315
[14:23]  * adeuring is looking...
[14:51] <adeuring> jtv: you're adding a permission for public.project. Do you really need that?
[15:46] <jtv> adeuring: hi, sorry for the delay... yes I want to do that because a project can have a translation group.
[15:47] <adeuring> jtv: ah, right, I didn't know that. Anyway, r=me
[15:47] <jtv> adeuring: thanks!
[15:48] <adeuring> welcome :)
[15:50] <allenap> adeuring: Do you fancy another review today?
[15:50] <adeuring> allenap: sure
[15:51] <allenap> adeuring: It's a big one at ~1500 lines but there's a lot of removals and refactoring in there. Take a look and see if you feel like doing it. I won't mind at all if you say no :) https://code.edge.launchpad.net/~allenap/launchpad/update-project-group-page-bug-434764/+merge/15322
[15:51] <allenap> adeuring: I just don't know how to break it up now :(
[15:51] <adeuring> allenap: I'll give it a try ;)
[15:53] <allenap> adeuring: Thanks :)
[16:17] <noodles775> adeuring: time for a one-liner? https://code.edge.launchpad.net/~michael.nelson/launchpad/470181-ppa-inline-style/+merge/15325
[16:18] <adeuring> noodles775: nice distraction from a novel I'm reviewing ;)
[16:18] <noodles775> lol
[16:37] <adeuring> noodles775: r=me
[16:37] <noodles775> Thanks!
[17:21] <adeuring> allenap: r=me; 1 nitpick
[17:22] <allenap> adeuring: Woohoo! Thank you :)