/srv/irclogs.ubuntu.com/2010/08/17/#launchpad-reviews.txt

=== Ursinha is now known as Ursinha-afk
=== Ursinha-afk is now known as Ursinha
thumpermwhudson: if you could take a look at http://bazaar.launchpad.net/~thumper/launchpad/new-bzr/revision/1105805:52
thumpermwhudson: I've reviewed the rest of the bzr 2.2 upgrade branch as it was done by abentley05:52
thumpermwhudson: but that revision is the only non-merge one05:52
thumperfrom me05:53
thumperI'm running it throug ec2 now (test only)05:54
mwhudsonthumper: assuming it makes the tests work, it looks ok to me05:55
thumperhttps://code.edge.launchpad.net/~thumper/launchpad/new-bzr/+merge/32845 is the entire MP05:55
thumperthe tests all pass with flying colours05:55
mwhudsonthumper: reviewed, then05:56
thumperta05:56
=== jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvhenninge: approved09:33
henningejtv: thank you very much! ;)09:33
jtvnp09:34
=== gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
gmbjtv, https://code.edge.launchpad.net/~jtv/launchpad/recife-approveSuggestion/+merge/32848 is it?10:04
jtvgmb: yes, that's it10:10
gmbRighto10:10
=== gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb should really write an x-chat plugin to deal with the topic in this channel.10:10
jtvWe could write a chat server for Launchpad…10:12
jtvgmb: I do unfortunately have to run off very soon.10:13
gmbjtv, No worries, we can do any back-and-forth by email.10:14
jtvgmb: thanks10:17
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/milestones/+merge/3285510:40
=== lifeless changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvgmb: still getting used to adding more comments to unit tests… I'd like them to be shorter and have simpler subject material so I could make the code speak for itself, but doesn't always work.11:00
gmbjtv, Yeah; All I'm after is a comment (or docstring; I prefer comments but have no reason for it) at the start of the test telling me the expected behaviour.11:02
jtvgmb: doing that now11:03
gmb# If the foo is prodded the frobnob goes boing.11:03
gmbAnd so on.11:03
gmbjtv, Cool.11:03
jtvthanks for the review btw.11:03
gmbnp11:03
gmbjtv, Thanks for the awesome cover letter that saved me a lot of confusion :)11:03
jtvGlad to know the effort's not wasted!11:03
gmbIndeed.11:03
=== 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: thanks11:47
gmbnp11:49
cjwatsongmb: I'd like to have a pre-impl chat about bug 61915212:22
gmbcjwatson, I think you'd be better off chatting to one of { bigjools, jelmer, noodles775 }; I have this much context on Soyuz: ><12:26
cjwatsonbigjools: ^- would you have a moment?12:26
* StevenK idly notes he isn't in that list :-P12:27
gmbStevenK, I assumed that you were sleeping or otherwise engaged.12:28
StevenKTrying to stop working. Failing. :-/12:28
cjwatsonwell, this is *cough* "pre"-impl, I have a branch and would quite like a sanity check that it covers everything I need; especially as running ec2test involves some pile of local setup I haven't managed to do12:30
gmbcjwatson, *definitely* one for the Soyuz team, then. Sanity is defined differently once you enter lp.soyuz.12:31
wgrantNo, it's undefined :/12:32
cjwatsonlp:~cjwatson/launchpad/dpkg-xz-support-61915212:33
jelmercjwatson: Hi12:33
cjwatsonhi12:33
jelmercjwatson: I can do a review and run your branch through ec2 if you like.12:35
cjwatsonthat would be awesome12:35
jelmercjwatson: Looks great12:42
jelmercjwatson: I'm wondering though how much policy enforcement we should be doing in nascentuploadfile.12:43
jelmerIt would be nice if we could do this sort of testing by enforcing a few lintian errors, as dak is doing.12:44
jelmerbigjools: Is there a particular reason we aren't doing more checking of control fields in nascentupload?12:46
jelmercjwatson: Before this can land, we also need to get a newer version of dpkg deployed on the Launchpad servers.12:50
cjwatsonjelmer: the policy enforcement there is just a copy of what we used to do when the other compression formats were new13:00
cjwatsonjelmer: (not that this really answers your point as such)13:00
cjwatsonI actually literally resurrected it from old commits13:00
cjwatsonjelmer: would you like me to take care of getting a new dpkg in place?13:01
cjwatsonmay take a while ...13:01
jelmercjwatson: Ah, sorry. I wasn't aware there had been something like that previously.13:02
jelmercjwatson: I can take care of it if you like; I need to upload some updates to the Launchpad PPA anyway.13:02
jelmercjwatson: If you can propose a merge into lp:launchpad/devel I'll do a proper review as well landing this.13:03
cjwatsonOK, either way.  Do you know the stuff that's required for hardy-cat backporting?13:03
jelmerHmm, I hadn't considered hardy-cat actually, was just thinking about the launchpad servers (which do unpacking).13:04
cjwatsonhardy-cat is how dpkg gets to them13:05
wgrantOr is Lucid happening soon enough that that's irrelevant?13:05
cjwatsonlucid doesn't have dpkg 1.15.6, so it would be an issue either way13:06
jelmerwgrant: Lucid's dpkg is too old as well if I understand correctly. (1.15.5 vs 1.15.6)13:06
cjwatsonthe backport diff to hardy-cat is http://paste.ubuntu.com/479368/; there was an extra upload by LaMont after that as well, but that was a backport from 1.15.6 so I think it can be discarded13:06
cjwatsonhttp://paste.ubuntu.com/479369/ is the complete diff from hardy to hardy-cat currently13:07
jelmerdo the launchpad servers run hardy-cat as well, not just the buildd's ?13:07
cjwatsonyes13:07
=== mrevell is now known as mrevell-lunch
=== matsubara-afk is now known as matsubara
cjwatsongah, dpkg gets harder and harder to backport - now I have to care about gettext13:26
cjwatsonmaybe it would be easier just to cherry-pick the change in question13:27
jelmerhow intrusive is the patch?13:28
cjwatsonnot hugely13:32
cjwatsonwill still need to get xz-utils into hardy-cat though13:33
cjwatsonhm, there was some packaging awkwardness there, I recall13:34
cjwatsonshould be OK with maverick's version13:34
=== mars changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== fjlacoste is now known as flacoste
=== mrevell-lunch is now known as mrevell
jelmerhi gmb, mars14:24
gmbjelmer, Howdy.14:24
jelmercan I add a simple soyuz branch to either of your queues?14:24
gmbjelmer, How small is it?14:24
gmbI ask because I need to go afk for 1:30 at about 14 UTC14:25
jelmerIt's 458 lines, but pretty much only adds tests for existing code14:25
gmbjelmer, Okay, I'll take a look now. Diff me.14:25
jelmergmb: thanks - mp is @ https://code.edge.launchpad.net/~jelmer/launchpad/nascentuploadfile-tests/+merge/3287414:25
=== gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: jelmer, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
gmbjelmer, Review sent; quite a lot of cosmetic changes needed (callsite formatting and tests that need a comment).14:42
gmbBut the code is sound; nice work.14:42
jelmergmb: Thanks for the review.14:42
gmbnp14:43
* gmb goes off review for a while for travelling.14:43
=== gmb changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjimars: I have a couple revisions to an already-approved branch that need review: http://bazaar.launchpad.net/~benji/launchpad/bug-597324/revision/11117 and http://bazaar.launchpad.net/~benji/launchpad/bug-597324/revision/1111815:07
jelmerWhat should the per-line imports look like exactly? The python style guide doesn't appear to've been updated yet.15:22
marsjelmer, mister review master wrangler back may know15:25
mars'bac' even15:25
bacjelmer: no, it hasn't15:25
marsbenji, sure, please add yourself the queue15:25
=== benji changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bacjelmer: we agreed that single imports remain on one line, and that more than one require multiple lines, one per15:26
bacjelmer: we did not discuss alphabetizing, so i assume that criterion has not been relaxed15:26
jelmershould the closing ")" be on a separate line?15:27
bacyes15:27
jelmerok, that was the main thing I wasn't sure about - thanks15:27
bacwith a trailing comma on the last item15:27
bacunfortunately, the ) on a separate line irritates our linter15:27
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [benji²] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjibac: I missed that decision; was it in the email thread [Launchpad-dev] RFC: format lists one-per line ?15:28
bacbenji: it was in the reviewers meeting and there was an email discussion.  i think you contributed to it.15:30
bacbenji: i do need to send an email, though, finalizing the decision15:31
benjibac: right, I hadn't seen a finalization email; thanks15:31
bacbenji: that's b/c i am slack15:32
benjiheh15:32
marsUrsinha, merge is ready: https://code.edge.launchpad.net/~mars/qa-tagger/lazr-conversion/+merge/3287815:54
Ursinhamars, thanks!15:54
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: benji || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjimars: should I add another MP for those revisions?  I'm not sure how to handle the situation when a branch has been approved and then more changes have to be made to it.15:59
marsbenji, usually we just ask for another review explicitly for the changed revisions15:59
marsand add it to the existing MP16:00
marsbenji, what is the MP?16:00
benjimars: https://code.edge.launchpad.net/~benji/launchpad/bug-597324/+merge/3082916:00
marsthanks16:01
marsbenji, I think you need a new MP.  The old one was not updated with your latest changes16:06
marsit looks like you can indeed keep hacking on the same branch16:06
marsbenji, there is also a big revision gap: the MP ends at 112, and you asked for 117 and 118 to be reviewed.  Where did the others go?16:07
benjimars: one was a merge, two were trivial, and one should have been included (and is in the MP I'm writing right now)16:08
marsbenji, ok, then I think a new proposal should just Do The Right Thing16:09
benjimars: here you go: https://code.edge.launchpad.net/~benji/launchpad/bug-597324/+merge/3288416:12
marsthanks16:13
marsbenji, you may want to try 'bzr qannotate' to browse publication.py and figure out what the intent of the 'appears to be a XSRF countermeasure' thing is16:14
marsor bzr explorer16:14
benjiI'll look at that now.16:14
marsbenji, db-devel is the right target?16:14
benjidarn; no it's not16:15
benjimars: let me delete and re-create the MP so it has the right target16:15
marsok16:15
benjimars: https://code.edge.launchpad.net/~benji/launchpad/bug-597324/+merge/3288616:16
marsok16:17
benjimars: did you mean "bzr qannontate" or just "bzr annotate"?16:25
marsbenji, a method has 40 lines of comments to 3 lines of code - I don't know what to say16:25
benjimars: ?16:26
marsbenji, there is a qbzr plugin that gives gtk versions of some bzr commands - qannotate and qlog are really nice versions of the set16:26
benjimars: oh, the referer exception  thing16:26
benjiok; I'm more of a text kind of guy :)16:27
marsbenji, no, the text is good16:27
marsit's the very fact that a small book must be written to explain the purpose of a single call to 'return' that I find... strange16:28
marssounds like something NASA would do16:28
=== salgado is now known as salgado-lunch
benjiheh16:30
marsbenji, your comments there are fine.  I'm surprised that that one block of code has so many XXXs and so much text16:32
benjimars: speaking of NASA, I'm sure you've read this http://www.fastcompany.com/node/28121/print; if not, you'll enjoy it16:32
benjiyeah, it's a bit of an oddity16:33
benjimaybe we should start a contest for the most well-documented bare return16:33
marsbenji, yep, that article is what inspired my NASA comment :)16:34
benjicool16:34
benjiI like to read that every year or so; lots of good stuff in there16:34
marsfor me that article perfectly demonstrates one of the three faultlines (or continents) in the software world: hackers, enterprise, and academic16:35
marsthey all hate each other16:35
marsand insist that if the others just did it their way, the world would be a better place :)16:35
benjiheh16:37
jmlmars, "they"?16:37
marsjml, ?16:38
mars'they all' => 'they'16:38
marsmaybe?16:38
jmlmars, surely "we" would be more appropriate :)16:38
mars:D16:39
benjiI would hope that the NASA folks realize that very few people can afford to put 260 people on building something about the size of LP.  I'd also hope that the hacker type (which I count my self as) realize that when lives depend on your work product you have to be quite bit more particular about processes.16:39
jmlmars, unless you're hiding on some uncharted fourth continent :P16:39
benjiheh16:40
benjijourney to the center of mars16:40
marsbenji, then I count thee as enlightened16:40
benjiok, mars: I was able to verify that the referer-rejection code is indeed about XSRF; I'll update the comment to be a statement instead of a question.16:41
marscool16:42
marsbac, are we transitioning slowly to multi-line imports, or is someone going to nuke the source tree with a macro over the weekend?16:43
bacmars: unclear16:43
bacmars: the code team seemed to indicate they were going to rush off and do theirs quickly16:43
bacmars: no one volunteered for an apocalyptic branch16:44
marsbac, I'm reviewing benji's branch, and he worked with the imports in test_publication.py.  Should the ones he worked with be updated to multiline as part of the policy?16:44
marsnot suggesting the whole file be updated16:44
bacmars: i'd suggest not requiring it until we get some tool support.16:45
marsperfect16:45
marsthanks back16:45
marsthanks bac16:45
marstwice today, argh16:45
bacthere is an emacs 'sort-imports' and i think vim has something similar.16:45
bacthey should be updated16:45
marspersonally I would like a Python or sed script16:45
bacmars: that too16:46
marsbenji, nice test suite addition16:47
benjicool16:47
marsbenji, done, one errant comment, r=mars with the fix16:55
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
marsoff to lunch!16:55
benjimars: thanks16:55
=== Ursinha is now known as Ursinha-lunch
=== matsubara is now known as matsubara-lunch
EdwinGrubbsmars: can I get a small branch reviewed? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-fillTeamParticipation-timeout/+merge/3289517:30
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== salgado-lunch is now known as salgado
=== benji is now known as benji-lunch
bdmurraymars: Could you review https://code.edge.launchpad.net/~brian-murray/launchpad/x-launchpad-bug-modifier-follow-on/+merge/3290018:16
=== matsubara-lunch is now known as matsubara
=== Ursinha-lunch is now known as Ursinha
=== benji-lunch is now known as benji
marsXorg server crash19:13
=== benji changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [Edwin, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjiI hate those.19:13
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [Edwin, bdmurray, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: Edwin || queue: [bdmurray, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjimine is at https://code.edge.launchpad.net/~benji/launchpad/bug-580035/+merge/3261519:14
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: Edwin || queue: [bdmurray, benji, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
marscool, thanks19:14
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: bdmurray || queue: [benji, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
marsjcsackett, feel free to put your name in the topic and leave a PM with the merge proposal link19:28
jcsackettmars: okay, thanks.19:28
=== jcsackett changed the topic of #launchpad-reviews to: On call: mars || reviewing: bdmurray || queue: [benji, jml, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
marswell, PM isn't right, just a message19:29
marseither myself or a passer-by will pick it up19:29
jcsackettmars: okay.19:29
jcsackettmars: mp is at https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/3282019:30
jcsackettmars: thanks so much!19:30
marsno problem19:31
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: benji || queue: [jml, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
marsbenji, ping, looks like bug-580035 has a merge conflict20:13
marsbenji, merged with db-devel again20:14
marsEdwinGrubbs, btw, in your branch, for the routines that the SQL query replaced: was that their only callsite?  If so, can the routines be eliminated?20:22
EdwinGrubbsmars: what do you mean eliminated?20:23
marsEdwinGrubbs, ah, I guess not.  Lines 97-100 of the diff20:23
marsthose are probably used elsewhere20:24
mars:)20:24
EdwinGrubbsmars: oh, you mean whether hasParticipationEntryFor can be eliminated. good question.20:25
benjiok, mars: the conflict is fixed and a new MP with the right target is at https://code.edge.launchpad.net/~benji/launchpad/bug-580035/+merge/3291720:26
EdwinGrubbsit looks like theat is getting used in asserts and tests, so it would be a pain to remove.20:26
marsthanks benji20:27
marsEdwinGrubbs, then it still sounds useful to me20:27
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: jcsackett || queue: [jml, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== mars 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
marsalright, I'm done for the day.  Take care everyone21:20
leonardrgary, can you take a quick look at https://code.edge.launchpad.net/~leonardr/launchpadlib/fix-test-failure/+merge/32925 ?21:31
gary_posteron call leonardr21:31
leonardrnp, i'll wait. i have an ec2 test running and i'll come back tomorrow to see if this solved all the problems or only one21:37
=== salgado is now known as salgado-afk
=== matsubara is now known as matsubara-afk
gary_posterapproved leonardr23:05

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