=== henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jelmer changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [10:13] jtv: are you ok with a short cover letter? [10:14] henninge: sure, just mention anything that might be surprising. [10:14] * henninge surprises himself sometime ... ;) [10:15] *sometimes* [10:24] Oops, my gnome just screwed itself up. [10:55] jelmer: Hi! you have two branches ready for review, which one do you want reviewed? [10:55] henninge: both, if possible [10:55] henninge: the cleanup one is trivial but the upload log one is more important [10:55] jelmer: ok, I'll look at both [10:59] jelmer: thanks for the clean-up! ;) If your team is switching to tag-based QA (I think all are), you should attach a bug to it. [10:59] jelmer: r=me, though. [10:59] ;) [10:59] henninge: Will do. Thanks for the review! === henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: jelmer || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [11:01] jtv: back up and running [11:01] https://code.edge.launchpad.net/~henninge/launchpad/bug-517080-pottery-split/+merge/19325 [11:01] ? [11:02] henninge: yes, back up and running... This gets a bit scary though. I wonder if it's the temperature differences. [11:02] jtv: which is close to 40 degrees, isn't it? ... ;) [11:03] 35 at this very moment [11:03] The difference is probably about 20°C since I'm working in a building (though unheated). But I think it's what caused the trouble with the backup disk in my office. [11:09] henninge: have a look at TestCaseWithFactory.useTempDir [11:10] jtv: yes, I actually saw that just today. Do you know if I can use TestCaseWithFactory without setting a layer? I am quite happy about the quick start-up of the test. [11:11] oh, you probably need a layer for it, true. So why not move that helper into lp.testing.TestCase? [11:11] I don't see it using anything that depends on any layers [11:12] jtv: good idea. [11:12] henninge: also, line 91 of the diff (not touched here, but still): packagename+".tar.bz2"... spaces around the + please [11:13] right, wonder who reviewed that line before ... [11:13] :-P [11:14] jelmer: the other mp has a merge conflict [11:16] jelmer: I guess you need to merge a current devel and push the branch again. [11:18] henninge: will do [11:22] henninge: could you replace the comments in TestDetectIntltoolInBzrTree with less confusing ones? [11:26] jtv: oh, sorry, copy-and-paste error ... ;) [11:27] henninge: Good idea to have the "with" here. === intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: jelmer || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [11:45] henninge: it'd be nice to have some kind of clear distinction between "server-safe" and "slave-side" code. [11:46] jtv: yes, I've been wondering about that. Should I move it to its own module? [11:49] henninge: I'm not sure... it can also be useful to have it close to the template-generation code whose preconditions it mimicks. [11:50] All I'm really interested in "from the outside" is (1) a server-side check whether pottery thinks it'll be able to produce templates for my branch, and (2) a slave-side operation to generate the templates. Maybe in the future we'll want to have that plus a bunch of modules for the different kinds of translation setup that pottery may encounter. [11:54] So I'd suggest not splitting this up into modules until a pattern emerges. [11:59] henninge: Can you come up with another clear way to mark the unsafe code? [12:00] henninge: new proposal at https://code.edge.launchpad.net/~jelmer/launchpad/bug499115/+merge/19328 [12:00] jtv: "unsafe" as in "should run on slave", right? [12:00] henninge: right [12:00] s/should/must/ [12:02] jtv: I had been thinking about moving the functions into a class. That way they are clearly groped. Maybe the class could even do some automatic checking that it is really running on a slave. [12:03] henninge: a class could be very nice. I wouldn't do any checking there though; that's hard to test and at the same time security-sensitive. [12:03] It makes more sense to isolate as much as you can in code that doesn't care where it runs. [12:03] jtv: I didn't meant to add the checks right now, anyway. [12:04] Another problem is getting the code onto the slave. It'll need to be either in the buildd package, or some other package we can install on the slave. [12:07] jelmer: no test? [12:08] henninge, unfortunately, no :-/ I couldn't find a good way to test this. [12:44] * henninge lunches === henninge is now known as henninge-lunc [13:06] intellectronica, got time for a pretty big one? https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19131 [13:06] jtv: i'm just about to break for lunch, but if it can wait until i'm back then sure [13:06] intellectronica: it can wait. thanks. [13:06] np [13:11] on call: henninge, intellectronica || reviewing: jelmer || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [13:21] wgrant: still around? === henninge-lunc is now known as henninge [13:56] -/n [13:57] henninge: can you take a look at the last comments for bug 127171 ? [13:57] Bug #127171: Rosetta experts not allowed to "Change translators" [13:57] I would like to finalize the work and submit a MP [14:07] adiroiban: looking [14:20] adiroiban: you need to merge a current devel, first. Your branch seeems to be rotting. [14:22] henninge: I just found bug 516317, which is about renaming that URL [14:22] Bug #516317: Product:+changetranslators should become +settings [14:23] I guess I will have to wait for Danilo to merge that branch and then continue my work [14:25] adiroiban: yes and you should finish the pre-imp discussion with him, I guess. [14:26] adiroiban: but the thing about updating your branch is true. Your addition to security.py won't work the way it is now because of a branch I landed a few weeks ago. [14:28] henninge: true. I have merged the branch on my local machine, but did not push the changes to LP. I will continue with Danilo. Thanks! [14:35] jtv: there are merge conflict artifacts in your diff [14:35] intellectronica: I guess I accidentally proposed for merging in db-devel. [14:36] indeed [14:36] care to paste the correct diff for me? [14:36] intellectronica: I guess I can make a corrected copy of the MP. [14:36] jtv: even better [14:38] intellectronica: https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19335 [14:38] jtv: cheers [14:39] jtv: in extractBuildStatus, how about raising a ValueError instead of asserting? [14:40] intellectronica: I'd rather be conservative there... that part is a refactoring, in code that's basically impossible to integration-test. [14:40] intellectronica: I do agree it would make more sense though. [14:41] jtv: oh, you mean the assertion isn't a new addition but just code moved from somewhere? [14:41] intellectronica: eso. [14:41] eso? [14:41] exactly so? [14:41] Right. :) [14:41] Well, from Spanish, but semantically yes. [14:41] gotcha [14:41] "That." [14:42] oooh i like FakeMethod [14:43] it's a nice thing to have [14:44] thanks... I'm sure you can guess what name I _wanted_ to give it but didn't in consideration for a colleague who is geographically capable of getting to me physically? [14:48] I did write (and heavily use) a class of that name in the ec2test unit-tests I wrote, so I'll also file a bug for replacing that with FakeMethod. [14:49] Or better yet, I could do it in this branch. But I'll leave that for after the main review so it doesn't clutter up the diff. [14:54] jtv: r=me. great work. [14:54] intellectronica: thanks! No mean compliment from you. :) [14:58] henninge or intellectronica: please review https://code.edge.launchpad.net/~leonardr/lazr.restful/exported-subclass/+merge/19337 [14:59] leonardr: i'll take it [15:01] leonardr: r=me [15:12] jelmer: sorry, got distracted. I sent a review with a suggestion on how to add a test. Please simply push to the branch, don't "resubmit" the proposal, like you did earlier. [15:12] henninge, ok, thanks === henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jamlata-afk is now known as jamalta [15:36] jtv: you did not mention moving the functions into a class in your review - are you with holding that back for a later branch? [15:36] henninge: yes, I think it makes sense to leave that for a next step: getting the right code to the slave. === henninge is now known as henninge-afk [16:35] intellectronica, i need a launchpadlib and a launchpad branch reviewed in tandem [16:36] leonardr: oright [16:37] intellectronica: if you can give the launchpadlib branch the once-over i can make a preliminary package for it and put it in my site-dist to run the ec2 test on the launchpad branch [16:37] leonardr: ok [16:37] leonardr: MP? [16:38] intellectronica: on the way [16:38] cool [16:44] intellectronica: https://code.edge.launchpad.net/~leonardr/launchpadlib/multiversion/+merge/19343 [16:49] leonardr: r=me [16:49] cool [16:49] working on launchpad one now [16:53] intellectronica: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346 [16:56] leonardr: looks good, as far as i can understand [16:57] leonardr: can you explain briefly how the grok thing works and why it's necessary? [16:57] intellectronica: sure [16:57] the way we use grok, it checks to see if you subclassed a certain class, and then performs some magic [16:59] in this case, the magic looks like this: [16:59] http://pastebin.ubuntu.com/376972/ [17:00] intellectronica: so when the magic code runs, we get an autogenerated marker interface for each version of the web service [17:00] IWebServiceClientRequestVersion_beta [17:00] IWebServiceClientRequestVersion_1_0 [17:00] gotcha [17:00] IWebServiceClientRequestVersion_devel [17:00] leonardr: r=me [17:00] cool [17:01] now to get it all together and run the test [17:01] good luck === adiroiban changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue [adiroiban(codereview bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:03] adiroiban: i apologize, but i probably won't have time to review your branch today [17:04] it's quite a big branch [17:04] intellectronica: no hurry, I just put in there to be reviewed when someone has a bit of free time === intellectronica changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [adiroiban(codereview bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === flacoste is now known as flacoste_lunch === danilos is now known as daniloff === flacoste_lunch is now known as flacoste === jamalta is now known as jamalta-afk === jamalta-afk is now known as jamalta === jamalta is now known as jamalta-afk