[10:13] <henninge> jtv: are you ok with a short cover letter?
[10:14] <jtv> henninge: sure, just mention anything that might be surprising.
[10:14]  * henninge surprises himself sometime ... ;)
[10:15] <henninge> *sometimes*
[10:24] <jtv> Oops, my gnome just screwed itself up.
[10:55] <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:59] <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!
[11:01] <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:02] <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:03] <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:09] <jtv> henninge: have a look at TestCaseWithFactory.useTempDir
[11:10] <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:11] <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:12] <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:13] <henninge> right, wonder who reviewed that line before ...
[11:13] <henninge> :-P
[11:14] <henninge> jelmer: the other mp has a merge conflict
[11:16] <henninge> jelmer: I guess you need to merge a current devel and push the branch again.
[11:18] <jelmer> henninge: will do
[11:22] <jtv> henninge: could you replace the comments in TestDetectIntltoolInBzrTree with less confusing ones?
[11:26] <henninge> jtv: oh, sorry, copy-and-paste error ... ;)
[11:27] <jtv> henninge: Good idea to have the "with" here.
[11:45] <jtv> henninge: it'd be nice to have some kind of clear distinction between "server-safe" and "slave-side" code.
[11:46] <henninge> jtv: yes, I've been wondering about that. Should I move it to its own module?
[11:49] <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:50] <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:54] <jtv> So I'd suggest not splitting this up into modules until a pattern emerges.
[11:59] <jtv> henninge: Can you come up with another clear way to mark the unsafe code?
[12:00] <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:02] <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:03] <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:04] <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:07] <henninge> jelmer: no test?
[12:08] <jelmer> henninge, unfortunately, no :-/ I couldn't find a good way to test this.
[12:44]  * henninge lunches
[13:06] <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:11] <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:21] <bigjools> wgrant: still around?
[13:56] <adiroiban> -/n
[13:57] <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
[14:07] <henninge> adiroiban: looking
[14:20] <henninge> adiroiban: you need to merge a current devel, first. Your branch seeems to be rotting.
[14:22] <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:23] <adiroiban> I guess I will have to wait for Danilo to merge that branch and then continue my work
[14:25] <henninge> adiroiban: yes and you should finish the pre-imp discussion with him, I guess.
[14:26] <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:28] <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:35] <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:36] <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:38] <jtv> intellectronica: https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19335
[14:38] <intellectronica> jtv: cheers
[14:39] <intellectronica> jtv: in extractBuildStatus, how about raising a ValueError instead of asserting?
[14:40] <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:41] <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:42] <intellectronica> oooh i like FakeMethod
[14:43] <intellectronica> it's a nice thing to have
[14:44] <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:48] <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:49] <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:54] <intellectronica> jtv: r=me. great work.
[14:54] <jtv> intellectronica: thanks!  No mean compliment from you.  :)
[14:58] <leonardr> henninge or intellectronica: please review https://code.edge.launchpad.net/~leonardr/lazr.restful/exported-subclass/+merge/19337
[14:59] <intellectronica> leonardr: i'll take it
[15:01] <intellectronica> leonardr: r=me
[15:12] <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:36] <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.
[16:35] <leonardr> intellectronica, i need a launchpadlib and a launchpad branch reviewed in tandem
[16:36] <intellectronica> leonardr: oright
[16:37] <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:38] <leonardr> intellectronica: on the way
[16:38] <intellectronica> cool
[16:44] <leonardr> intellectronica: https://code.edge.launchpad.net/~leonardr/launchpadlib/multiversion/+merge/19343
[16:49] <intellectronica> leonardr: r=me
[16:49] <leonardr> cool
[16:49] <leonardr> working on launchpad one now
[16:53] <leonardr> intellectronica: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346
[16:56] <intellectronica> leonardr: looks good, as far as i can understand
[16:57] <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:59] <leonardr> in this case, the magic looks like this:
[16:59] <leonardr> http://pastebin.ubuntu.com/376972/
[17:00] <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:01] <leonardr> now to get it all together and run the test
[17:01] <intellectronica> good luck
[17:03] <intellectronica> adiroiban: i apologize, but i probably won't have time to review your branch today
[17:04] <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