/srv/irclogs.ubuntu.com/2010/07/20/#launchpad-reviews.txt

=== 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
lifelessgmb: hi09:55
lifelesssince you're here...09:55
gmbMorning lifeless. What can I do for you?09:55
lifelessyour timeout bug for prod09:55
lifelessis it ready to CP ?09:55
gmblifeless, I believe so; let me check...09:56
lifelesshave you seen the hourly oops graph :P09:57
gmblifeless, 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
gmb1) I need to prepare a branch to merge it into devel10:00
lifelessyes, yes you do :)10:00
gmb2) I need to do the same for production-stable10:00
gmb(Or whatever our CP target is these days)10:00
lifelesshave you checked on staging too see if its faster ?10:01
lifelessthats the really interesting thing for me10:01
gmblifeless, 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:01
lifeless\o/10:02
gmblifeless, Oh, hang on. It *is* in devel. I guess LP gets confused by the auto-merge from devel to db-devel.10:08
gmbSo, that's a win.10:08
jmlreview wanted: https://code.edge.launchpad.net/~jml/launchpad/remove-interface-import/+merge/3037511:56
=== 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
leonardrjml, i got it11:59
jmlleonardr, thanks.12:00
* jml off to lunch. back soon.12:00
jtvgmb, 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/3038312:07
=== 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
leonardrjtv, i'm here all day12:08
jtvleonardr: great, thanks12:08
=== 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
leonardrjtv, just to satisfy my curiosity, what do we use the xpi dtd parser for?12:33
=== 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
leonardrjtv, r=me13:06
=== mrevell-lunch is now known as mrevell
jtvleonardr: thanks for the review13:21
leonardrnp13:21
rockstarAnyone around for a review?13:55
rockstargmb, leonardr?13:55
lifelessrockstar: yo13:56
rockstarlifeless, you want to do a review?13:57
lifelessI've done 25 or so in the last 24 hours13:58
rockstarlifeless, https://code.edge.launchpad.net/~rockstar/launchpad/unsupported-series-recipe-api/+merge/3039713:58
lifelessso why not :P13:58
lifelessrockstar: check the +activereviews page :)13:58
rockstarlifeless, that's funny, because I've been thinking of changing the way I do reviews.13:59
lifelessis 400 the right error to return ?13:59
rockstarlifeless, that's what everything else returns.14:00
lifelessoh!14:00
* lifeless adds that to the list of things to look at14:00
rockstarI don't even know what 400 is.14:00
rockstarAh, Bad Request, I think that's right.14:00
lifeless'The request could not be understood by the server due to malformed syntax. The client SHOULD NOT repeat the request without modifications'14:00
lifelessits not malformed syntax14:00
rockstarlifeless, hm, maybe.  I guess REST does try and re-use HTTP descriptions for itself, so I can see the reasoning there.14:01
lifeless409/412 maybbe14:02
lifelessanyhow14:02
lifelessyou're following the pattern14:02
lifelessthats fine14:02
rockstar412 makes more sense than 40914:03
rockstarAlso, it's funny that if I google for "http 40x" the fourth item returned is xkcd...14:03
jtvgmb, leonardr: I need a taker for a cleanup branch that's over twice the review limit.  Any volunteers?15:02
jtvhttps://code.edge.launchpad.net/~jtv/launchpad/test-cruft-soyuz/+merge/3040715:02
leonardrjtv, i'll look at it15:02
jtvGreat, thanks.  It's basically lint.15:02
jtvJust lots of it.15:02
rockstarjtv, out of curiosity, why didn't you do them in small increments?15:02
rockstar(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
jtvrockstar: it went rather faster than I expected—first you remove some boilerplate from the tests, then you run "make lint"...15:03
jtv...and suddenly you're at 1,400 lines & growing.15:03
rockstarjtv, have you used bzr-pipes at all?15:03
jtvThis is rather atypical I hope.15:03
rockstarjtv, 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
jtvOnce 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
rockstarSo maybe saying "No branch more than 500 lines of diff" would be helpful for everyone's velocity.15:04
sinzuiThat is what U1 did15:05
rockstarjtv, 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
rockstarsinzui, really?  That's interesting.15:05
sinzuibarry fixed his productivity by timeboxing.15:05
rockstarsinzui, I'm curious to know what you think about my idea.15:05
jtvrockstar: 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 IMHO15:06
rockstarjtv, yeah, that's probably a good idea.15:06
jtvI think the limit of 800 was very helfpul, though sometimes you do just have to go over.15:06
rockstarjtv, yeah, I'm curious if those "you just have to" situations really could be avoided more if we planned for it.15:06
sinzui2/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 limit15:07
rockstarsinzui, yeah, although I'd contend that 6 hours might be a little high for most cases.15:07
rockstarI divide my days up into 3 hour increments.15:07
sinzuiI use 2 hours, and I get nervous when I reach 4. I start looking for ways to end the branch immediately15:08
rockstarsinzui, 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
rockstar(I know all the code gotchas, and how to avoid them)15:09
bigjoolsrockstar: *we* don't know all of the gotchas yet15:09
sinzuirockstar, 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:10
* bigjools is going to have a 1200 line branch soon. No way around it.15:11
rockstarsinzui, huh.  That's interesting as well.  A prep branch to go before the actual branch.15:11
sinzuiSalgado 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 task15:12
rockstarsinzui, yeah, it's the distraction thing that annoys me.15:14
rockstarsinzui, 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:14
leonardrjtv, does lint really complain that there should be a space in ('foo',) ?15:15
leonardri never knew that15:15
jtvleonardr: yup... I discussed with sinzui who confirmed that PEP8 wants it that way.15:15
jtv(it surprised me as well :-)15:15
sinzuiMaybe this is a conversation for the review's meeting. smaller branches or shorter development time.15:15
rockstarjtv, I've ignored that (it's stupid...)15:15
sinzuirockstar, you should submit a patch to pep8.py them15:16
rockstarsinzui, yeah, before I brought it to the reviewer's meeting, I wanted my own experiment.15:16
sinzuithen15:16
rockstarUsually, the reviewer's meeting can turn into yak shaving/bikeshedding, so I wanted actual facts (similar to your lightning talk)15:17
leonardrjtv, the blank line on line 627 of your patch looks strange. is it correct?15:27
jtvleonardr: looking...15:28
leonardrjtv: ditto for line 81115:29
jtvleonardr: yeah... those are how "make lint" wants them.15:29
leonardrjtv: blank line before any function definition?15:30
jtvYes, looks like.15:30
leonardrjtv, r=me15:32
jtvleonardr: thanks!  I'll try to bother gmb for the next one, because this just ain't fair.  :-)15:32
gmbYes.15:33
=== deryck is now known as deryck[lunch]
leonardrjtv, 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 want15:47
jtvleonardr: if they're MissingCelebrityErrors, that's a devel change and "make schema" will help.15:48
leonardrjtv: no, it started as a GpgmeError ("unknown error code") and then cascaded from there15:48
jtvleonardr: I think I had those a bit longer ago, and rocketfuel-{setup,get} or something made it go away.15:49
jtvDo you get them in devel?15:49
leonardrjtv: i don't know. sorry--i'm fighting another fire and don't want to context-switch unless this is really important to you15:49
jtvleonardr: not particularly—ec2 will be my judge15:50
leonardrok15:50
jtvgmb: here's one I'd like to ask your review for: https://code.edge.launchpad.net/~jtv/launchpad/exportstorage-cleanup/+merge/3041815:50
gmbjtv, Righto15:52
jtvfab15:52
=== 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
gmbjtv, Is the VWS on line 61 really necessary?15:56
jtvgmb: make lint says so15:56
gmbjtv, Garh.15:59
gmbOkay.15:59
gmbI'm not wholly sure the new linter isn't a bit crackful.15:59
jtvIt's a lot stricter—which by and large is a good thing, I think15:59
gmbjtv, True.16:00
gmbjtv, Anyway, r=me.\16:00
jtv\o/ thanks16:00
=== 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
jtvleonardr: 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
jtvI have one small lint cleanup: https://code.edge.launchpad.net/~jtv/launchpad/pre-export-cleanup/+merge/3044519:15
leonardrjtv, i see a conflict in potemplate.py19:17
jtvleonardr: thanks...  I'll update my branch & re-push.  Hang on.19:17
jtvleonardr: to keep you updated: I pushed a while ago but am still polling for the diff to update.  Will let you know.19:22
jtvleonardr: it's updated.19:23
leonardrjtv, r=me19:28
jtvleonardr: cool, thanks19:28
=== 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
* leonardr will be back in a couple hours in case no one else wants to do reviews19:28
jtvI'll ask around.  Thanks for all the reviews today!19:29
=== 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

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!