| 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:49 |
| 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 | 01:56 |
| 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:38 |
| 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:39 |
| mwhudson | ah cool | 02:40 |
| thumper | waiting for the scanner on that one | 02:40 |
| 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:58 |
| 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 | 02:59 |
| mwhudson | another thing to delay this review :( | 03:04 |
| mwhudson | thumper: hm, i was merging it into devel :( | 03:08 |
| thumper | :( | 03:18 |
| mwhudson | ah now it works at last | 03:25 |
| mwhudson | complete with duff looking multi-line editor :-) | 03:25 |
| thumper | https://code.launchpad.net/~thumper/launchpad/set-empty-review-type-as-none/+merge/15302 | 03:26 |
| thumper | mwhudson: yeah, I think deryck is looking at that one | 03:27 |
| 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:31 |
| thumper | and default it? | 03:32 |
| thumper | sure | 03:32 |
| mwhudson | ywah | 03:32 |
| mwhudson | yes even :) | 03:32 |
| mwhudson | thanks | 03:32 |
| mwhudson | thumper: reviewed | 03:33 |
| thumper | ta | 03:34 |
| 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:12 |
| stub | jtv: That shouldn't be an XXX - it should just be a standard comment | 07:13 |
| jtv | Fixing... | 07:13 |
| stub | jtv: Wouldn't the real fix be to include boto in the launchpad-developer-dependencies? | 07:14 |
| jtv | stub: well it's not used in the test, and probably shouldn't be | 07:15 |
| jtv | I can just imagine the potential for recursion if this test really fired up an EC2 image. :) | 07:16 |
| 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:17 |
| 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:19 |
| jtv | Fair enough. | 07:20 |
| stub | approved | 07:21 |
| jtv | I'll make them separate comments for the two imports. | 07:21 |
| jtv | thanks | 07:21 |
| === 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 | ||
| 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 :) | 09:41 |
| === 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 | ||
| 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:17 |
| allenap | adeuring: I don't understand. I thought the thing in ec2test was a try...else? | 10:18 |
| adeuring | allenap: ouch, right. But let me test this "finally" nevertheless... | 10:19 |
| adeuring | allenap: OK, the "finally" code is indeed called! my bad... sorry for the noise | 10:20 |
| allenap | adeuring: Not at all, it's fine :) | 10:21 |
| 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:52 |
| 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:54 |
| 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. | 10:55 |
| 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:09 |
| 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:13 |
| 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:14 |
| adeuring | allenap: r=me | 11:21 |
| === 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 | ||
| jtv | adeuring: got a review for you! https://code.edge.launchpad.net/~jtv/launchpad/bug-487447/+merge/15315 | 14:23 |
| * adeuring is looking... | 14:23 | |
| === 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 | ||
| adeuring | jtv: you're adding a permission for public.project. Do you really need that? | 14:51 |
| === 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 | ||
| jtv | adeuring: hi, sorry for the delay... yes I want to do that because a project can have a translation group. | 15:46 |
| adeuring | jtv: ah, right, I didn't know that. Anyway, r=me | 15:47 |
| jtv | adeuring: thanks! | 15:47 |
| adeuring | welcome :) | 15:48 |
| allenap | adeuring: Do you fancy another review today? | 15:50 |
| adeuring | allenap: sure | 15:50 |
| 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:51 |
| allenap | adeuring: Thanks :) | 15:53 |
| === beuno is now known as beuno-lunch | ||
| noodles775 | adeuring: time for a one-liner? https://code.edge.launchpad.net/~michael.nelson/launchpad/470181-ppa-inline-style/+merge/15325 | 16:17 |
| adeuring | noodles775: nice distraction from a novel I'm reviewing ;) | 16:18 |
| noodles775 | lol | 16:18 |
| adeuring | noodles775: r=me | 16:37 |
| noodles775 | Thanks! | 16:37 |
| === beuno-lunch is now known as beuno | ||
| adeuring | allenap: r=me; 1 nitpick | 17:21 |
| allenap | adeuring: Woohoo! Thank you :) | 17:22 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!