=== gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [09:55] gmb: hi [09:55] since you're here... [09:55] Morning lifeless. What can I do for you? [09:55] your timeout bug for prod [09:55] is it ready to CP ? [09:56] lifeless, I believe so; let me check... [09:57] have you seen the hourly oops graph :P [10:00] lifeless, So yes, it's been merged, but into db-devel (I must've chosen the wrong target for the mp), which is annoying. So: [10:00] 1) I need to prepare a branch to merge it into devel [10:00] yes, yes you do :) [10:00] 2) I need to do the same for production-stable [10:00] (Or whatever our CP target is these days) [10:01] have you checked on staging too see if its faster ? [10:01] thats the really interesting thing for me [10:01] lifeless, It's not on staging yet. We'll have to cowboy it there. I'll grab mthaddon in a few minutes and get that done. [10:02] \o/ [10:08] lifeless, Oh, hang on. It *is* in devel. I guess LP gets confused by the auto-merge from devel to db-devel. [10:08] So, that's a win. [11:56] review wanted: https://code.edge.launchpad.net/~jml/launchpad/remove-interface-import/+merge/30375 === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:59] jml, i got it [12:00] leonardr, thanks. [12:00] * jml off to lunch. back soon. [12:07] gmb, leonardr: time for this one? Just extracting something to a file of its own. https://code.edge.launchpad.net/~jtv/launchpad/xpi-dtd-parser/+merge/30383 === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: jml, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:08] jtv, i'm here all day [12:08] leonardr: great, thanks === mrevell is now known as mrevell-lunch === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:33] jtv, just to satisfy my curiosity, what do we use the xpi dtd parser for? === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.om/ || https://code.launchpad.net/launchpad/+activereviews [13:06] jtv, r=me === mrevell-lunch is now known as mrevell [13:21] leonardr: thanks for the review [13:21] np [13:55] Anyone around for a review? [13:55] gmb, leonardr? [13:56] rockstar: yo [13:57] lifeless, you want to do a review? [13:58] I've done 25 or so in the last 24 hours [13:58] lifeless, https://code.edge.launchpad.net/~rockstar/launchpad/unsupported-series-recipe-api/+merge/30397 [13:58] so why not :P [13:58] rockstar: check the +activereviews page :) [13:59] lifeless, that's funny, because I've been thinking of changing the way I do reviews. [13:59] is 400 the right error to return ? [14:00] lifeless, that's what everything else returns. [14:00] oh! [14:00] * lifeless adds that to the list of things to look at [14:00] I don't even know what 400 is. [14:00] Ah, Bad Request, I think that's right. [14:00] 'The request could not be understood by the server due to malformed syntax. The client SHOULD NOT repeat the request without modifications' [14:00] its not malformed syntax [14:01] lifeless, hm, maybe. I guess REST does try and re-use HTTP descriptions for itself, so I can see the reasoning there. [14:02] 409/412 maybbe [14:02] anyhow [14:02] you're following the pattern [14:02] thats fine [14:03] 412 makes more sense than 409 [14:03] Also, it's funny that if I google for "http 40x" the fourth item returned is xkcd... [15:02] gmb, leonardr: I need a taker for a cleanup branch that's over twice the review limit. Any volunteers? [15:02] https://code.edge.launchpad.net/~jtv/launchpad/test-cruft-soyuz/+merge/30407 [15:02] jtv, i'll look at it [15:02] Great, thanks. It's basically lint. [15:02] Just lots of it. [15:02] jtv, out of curiosity, why didn't you do them in small increments? [15:03] (I've been thinking about proposing a lower limit to review diff sizes, so I'm trying to find out the cause of outliers) [15:03] rockstar: it went rather faster than I expected—first you remove some boilerplate from the tests, then you run "make lint"... [15:03] ...and suddenly you're at 1,400 lines & growing. [15:03] jtv, have you used bzr-pipes at all? [15:03] This is rather atypical I hope. [15:04] jtv, yeah, but I think what sinzui talked about in his lightning talk is probably also the secret to my small amount of success. [15:04] Once or twice, but I was scared off by two things: one was some kind of version skew that broke the plugin for me locally, and the other was that I need to look up how it works every time—and then Aaron tells me what I find in the documentation isn't actually how it should be done. [15:04] So maybe saying "No branch more than 500 lines of diff" would be helpful for everyone's velocity. [15:05] That is what U1 did [15:05] jtv, I'm happy to sit down with you and show you how I do it. It's a lot easier than one might think. [15:05] sinzui, really? That's interesting. [15:05] barry fixed his productivity by timeboxing. [15:05] sinzui, I'm curious to know what you think about my idea. [15:06] rockstar: I know—I've done it. But I don't do it _quite_ frequently enough to know that I won't screw it up next time. If you could go over the online docs and make sure they're correct, that would be more valuable IMHO [15:06] jtv, yeah, that's probably a good idea. [15:06] I think the limit of 800 was very helfpul, though sometimes you do just have to go over. [15:06] jtv, yeah, I'm curious if those "you just have to" situations really could be avoided more if we planned for it. [15:07] 2/4/6 hours should be enough to produce something valuable. also. I have take 2 hours to make 2000 lines of change moving something to lp.registry. The time was the correct limit [15:07] sinzui, yeah, although I'd contend that 6 hours might be a little high for most cases. [15:07] I divide my days up into 3 hour increments. [15:08] I use 2 hours, and I get nervous when I reach 4. I start looking for ways to end the branch immediately [15:09] sinzui, yeah, but I've noticed that if I'm doing anything that might touch soyuz stuff, it takes a bit longer, because I don't know all of their gotchas just yet. [15:09] (I know all the code gotchas, and how to avoid them) [15:09] rockstar: *we* don't know all of the gotchas yet [15:10] rockstar, I factor that into getting something ready to code, and I use that time to refactor code so that it is clear. tech-debt amounts to a lot of bugs in Lp. [15:11] * bigjools is going to have a 1200 line branch soon. No way around it. [15:11] sinzui, huh. That's interesting as well. A prep branch to go before the actual branch. [15:12] Salgado does a lot of them. He seems to use it to make the real task easier to do, or at least to remove any distractions from his primary task [15:14] sinzui, yeah, it's the distraction thing that annoys me. [15:14] sinzui, I've been performing an experiment with my branch workflow, and my biggest patch is ~450 lines, so I think we really can break them up more. [15:15] jtv, does lint really complain that there should be a space in ('foo',) ? [15:15] i never knew that [15:15] leonardr: yup... I discussed with sinzui who confirmed that PEP8 wants it that way. [15:15] (it surprised me as well :-) [15:15] Maybe this is a conversation for the review's meeting. smaller branches or shorter development time. [15:15] jtv, I've ignored that (it's stupid...) [15:16] rockstar, you should submit a patch to pep8.py them [15:16] sinzui, yeah, before I brought it to the reviewer's meeting, I wanted my own experiment. [15:16] then [15:17] Usually, the reviewer's meeting can turn into yak shaving/bikeshedding, so I wanted actual facts (similar to your lightning talk) [15:27] jtv, the blank line on line 627 of your patch looks strange. is it correct? [15:28] leonardr: looking... [15:29] jtv: ditto for line 811 [15:29] leonardr: yeah... those are how "make lint" wants them. [15:30] jtv: blank line before any function definition? [15:30] Yes, looks like. [15:32] jtv, r=me [15:32] leonardr: thanks! I'll try to bother gmb for the next one, because this just ain't fair. :-) [15:33] Yes. === deryck is now known as deryck[lunch] [15:47] jtv, for the record i got some test failures when i ran the soyuz tests, but they kind of look like a problem with my larger setup. i can paste them if you want [15:48] leonardr: if they're MissingCelebrityErrors, that's a devel change and "make schema" will help. [15:48] jtv: no, it started as a GpgmeError ("unknown error code") and then cascaded from there [15:49] leonardr: I think I had those a bit longer ago, and rocketfuel-{setup,get} or something made it go away. [15:49] Do you get them in devel? [15:49] jtv: i don't know. sorry--i'm fighting another fire and don't want to context-switch unless this is really important to you [15:50] leonardr: not particularly—ec2 will be my judge [15:50] ok [15:50] gmb: here's one I'd like to ask your review for: https://code.edge.launchpad.net/~jtv/launchpad/exportstorage-cleanup/+merge/30418 [15:52] jtv, Righto [15:52] fab === gmb changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: jtv, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:56] jtv, Is the VWS on line 61 really necessary? [15:56] gmb: make lint says so [15:59] jtv, Garh. [15:59] Okay. [15:59] I'm not wholly sure the new linter isn't a bit crackful. [15:59] It's a lot stricter—which by and large is a good thing, I think [16:00] jtv, True. [16:00] jtv, Anyway, r=me.\ [16:00] \o/ thanks === gmb changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === deryck[lunch] is now known as deryck [19:15] leonardr: it's still highly unfair that your co-reviewer buggered off before I could be as demanding with him as I was with you... got room for a few more? [19:15] I have one small lint cleanup: https://code.edge.launchpad.net/~jtv/launchpad/pre-export-cleanup/+merge/30445 [19:17] jtv, i see a conflict in potemplate.py [19:17] leonardr: thanks... I'll update my branch & re-push. Hang on. [19:22] leonardr: to keep you updated: I pushed a while ago but am still polling for the diff to update. Will let you know. [19:23] leonardr: it's updated. [19:28] jtv, r=me [19:28] leonardr: cool, thanks === leonardr changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:28] * leonardr will be back in a couple hours in case no one else wants to do reviews [19:29] I'll ask around. Thanks for all the reviews today! === danilos is now known as daniloff === leonardr is now known as leonardr-afk === leonardr-afk is now known as leonardr === flacoste is now known as flacoste_afk