[01:49] mwhudson: could I talk through https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 with you? [01:49] mwhudson: it is very unexciting [01:49] mwhudson: the only major change is LPS vs YUI() in many places [01:49] mwhudson: I talked this through with mars and he is v.happy that someone else is taking something off his plate [01:56] thumper: okay [01:56] mwhudson: thanks [01:56] thumper: do you want to skype? [01:56] i should read the diff first in any case [01:56] sure [01:56] ping me if you want to cover anything [02:38] thumper: i'm trying to play with it but things keep getting in my way and then i get distracted [02:38] :) [02:39] I'll have another soon [02:39] normalizes the review_type in the merge proposal model code [02:39] so ' ' -> None [02:40] ah cool [02:40] waiting for the scanner on that one [02:58] thumper: the commit editor doesn't work for me any more in launchpad.dev :( [02:58] ?? [02:58] what do you mean? [02:59] thumper: i click the pencil icon and it works like a link [02:59] :) [02:59] I know what that is [02:59] make clean build [02:59] yui 3.0 [02:59] ok [03:04] another thing to delay this review :( [03:08] thumper: hm, i was merging it into devel :( [03:18] :( [03:25] ah now it works at last [03:25] complete with duff looking multi-line editor :-) [03:26] https://code.launchpad.net/~thumper/launchpad/set-empty-review-type-as-none/+merge/15302 [03:27] mwhudson: yeah, I think deryck is looking at that one [03:31] 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] hmm.. yeah [03:31] thumper: could you give makeProposalWithReviewer an optional "requester" argument do you think? [03:32] and default it? [03:32] sure [03:32] ywah [03:32] yes even :) [03:32] thanks [03:33] thumper: reviewed [03:34] ta [07:12] 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] https://code.edge.launchpad.net/~jtv/launchpad/bug-489068/+merge/15309 [07:13] jtv: That shouldn't be an XXX - it should just be a standard comment [07:13] Fixing... [07:14] jtv: Wouldn't the real fix be to include boto in the launchpad-developer-dependencies? [07:15] stub: well it's not used in the test, and probably shouldn't be [07:16] I can just imagine the potential for recursion if this test really fired up an EC2 image. :) [07:17] 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] (Which is understandable, as it is busted *now* and updating ec2 images seems to take days or longer) [07:19] in that case, I can file a bug for having a boto egg (or launchpad-dependency?) [07:19] Yes. I'm wondering if that comment should be an XXX referencing that bug then. [07:20] Fair enough. [07:21] approved [07:21] I'll make them separate comments for the two imports. [07:21] thanks === adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [09:41] adeuring: Could you review my multithreaded checkwatches branch please? https://code.edge.launchpad.net/~allenap/launchpad/multithreaded-checkwatches/+merge/15283 [09:41] allenap: sure [09:41] adeuring: Thanks :) === adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [10:17] 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] adeuring: I don't understand. I thought the thing in ec2test was a try...else? [10:19] allenap: ouch, right. But let me test this "finally" nevertheless... [10:20] allenap: OK, the "finally" code is indeed called! my bad... sorry for the noise [10:21] adeuring: Not at all, it's fine :) [10:52] 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] 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] 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] adeuring: Sure, good idea. [11:09] 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] adeuring: I don't know actually. I'll check. [11:13] 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] adeuring: What do you think? [11:14] allenap: yeah, sure. Though it might be worth to file a bug about the race condition [11:14] adeuring: Yeah, definitely :) [11:21] allenap: r=me === matsubara-afk is now known as matsubara === danilo_ is now known as danilos === adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [14:23] adeuring: got a review for you! https://code.edge.launchpad.net/~jtv/launchpad/bug-487447/+merge/15315 [14:23] * adeuring is looking... === adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [14:51] jtv: you're adding a permission for public.project. Do you really need that? === bigjools-afk is now known as bigjools === adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:46] adeuring: hi, sorry for the delay... yes I want to do that because a project can have a translation group. [15:47] jtv: ah, right, I didn't know that. Anyway, r=me [15:47] adeuring: thanks! [15:48] welcome :) [15:50] adeuring: Do you fancy another review today? [15:50] allenap: sure [15:51] 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] adeuring: I just don't know how to break it up now :( [15:51] allenap: I'll give it a try ;) [15:53] adeuring: Thanks :) === beuno is now known as beuno-lunch [16:17] adeuring: time for a one-liner? https://code.edge.launchpad.net/~michael.nelson/launchpad/470181-ppa-inline-style/+merge/15325 [16:18] noodles775: nice distraction from a novel I'm reviewing ;) [16:18] lol [16:37] noodles775: r=me [16:37] Thanks! === beuno-lunch is now known as beuno [17:21] allenap: r=me; 1 nitpick [17:22] adeuring: Woohoo! Thank you :) === adeuring changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-afk