| === 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 | ||
| henninge | jtv: are you ok with a short cover letter? | 10:13 |
|---|---|---|
| jtv | henninge: sure, just mention anything that might be surprising. | 10:14 |
| * henninge surprises himself sometime ... ;) | 10:14 | |
| henninge | *sometimes* | 10:15 |
| jtv | Oops, my gnome just screwed itself up. | 10:24 |
| henninge | jelmer: Hi! you have two branches ready for review, which one do you want reviewed? | 10:55 |
| jelmer | henninge: both, if possible | 10:55 |
| jelmer | henninge: the cleanup one is trivial but the upload log one is more important | 10:55 |
| henninge | jelmer: ok, I'll look at both | 10:55 |
| henninge | 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 |
| henninge | jelmer: r=me, though. | 10:59 |
| henninge | ;) | 10:59 |
| jelmer | henninge: Will do. Thanks for the review! | 10:59 |
| === 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 | ||
| henninge | jtv: back up and running | 11:01 |
| henninge | https://code.edge.launchpad.net/~henninge/launchpad/bug-517080-pottery-split/+merge/19325 | 11:01 |
| henninge | ? | 11:01 |
| jtv | henninge: yes, back up and running... This gets a bit scary though. I wonder if it's the temperature differences. | 11:02 |
| henninge | jtv: which is close to 40 degrees, isn't it? ... ;) | 11:02 |
| henninge | 35 at this very moment | 11:03 |
| jtv | 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:03 |
| jtv | henninge: have a look at TestCaseWithFactory.useTempDir | 11:09 |
| henninge | 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:10 |
| jtv | oh, you probably need a layer for it, true. So why not move that helper into lp.testing.TestCase? | 11:11 |
| jtv | I don't see it using anything that depends on any layers | 11:11 |
| henninge | jtv: good idea. | 11:12 |
| jtv | henninge: also, line 91 of the diff (not touched here, but still): packagename+".tar.bz2"... spaces around the + please | 11:12 |
| henninge | right, wonder who reviewed that line before ... | 11:13 |
| henninge | :-P | 11:13 |
| henninge | jelmer: the other mp has a merge conflict | 11:14 |
| henninge | jelmer: I guess you need to merge a current devel and push the branch again. | 11:16 |
| jelmer | henninge: will do | 11:18 |
| jtv | henninge: could you replace the comments in TestDetectIntltoolInBzrTree with less confusing ones? | 11:22 |
| henninge | jtv: oh, sorry, copy-and-paste error ... ;) | 11:26 |
| jtv | henninge: Good idea to have the "with" here. | 11:27 |
| === 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 | ||
| jtv | henninge: it'd be nice to have some kind of clear distinction between "server-safe" and "slave-side" code. | 11:45 |
| henninge | jtv: yes, I've been wondering about that. Should I move it to its own module? | 11:46 |
| jtv | henninge: I'm not sure... it can also be useful to have it close to the template-generation code whose preconditions it mimicks. | 11:49 |
| jtv | 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:50 |
| jtv | So I'd suggest not splitting this up into modules until a pattern emerges. | 11:54 |
| jtv | henninge: Can you come up with another clear way to mark the unsafe code? | 11:59 |
| jelmer | henninge: new proposal at https://code.edge.launchpad.net/~jelmer/launchpad/bug499115/+merge/19328 | 12:00 |
| henninge | jtv: "unsafe" as in "should run on slave", right? | 12:00 |
| jtv | henninge: right | 12:00 |
| henninge | s/should/must/ | 12:00 |
| henninge | 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:02 |
| jtv | 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 |
| jtv | It makes more sense to isolate as much as you can in code that doesn't care where it runs. | 12:03 |
| henninge | jtv: I didn't meant to add the checks right now, anyway. | 12:03 |
| jtv | 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:04 |
| henninge | jelmer: no test? | 12:07 |
| jelmer | henninge, unfortunately, no :-/ I couldn't find a good way to test this. | 12:08 |
| * henninge lunches | 12:44 | |
| === henninge is now known as henninge-lunc | ||
| jtv | intellectronica, got time for a pretty big one? https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19131 | 13:06 |
| intellectronica | jtv: i'm just about to break for lunch, but if it can wait until i'm back then sure | 13:06 |
| jtv | intellectronica: it can wait. thanks. | 13:06 |
| intellectronica | np | 13:06 |
| jtv | on call: henninge, intellectronica || reviewing: jelmer || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | 13:11 |
| bigjools | wgrant: still around? | 13:21 |
| === henninge-lunc is now known as henninge | ||
| adiroiban | -/n | 13:56 |
| adiroiban | henninge: can you take a look at the last comments for bug 127171 ? | 13:57 |
| mup | Bug #127171: Rosetta experts not allowed to "Change translators" <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/127171> | 13:57 |
| adiroiban | I would like to finalize the work and submit a MP | 13:57 |
| henninge | adiroiban: looking | 14:07 |
| henninge | adiroiban: you need to merge a current devel, first. Your branch seeems to be rotting. | 14:20 |
| adiroiban | henninge: I just found bug 516317, which is about renaming that URL | 14:22 |
| mup | Bug #516317: Product:+changetranslators should become +settings <ui> <Launchpad Translations:In Progress by danilo> <https://launchpad.net/bugs/516317> | 14:22 |
| adiroiban | I guess I will have to wait for Danilo to merge that branch and then continue my work | 14:23 |
| henninge | adiroiban: yes and you should finish the pre-imp discussion with him, I guess. | 14:25 |
| henninge | 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:26 |
| adiroiban | 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:28 |
| intellectronica | jtv: there are merge conflict artifacts in your diff | 14:35 |
| jtv | intellectronica: I guess I accidentally proposed for merging in db-devel. | 14:35 |
| intellectronica | indeed | 14:36 |
| intellectronica | care to paste the correct diff for me? | 14:36 |
| jtv | intellectronica: I guess I can make a corrected copy of the MP. | 14:36 |
| intellectronica | jtv: even better | 14:36 |
| jtv | intellectronica: https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19335 | 14:38 |
| intellectronica | jtv: cheers | 14:38 |
| intellectronica | jtv: in extractBuildStatus, how about raising a ValueError instead of asserting? | 14:39 |
| jtv | intellectronica: I'd rather be conservative there... that part is a refactoring, in code that's basically impossible to integration-test. | 14:40 |
| jtv | intellectronica: I do agree it would make more sense though. | 14:40 |
| intellectronica | jtv: oh, you mean the assertion isn't a new addition but just code moved from somewhere? | 14:41 |
| jtv | intellectronica: eso. | 14:41 |
| intellectronica | eso? | 14:41 |
| intellectronica | exactly so? | 14:41 |
| jtv | Right. :) | 14:41 |
| jtv | Well, from Spanish, but semantically yes. | 14:41 |
| intellectronica | gotcha | 14:41 |
| jtv | "That." | 14:41 |
| intellectronica | oooh i like FakeMethod | 14:42 |
| intellectronica | it's a nice thing to have | 14:43 |
| jtv | 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:44 |
| jtv | 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:48 |
| jtv | 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:49 |
| intellectronica | jtv: r=me. great work. | 14:54 |
| jtv | intellectronica: thanks! No mean compliment from you. :) | 14:54 |
| leonardr | henninge or intellectronica: please review https://code.edge.launchpad.net/~leonardr/lazr.restful/exported-subclass/+merge/19337 | 14:58 |
| intellectronica | leonardr: i'll take it | 14:59 |
| intellectronica | leonardr: r=me | 15:01 |
| henninge | 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 |
| jelmer | henninge, ok, thanks | 15:12 |
| === 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 | ||
| henninge | 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 |
| jtv | henninge: yes, I think it makes sense to leave that for a next step: getting the right code to the slave. | 15:36 |
| === henninge is now known as henninge-afk | ||
| leonardr | intellectronica, i need a launchpadlib and a launchpad branch reviewed in tandem | 16:35 |
| intellectronica | leonardr: oright | 16:36 |
| leonardr | 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 |
| intellectronica | leonardr: ok | 16:37 |
| intellectronica | leonardr: MP? | 16:37 |
| leonardr | intellectronica: on the way | 16:38 |
| intellectronica | cool | 16:38 |
| leonardr | intellectronica: https://code.edge.launchpad.net/~leonardr/launchpadlib/multiversion/+merge/19343 | 16:44 |
| intellectronica | leonardr: r=me | 16:49 |
| leonardr | cool | 16:49 |
| leonardr | working on launchpad one now | 16:49 |
| leonardr | intellectronica: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346 | 16:53 |
| intellectronica | leonardr: looks good, as far as i can understand | 16:56 |
| intellectronica | leonardr: can you explain briefly how the grok thing works and why it's necessary? | 16:57 |
| leonardr | intellectronica: sure | 16:57 |
| leonardr | the way we use grok, it checks to see if you subclassed a certain class, and then performs some magic | 16:57 |
| leonardr | in this case, the magic looks like this: | 16:59 |
| leonardr | http://pastebin.ubuntu.com/376972/ | 16:59 |
| leonardr | intellectronica: so when the magic code runs, we get an autogenerated marker interface for each version of the web service | 17:00 |
| leonardr | IWebServiceClientRequestVersion_beta | 17:00 |
| leonardr | IWebServiceClientRequestVersion_1_0 | 17:00 |
| intellectronica | gotcha | 17:00 |
| leonardr | IWebServiceClientRequestVersion_devel | 17:00 |
| intellectronica | leonardr: r=me | 17:00 |
| leonardr | cool | 17:00 |
| leonardr | now to get it all together and run the test | 17:01 |
| intellectronica | good luck | 17:01 |
| === 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 | ||
| intellectronica | adiroiban: i apologize, but i probably won't have time to review your branch today | 17:03 |
| intellectronica | it's quite a big branch | 17:04 |
| adiroiban | intellectronica: no hurry, I just put in there to be reviewed when someone has a bit of free time | 17:04 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!