#launchpad-reviews 2010-01-11
<al-maisan> hello BjornT, the MP in question is here: https://code.edge.launchpad.net/~al-maisan/launchpad/ibuilder-api-cleanup-505647/+merge/17122
<al-maisan> hello jml and stub, could you please review the schema patch proposed here: https://code.edge.launchpad.net/~al-maisan/launchpad/bq-platform-columns-505725/+merge/17125 ?
<rockstar> Anyone around for a review?
* 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
<gmb> henninge: Can i get a review for https://code.edge.launchpad.net/~gmb/launchpad/bug-heat-infrastructure-bug-503745/+merge/17137 please?
<gmb> Fixes bug 503745
<mup> Bug #503745: Add infrastructure for calculating bug heat <story-bug-heat> <Launchpad Bugs:In Progress by gmb> <https://launchpad.net/bugs/503745>
<henninge> gmb: Sure. ;)
<gmb> Thanks.
<henninge> gmb: That's a very brief cover letter for a 509 lines diff. Expect questions.
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> henninge: Sure; I look forward to them.
<henninge> :)
<henninge> gmb: First thing I notice: "heat" and "hotness" seem to be the same thing. From the ml discussion I saw, I think "heat" is the agreed term, isn't it?
<gmb> henninge: Yes.
<henninge> gmb: I think it'd prevent confusion if you iliminated the term "hotness" from the code, don't you think so, too?
<henninge> or are there other constraints?
<gmb> henninge: Hmm. Probably; I was sticking with our usual policy of reflecting DB fieldnames in the interface.
<gmb> henninge: No, this is brand new, so it makes no difference.
<henninge> so the db fieldname was named wrongly or prematurely?
<gmb> henninge: Prematurely.
<gmb> henninge: I agree, consistence in the code is better.
<gmb> I'll change hotness -> heat in the code.
<henninge> gmb: Great!
<gmb> (We can always patch the DB later to update the field name)
<henninge> thanks
<henninge> yup
* intellectronica changed the topic of #launchpad-reviews to: on-call: henninge, intellectronica || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> gmb: This line: bugs = list(getUtility(IBugSet).dangerousGetAllBugs()[start:end])
<gmb> henninge: Yes?
<gmb> henninge: Hmm. That list() seems unnecessary. Don't remember why I did that...
<henninge> gmb: I am not sure how storm does this. Are you sure you are not querying all bugs each time or does the slicing work on the storm iterator.
<gmb> Let me check to see if I can remove it.
<henninge> ?
<gmb> henninge: The slicing works on the iterator.
<henninge> gmb: ah, cool
<gmb> But the list() might be totally bogus.
<henninge> gmb: You have an "if bugs:" after that for which you might need it.
<henninge> gmb: But I don't like "if bugs:" anyway, or rather, our standards don't, afaik.
<henninge> ;)
<gmb> henninge: Hmm, yes, I suspect that's it. I'll see what happens when I remove the list().
<gmb> henninge: Right. That should be if bugs.any().
<gmb> (is not none) ;)
<gmb> henninge: I cribbed some of this from the bugtask targetnamecache updater work, so I think this is a copy-paste error on my part.
<henninge> gmb: either that or calculate the length into a local variable, you need it for the logger output anyway.
<henninge> I know the feeling.
<gmb> henninge: Good point.
<gmb> henninge: Ah; I need the list() so I can do bugs[0].
<henninge> is there no "bugs.first()" in storm?
<gmb> henninge: I'm about to find out :)
<gmb> henninge: There is, and it works :)
<henninge> cool
 * henninge just learned about test_doc.py ... 
<stub> gmb: Just added a comment to that MP to use DBLoopTuner instead of LoopTuner - its in the same file and you should use DBLoopTuner whenever doing bulk database updates.
<gmb> stub: Thanks, will do.
<stub> gmb: Also consider putting this in garbo-daily rather than a seperate script.
<gmb> stub: Ah, good thinking. I'll look into that.
<stub> You get some testing infrastructure for free and our daily jobs get run serially for best throughput and least pain. Should drop in easily enough since it is a tunable loop.
<gmb> Right.
<gmb> stub: Right, I think I'll do that, thanks. Seems like an easy win.
<mrevell> Hey henninge, any chance of a review on https://code.edge.launchpad.net/~matthew.revell/launchpad/translations-help-10.1/+merge/17032 when you have a moment?
<henninge> mrevell: np, but it will have to be after lunch.
<mrevell> thanks henninge
<henninge> mrevell: put it in the queue
<henninge> please
<henninge> ;)
<mrevell> henninge, It's there already :)
* henninge changed the topic of #launchpad-reviews to: on-call: henninge, intellectronica || reviewing: gmb || queue [mrevell] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> henninge, mrevell: i can review this, since you're already reviewing stuff
<henninge> mrevell: it's rosetta-specific help text, right?
<mrevell> thanks intellectronica, although henninge if you have time to look at it too I'd like your view from a Translations-guy PoV.
<intellectronica> right
<henninge> gmb, review sent. I am off to lunch but we'll have a (short) second round later ... ;)
<gmb> henninge: Cool, thanks. I'll grab some lunch too.
<mrevell> thanks intellectronica
<salgado> intellectronica, care to review a one-liner for me? http://paste.ubuntu.com/355009/
<intellectronica> salgado: sure
<salgado> we currently have these two branches for pygettextpo, but we should be using only the ~launchpad-pqm one.  I'll delete the other one once this fix lands
<intellectronica> salgado: cool. r=me
<salgado> thanks intellectronica 
* hennige changed the topic of #launchpad-reviews to: on-call: henninge, intellectronica || reviewing: mrevell, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> intellectronica: Would you be able to review an external bug tracker branch for me? https://code.edge.launchpad.net/~allenap/launchpad/trac-sniffing-broken-bug-504310/+merge/17139
<intellectronica> allenap: of course
<gary_poster> jml: I'm looking at https://code.edge.launchpad.net/~jml/launchpad/use-testtools/+merge/16711
<gary_poster> you there?
<allenap> intellectronica: Thanks :)
<gary_poster> jml lines 193 and 201 look like you have checked in diff cruft
<gary_poster> other than that looks fine.  will comment as such.
<intellectronica> allenap: why did you remove the assignment to html_data?
<intellectronica> was it lint?
<salgado> intellectronica, care to review another trivial one?  this time a testfix: http://pastebin.ubuntu.com/355032/
<allenap> intellectronica: Yes, lint.
<intellectronica> salgado: sure, let me just finish with allenap's branch first
<intellectronica> allenap: r=me
<salgado> gary_poster, I was going to fix that in my testfix branch, but I don't see the conflict markers in that file, so I think jml solved the conflict before landing
<allenap> intellectronica: Woohoo, thanks :)
<gary_poster> salgado: great thanks
<intellectronica> salgado: r=me
<salgado> thanks intellectronica 
<bac> intellectronica: can i have a UI review?  https://code.edge.launchpad.net/~bac/launchpad/bug-419930/+merge/17015  note there are screenshots attached on the bug.
<intellectronica> bac: sure, though note that i'm not a graduated ui reviewer. you will require an additional ui review after me (unless it's really trivial)
<bac> intellectronica: darn, i forgot.  that would just be beuno and noodles at this point.
<bac> intellectronica: but please have a look if you want
<bac> beuno: could you do a mentored review for a UI change?
<intellectronica> bac: sure thing
<bac> thanks
<intellectronica> bac: looks good. one thing i think can be improved is if we used the common format '[Merge accounts] or _Cancel_', instead of 'Submit'
<beuno> bac, sure
<bac> intellectronica: ok.  i'll make that change to both cases
<intellectronica> bac: cool, so ui=me with that. let's see what beuno thinks
 * beuno looks
<beuno> bac, I have a few comments
<beuno> IRC or MP?
<bac> beuno: either
<bac> beuno: i need to be afk for about 15 minutes, though
<beuno> IRC it is
<beuno> ok
<beuno> MP it is  :)
<hennige> mrevell: The help popups are missing CSS on my lp.dev and the spinner keeps spinning ...
<hennige> mrevell: btw, I wonder if "continue" is the right labeling for the "close" button or the popup.
<hennige> s/or/of/
<mrevell> hennige, Hmm, that seems to happen on the live site too. As for the "Close" button, I have a bug for that and will fix it this cycle.
<mrevell> hennige, I'm not sure what's happening with the stylesheet there.
 * hennige revs up firebug
<hennige> mrevell: 404 on reset.css, fonts.css, base.css
<hennige> those are the YUI css base files.
<beuno> intellectronica, bac, reviews and OOPSES!
<mrevell> sinzui, I rocketfuel-branched so there shouldn't be any real divergence. As I say, the same problem shows on edge.
<mrevell> sorry, hennige, not sinzui
<beuno> rockstar, thumper, https://pastebin.canonical.com/26402/
<mrevell> hennige, Do you know your nick is mis-spelt?
<beuno> intellectronica, bac, I can't review, the MP oopses. So: http://paste.ubuntu.com/355050/
<henninge> mrevell: didn't, thanks
<intellectronica> beuno: what oopses, adding a comment to the mp?
<beuno> intellectronica, a review, yes
<intellectronica> beuno: let me try. worked for me with other MPs earlier
<intellectronica> yep. oopses for me too :(
<bac> beuno: thanks for the comments.  i'll get cracking on them
<henninge> mrevell: I don't know what the change on "https://translations.launchpad.dev/~name16" is.
 * henninge reads diff abain
<bac> beuno: i assume your comment about wordiness applies to the old (http://launchpadlibrarian.net/37626290/merge-visible-email.png) and new?
<henninge> again
<mrevell> henninge, It's the "More about reviewing" link
<henninge> mrevell: ah, cool
<henninge> mrevell: looking at the diff (again), I saw that the css files in question are being included in each html page. That boiler plate code has probably changed (or is gone).
<mrevell> henninge, Should a merge from devel fix that?
<henninge> mrevell: no, because those are new files you created.
<mrevell> henninge, Oh, the html files for the pop-ups?
<henninge> mrevell: you should look at one of the existing help html files and see how the <head> looks in those.
<henninge> right
<henninge> mrevell: oh no sorry, I forgot that it's not working on edge, either.
<beuno> bac, it does
<mrevell> henninge, Heh, I was just typing that
<henninge> mrevell: but I don't see the 404 errors on edge.
<mrevell> henninge, This is odd. All I did was rocketfuel-branch, so those 404 errors are derived from devel.
<leonardr> flacoste, thanks for your reviews
<leonardr> i'm going through them today
<flacoste> leonardr: no problem
<henninge> mrevell: finally found the "more about reviewing" link - I need to be logged in as a reviewer, obviously ... ;-)
<mrevell> aha :)
<henninge> mrevell: ah, my error. The 404's are on edge, too.
<mrevell> henninge, So, I'm not sure what needs to be done about that. Do I need to change the help pop-ups to reference combo.css?
<henninge> mrevell: yes, basically
<henninge> mrevell: this is what the normal pages do: http://paste.ubuntu.com/355065/
 * mrevell looks
<henninge> mrevell: but I just tried that and it makes everything centered.
<mrevell> henninge, Heh, yeah, so it does. Hmm. :)
<henninge> mrevell: so the help files might also need some class added somewhere.
<henninge> mrevell: which brings us back to asking sinzui if he can help us ... ;-)
<mrevell> :)
<henninge> mrevell: also, that line will have been generated somehow to include the correct revision number.
<henninge> some TAL
<henninge> mrevell: or maybe salgado can help?
<mrevell> henninge, Ah, that's a shame as they've been nice and simple HTML up until now. I'll ask Mr Salgado :) Cheers henninge
<allenap> intellectronica: Up for another external bug tracker branch? https://code.edge.launchpad.net/~allenap/launchpad/kde-bugzilla-being-odd/+merge/17142
<henninge> mrevell: ask him here, so I can learn, too.
<henninge> or on #lp-dev
<mrevell> salgado, same questions as I just asked elsewhere :)
<intellectronica> allenap: on the phone, but can do it in ~30m
<mrevell> salgado, I'm asking because we need to somehow translate that to the help pop-ups too
<allenap> intellectronica: Cool, thanks, that would be great. I'll put it on the queue; maybe henninge will get to it first.
* allenap changed the topic of #launchpad-reviews to: on-call: henninge, intellectronica || reviewing: mrevell, - || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> salgado, Perhaps we could have a chat when you're off your call.
<salgado> mrevell, sure
<mrevell> thanks :)
<henninge> allenap: I will look at it but I will not last much longer ...
<allenap> henninge: Hehe, don't worry then. It's certainly not urgent.
<henninge_> allenap: yes, thank you. I have to stop now.
* henninge_ changed the topic of #launchpad-reviews to: on-call: intellectronica || reviewing: - || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> mrevell, I'm free now
<mrevell> hi salgado. The help pop-ups (as an example lib/lp/translations/help/translation-groups.html) need to be reworked for combo.css. henning mentioned that they'll probably need some TAL now to update the version number.
<mrevell> salgado, Is that something you can help with?
<salgado> mrevell, why do they need to be re-worked for combo.css?
<mrevell> salgado, Because I believe the css files the pop-ups rely on have been deleted. Take a look at https://edge.launchpad.net/+help/openid.html for an example
<salgado> mrevell, oh, no, that's not because of my combo.css work -- it's because the latest lazr-js moved these files to a different place
<mrevell> oh right
<salgado> mrevell, https://edge.launchpad.net/+icing/yui/cssreset/reset.css
<salgado> mrevell, so, just drop /current/build from the path
<mrevell> salgado, aha, thanks :)
<allenap> intellectronica: I've got to head off soon to pick up Robin. I'll be back on at about 2030, but I assume you'll be gone by then. It's not urgent, so please leave it for tomorrow if you'd rather chat about it.
<allenap> intellectronica: When I say "leave it for tomorrow", I mean I'll ask tomorrow's OCR to look at it.
<intellectronica> allenap: okie doke
<allenap> intellectronica: Cool. Of course, if there's no OCR I might come knocking... :)
* intellectronica changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> flacoste, would you take a look at my most recent comment at https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-collection/+merge/16924 and let me know if i'm missing anything?
<thumper> beuno: please file a bug with the oops, and I'll try to get to it today
<beuno> thumper, will do, thanks
<rockstar> al-maisan, https://code.edge.launchpad.net/~rockstar/launchpad/branch-scanner-job/+merge/17132
<al-maisan> jml: could you please review the schema patch we discussed yesterday (https://code.edge.launchpad.net/~al-maisan/launchpad/bq-platform-columns-505725/+merge/17125) ?
<jml> al-maisan, sure.
<jml> al-maisan, once I'm done w/ bfiller on #launchpad
<al-maisan> jml: thanking you :)
<jml> al-maisan, done. approved.
<al-maisan> rockstar: https://code.edge.launchpad.net/~al-maisan/launchpad/bq-platform-columns-505725/+merge/17125
* jtv changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jtv - http://tinyurl.com/y87el9a] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> al-maisan, https://code.edge.launchpad.net/~rockstar/launchpad/job-suspended-status/+merge/17173
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/imports-urls/+merge/17172
<thumper> mwhudson: diff updated sans-conflicts
#launchpad-reviews 2010-01-12
<jtv> come on you slackers, I want a review!
* jtv changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/hg-imports-using-url/+merge/17187
<mwhudson> thumper: i'm writing billions of comments for my db patch
<thumper> heh
<thumper> I'm about to go through your last respones
<mwhudson> thumper: also there are no requested reviews, how did you do that?
<thumper> mwhudson: it is in progress :)
<mwhudson> ah
<thumper> stub: https://code.edge.launchpad.net/~thumper/launchpad/imports-urls/+merge/17172
<thumper> stub: I took jelmer's import work
<thumper> stub: and unified the url fields
<thumper> stub: in prelim of the hg import work
<stub> mwhudson: SourcePackageRecipeDataInstruction.recipe_data is still nullable
<mwhudson> stub: darn
 * mwhudson pushes again
<stub> mwhudson: Looks like the owner's name is used to refer to recipes. Is this going to explode if someone changes their Person.name?
<mwhudson> stub: recipes don't refer to other recipes (yet)
<mwhudson> stub: i guess it will explode much like branches
<stub> mwhudson: And this only exists in Launchpad? Or are the URLs or ids referenced in sourcepackages or anything like that?
<mwhudson> stub: hm
<mwhudson> stub: actually, all references within launchpad will be at the database level
<mwhudson> URLs won't be stored by programs outside launchpad, i expect
<stub> I never got a rationale on the SourcepackageRecipe/SourcepackageRecipeData split either.
<stub> Unless you need to keep orphaned SourcepackageRecipeData information I can't see the gain.
<mwhudson> i've explained this several times
<mwhudson> although possibly not in an email to you specifically
<mwhudson> SourcepackageRecipeData is also referenced from SourcePackageRecipeBuild
<stub> Not to me or in the MP where I asked the q ;)
<mwhudson> stub: one way of looking at the split is that the SourcepackageRecipeData is the bzr-builder part
<mwhudson> stub: and SourcePackageRecipe is the launchpad metadata around it
<mwhudson> a little like the way the bazaar branch is stored separately from the Branch row
<mwhudson> and the reason they're separate is that the build needs to refer to the manifest, which is a "frozen" version of the bzr-builder part of the recipe
<stub> So the SourcePackageRecipeBuild.manifest may point to a different recipe via SPRBD than SPRB.recipe?
<stub> Or in that case, will the manifest point to a SPRBD with no corresponding recipe?
<mwhudson> stub: a particular SPRD is references from exactly one other row
<mwhudson> either a SPR or a SPRB
<mwhudson> never both
<stub> If the pointers went the other way (add SPRD.recipe and SPRD.recipe_build), we can enforce that constraint.
<mwhudson> stub: the patch has comments now
<stub> Rather than leaving it to your documentation :)
<rockstar> jtv, https://code.edge.launchpad.net/~rockstar/launchpad/use-suspended-job-status/+merge/17191
<jtv> ahh, there it is
<mwhudson> stub: yeah, that's probably a good idea
<stub> mwhudson: Is bzr-builder a Launchpad project or an Ubuntu project btw?
<mwhudson> stub: it's a james_w project
<mwhudson> stub: i guess that makes it more an ubuntu thing than a launchpad thing
<stub> Just wondering if revspec should be used or not. If it is the term used in the domain it is probably valid. I suspect it violates our Python coding standards though.
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/js-play/+merge/17193
<thumper> stub: I'd love to get https://code.edge.launchpad.net/~thumper/launchpad/imports-urls/+merge/17172 landed
<mwhudson> stub: hm, it's a pretty standard term in the bzr world
<thumper> :)
<stub> mwhudson: Thats fine then.
<mwhudson> thumper: stop distracting the DBA
<mwhudson> thumper: this branch blocks everything
<thumper> ok
<thumper> mwhudson: didn't realise he was still hastling you so much
<thumper> how about revision specifier?
<stub> I suspect it is a good idea that this isn't entwined with the existing Build table
<stub> If I was going to call it anything, I'd just call it 'revision' but domain naming trumps since that pulls in more meaning.
<mwhudson> stub: for the constraints, you mean something like http://pastebin.ubuntu.com/355335/ ?
<mwhudson> stub: revision would definitely not be right, that would imply to me that it specifies a particular revision
<mwhudson> whereas it could be a tag, or a revno, or even a lca with some other branch
<mwhudson> (this won't work as i have to reorder the file, but the idea is there at least)
<stub> mwhudson: That allows them both to be NOT NULL. You want (recipe IS NULL != build IS NULL) if its one or the other but not both.
<mwhudson> stub: doh
<stub> mwhudson: Review in including indexes etc.
<mwhudson> stub: thanks
<stub> mwhudson: sourcepackage is one word in Launchpad at the moment so there is some renaming needed or discussion
<mwhudson> stub: why is sourcepackagerecipe__owner__idx commented out
<mwhudson> ?
<mwhudson> stub: ok, that's easy enough to fix
<stub> Oh - we don't need it (the unique constraint on owner, distrorelease, ... can be used.
<mwhudson> ah ok
<stub> Unless we see people merge timeouts because the table is too big and the index too wide ;)
<mwhudson> stub: that definitely falls into your area :-)
<stub> thumper: Any particular reason you are not creating a modified version of absolute_url() in database/schema/trusted.sql and fixing Bug #506146?
<mup> Bug #506146: git and hg imports don't check for valid_absolute_url <code-import> <testing> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/506146>
<thumper> stub: because it has to do with how we are testing git imports
<thumper> stub: currently it is using the local file system
<thumper> stub: and I don't want to fix this in this branch
<thumper> stub: we'd need to fire up a git server in order to fix this bug
<thumper> stub: so we'll probably add git and hg to the developer dependancies
 * thumper leaves now
<stub> thumper: The web ui protects us?
<thumper> stub: it does
<stub> mwhudson: I'm out to lunch. You can land the branch with the fixes if it is blocking things - I can do any tweaks post landing if they are needed.
<mwhudson> stub: we're EODing now, so i'll make fixes, push and if you say ok i'll land it later
<al-maisan> noodles775: https://code.edge.launchpad.net/~al-maisan/launchpad/url-props-505382/+merge/17197
* gmb changed the topic of #launchpad-reviews to: on-call: gmb (reduced workload whilst fixing checkwatches) || reviewing: - || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> mwhudson: Should BuildSourcePackageFromRecipeJob.job be UNIQUE?
<mwhudson> yes, i guess
<mwhudson> mind you it seems branchjob.job should also be unique and it isn't
<stub> mwhudson: Feel free to fix branchjob.job in your patch while you are there :)
<stub> More indexes I missed the first time sorry and a constraint fix.
<mwhudson> stub: ok, now?
 * stub looks
<stub> quick, land it before I notice anything else.
<mwhudson> thanks
<mwhudson> :)
 * mwhudson zzzs
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac (gmb:reduced workload whilst fixing checkwatches) || reviewing: -,- || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> good morning gmb
<gmb> bac: Hi bac.
<bac> gmb: pretty quiet around here today?
<gmb> bac: Yes, so far.
<bac> cool.  have fun with CW.  i'll grab jtv's branch now
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac (gmb:reduced workload whilst fixing checkwatches) || reviewing: -,jtv || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jtv: ping
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac (gmb:reduced workload whilst fixing checkwatches) || reviewing: -,- || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,- || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,sinzui-w&w || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> beuno: any thoughts on the new screenshots i sent you?
<beuno> bac, looking now (was knee deep in some sprite fixes)
<beuno> bac, looks much much better, thanks
<beuno> 2 quick questions
<beuno> where does the "continue" link take you?
<beuno> actually just that question  :)
<beuno> bac, also, I have an almost trivial branch for review when you have a minute: https://code.edge.launchpad.net/~beuno/launchpad/sprite-issues-never-die/+merge/17229
<bac> beuno: 'continue' takes you back to ~beuno
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,sinzui-w&w || queue [beuno] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<beuno> bac, s/continue/Back to my account/ and ui=me
<bac> beuno: great, thanks
<bac> beuno: can you update the MP so i can use 'ec2 land'?
<beuno> bac, I can, do you have the link handy?
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-419930/+merge/17015
 * beuno goes get his rubber stamp
<beuno> bac, done
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,beuno || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> beuno: what about this in projects-index.pt: <p class="sprite error application-summary"
<bac> beuno: does it need changing too?
 * beuno looks
<beuno> bac, yes, it's the only place where that sprite is used
<beuno> since I renamed it, I need to change it as well
<bac> beuno: ok, r=bac with that.  MP updated
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,- || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<beuno> bac, thanks
<gmb> henninge: Re: my review from yesterday. W.R.T the use of constants and updating doctests, I think it would also be a good idea to make the tests for _getHeatFrom*() into unittests, since we'll want to add more later and unittests are more flexible in that way. How does that sound to you?
<gmb> (I'm using constants as you suggested, though)
<henninge> gmb: I like unittests ;-)
<gmb> henninge: Ah, good! I shall do that forthwith, then.
<henninge> gmb: and you are right, the way they are multiplied there for coverage does not belong in a doctest.
<henninge> and good coverage is cool.
<gmb> henninge: Right. I wrote the doctest because I was doing TDD and trying to work out the best way to do things, but now that I've done that a unittest would be better to ensure good coverage.
<henninge> gmb: I can't wait to see the new code ... ;_)
 * beuno curses at ec2 test not being headless by default
<bac> salgado-lunch: you asked that i file a bug in your review of https://code.edge.launchpad.net/~bac/launchpad/bug-419930/+merge/17015 -- i've discussed the issue at length with edwin and he says it is handled.  so i'd rather not open a bug unless you insist.
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,sinzui || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> henninge: I don't think the BugHeatConstants need to be in interfaces/bug.py. They're only going to be used in the one place.
<henninge> gmb: what about the rule that tests must not import from model code?
<henninge> At least I thought there was such a rule ...
<gmb> henninge: We aren't importing from model code though. We're importing from scripts.bugheat, which has nothing to do with the model whatever.
<gmb> henninge: I'm not familiar with that rule, though it makes sense (We should be using the factory where possible, getUtility() where not)
<henninge> gmb: ok, I am not too fussed about that.
<gmb> Cool.
<henninge> exactly
<salgado> bac, don't worry about it, then.
<bac> salgado: great
<bac> hi sinzui, i'm looking at https://code.edge.launchpad.net/~sinzui/launchpad/non-existent-packages-bug-204119/+merge/17176
<bac> sinzui: at line 64 is there a reason you're using PackagingAddView in the super() and not the actual class name?
<sinzui> oops
<sinzui> bac: Yes, I am incompetent. I moved the method from  PackagingAddView when I decided it was not the right place for the rule.
<bac> sinzui: easy mistake
 * bac hates super() syntax anyway
<sinzui> bac: The code works because PackagingAddView does not have a setupFields.
<sinzui> I will fix that
<mrevell> henninge_, If you're still around, I've updated my translations help MP -- https://code.edge.launchpad.net/~matthew.revell/launchpad/translations-help-10.1/+merge/17032
<henninge> mrevell: I am, I will look at it in a minute.
<mrevell> thanks henninge
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: lunch,/unch/sinzui || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> gmb, bac: If either of you feels like reviewing my branch, I'd be really happy.
<henninge> It is oversized and I won't be around until tomorrow, so I'd understand if you refuse. https://code.edge.launchpad.net/~henninge/launchpad/bug-503454-security-py/+merge/17241
<henninge> gmb is probably done for the day anyway.
<henninge> mrevell: I like the mo-style layout comment ;)
<mrevell> heh
<mrevell> I'm heading off to cook. Will be back around these parts three or four hours.
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: henning|| queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<flacoste> sinzui: i replied to your review
<sinzui> great because I just sent my windmill test fixes to EdwinGrubbs. I'll take a look at your changes
<kfogel> bigjools: see my mail -- need a 100x100 px face or avatar image from you for planet.launchpad.net
<bigjools> kfogel: feel free to use my LP mug
<kfogel> bigjools: thx
<kfogel> bigjools: the cartoony one? :-)
<bigjools> kfogel: yep :)
<bigjools> kfogel: it's actually a scan of what was on my wedding invitations
<kfogel> bigjools: I also just used it as a test attachment on a bug in my dev instance, for working on bug #506018.
<kfogel> so there
<bigjools> kfogel: copyright infringement!
<mup> Bug #506018: Need a "+patches" view: report lists patches attached to bugs. <story-patch-report> <Launchpad Bugs:Triaged by kfogel> <https://launchpad.net/bugs/506018>
<kfogel> bigjools: no, because I'm not publishing :-)
<bigjools> :)
<kfogel> bigjools: btw, one's launchpad.dev DB stays unmolested between 'make run', shutdown, 'make run', shutdown, etc, right?  I won't lose this new project when I shut down, only when I do 'make schema', I'm guessing?
<bigjools> kfogel: yep, only make schema blitzes it
<kfogel> bigjools: cool
<kfogel> bigjools: I can't quite tell when I need to restart.  Apparently, not after modifying a .pt file?  But always after modifying a .py file?  (I mean to see the effect of my changes.)
<bigjools> kfogel: that is correct on both counts
<EdwinGrubbs> abentley: are you still on call?
<abentley> EdwinGrubbs: No, I'm at a sprint.
<EdwinGrubbs> ok
#launchpad-reviews 2010-01-13
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-code-windmill-tests/+merge/17268
<stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails.
<allenap> henninge: I assume the topic is out of date...?
<henninge> allenap: well, bac should be in bed by now ... ;-)
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> allenap:  My branch is not reviewed yet and bac has not claimed it.
<allenap> henninge: And he's a very naughty boy if he's not.
<allenap> henninge: I'll go and have a look.
<henninge> ;-) definitely
<henninge> allenap: thanks
<henninge> allenap: it's over-sized.
<allenap> henninge: I've just read my email; my duty first and foremost today is to get checkwatches working again, so thank you for flying AllenapAir, but I need to jump out of the emergency exit now.
* allenap changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> allenap: right, I read that, too. Good luck!
<henninge> Don't hit the deck...
<stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails.
* stub changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge,stub] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> hey henninge, do you have a few moments for me?
<henninge> mrevell: shure
<henninge> ;)
<mrevell> henninge, Axctually, don't worry. unping
<henninge> mrevell: np, you can unping me any time ... ;)
<mrevell> heh
<henninge> stub: review done ;)
* henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> w00t
<adeuring> beuno: could you do a ui review of this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-172501 ?
<beuno> adeuring, sure, looking
<adeuring> beuno: thanks!
<beuno> adeuring, screenshots?   :)
<adeuring> beuno: sorry, forgot them. let me make them...
<beuno> or do I need to really interact with this to review it?
<adeuring> beuno: nah, i don't think that's necessary. The main idea is: if you upload an attachment called something.diff but don't mark that as a patch, you're redirected to the attachment edit page where you see a note that LP thinks there is ann incoistency.
<adeuring> beuno: same thing, if you upload a file without the suffix .diff, but mark it as a patch
<beuno> great, screenshots will do fine then
<adeuring> beuno: http://people.canonical.com/~adeuring/patch-check-1.png http://people.canonical.com/~adeuring/patch-check-2.png
<beuno> adeuring, thanks, that's enough for me to review
<beuno> a few layout tweaks come to mind, I'll add them to the MP
<adeuring> beuno: OK
<beuno> adeuring, so you upload the file, and if it detects either of the cases mentioned above, you get redirected here, correct?
<adeuring> beuno: right. And, BTW, the  "patch" checkbox still has the value LP thinks is correct
<adeuring> the user must override that value again.
<beuno> gotcha, so it will check/unchecs based on our guess
<adeuring> beuno: yes
<beuno> adeuring, perfect, thanks for the info. I think we'll need to re-jig the UI a bit so people understand what happened without reading all that text (which they won't)
<adeuring> beuno: yeah...
<beuno> I'll chew on it for a bit and propose something, hopefully very easy to change   :)
<adeuring> ok
<beuno-lunch> adeuring, starting to write the review, I'll finish it after lunch
<adeuring> beuno-lunch: ok
<EdwinGrubbs> allenap, jtv: are you not on-call today?
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> EdwinGrubbs: I am, but I have a semi-critical bug to fix (checkwatches).
<EdwinGrubbs> henninge: I will look at your mp now
<EdwinGrubbs> ah
<allenap> EdwinGrubbs: I saw that you assigned a review to me. I can look at it tomorrow... is that okay?
<EdwinGrubbs> that's fine
<henninge> EdwinGrubbs: great, thank you! Note: it's over-sized.
<henninge> but it would be great to get it reviewed because it is likely to rot quickly.
<EdwinGrubbs> henninge: what is the purpose of IPersonRoles?
<henninge> EdwinGrubbs: ;-)
<henninge> EdwinGrubbs: to reduce noise in code from repeatedly having to do the ILaunchpadCelebrities dance.
<henninge> EdwinGrubbs: it has a little history behind it, where I first created a helper file that provided functions to encapsulate this.
<henninge> But an adapter on IPerson is much nicer to handle.
<henninge> EdwinGrubbs: bug 503454 was the previous one that introuced it (well, the branch linked to it)
<mup> Bug #503454: Make checks for celebrity persons and roles easier <tech-debt> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/503454>
 * henninge just realized that he forgot to link the branch on that bug.
<henninge> done
<henninge> danilos: fancy to review my fix for our critical bug? https://code.edge.launchpad.net/~henninge/launchpad/bug-506925-oops-export/+merge/17304
<henninge> danilos: I got Edwin busy on my big branch ...
<henninge> or anybody else? ;-)
<danilos> henninge, sure, if it's not longer than 275 lines ;)
<danilos> henninge, whoops, sorry, it's 277 :)
<henninge> danilos: hey, you've been peeking.
<henninge> ;)
<danilos> henninge, ok, I've got to start with a question: who's Kubla Kahn at pleasure dome? (you scare me sometimes, did I ever mention it? ;)
<henninge> danilos: heh
<henninge> danilos: ;)
<henninge> danilos: I believe he is older than me.
<henninge> ...
<danilos> line 29, you should be able to re-raise the exception simply by calling "raise" I think
<danilos> henninge, ^
<henninge> danilos: I copied that from the previous code but I believe this adds the file name to the message.
<danilos> henninge, also, the check for UTF-8 is likely not bulletproof (i.e. does "UTF8" also work? I am not sure, but it might)
<danilos> henninge, right, I guess that makes sense then
<danilos> henninge, I see that bit is also copied code, so let's ignore it for now
<danilos> henninge, i.e. we are not making anything worse for now
 * henninge is good at copying other people's code
<danilos> henninge, I also see you optimized it a bit, woohoo (i.e. do .encode() only once on the entire file content, and not separately on each message)
<henninge> danilos: if the encoding was something like "UTF8" it would be caught on the second run of the method.
<danilos> henninge, "run, method, run"
<henninge> danilos: yes, it was the obvious thing to do.
<henninge> it's probably a fast one, too. ;)
<danilos> henninge, good job :)
<danilos> henninge, that's all I am saying, no need to justify your actions :P
<danilos> henninge, now, a comment
<danilos> henninge, why not catch the exception in the callsite for _try_encode_file_content and do an "else" there?
 * henninge tries to remember what else: did with try: ...
<danilos> henninge, well, try...except
<henninge> yeah, of course.
<henninge> It executes if no exception was raised
<henninge> or caught?
<danilos> henninge, ok, so how can you get None for the return value, that's the question?
<danilos> henninge, why not instead of returning none just try UTF-8 inside the method? it would be much saner to me
<henninge> danilos: well, there is the logging bit.
<danilos> henninge, i.e. pass in a list of encodings to try and just fail if it doesn't work
<danilos> henninge, you can still log if it doesn't work with the first encoding :)
<danilos> henninge, the only ugly side effect is the "update header in DB" bit
<henninge> danilos: and what's in the method is the part that needs to be repeated.
<danilos> henninge, good point
<henninge> that's why I put it there
<danilos> henninge, but the return values seem very weird (i.e. None indicates that it couldn't encode it; that's what exceptions are for)
<henninge> danilos: ok, true
<henninge> it is only meant to make sense in this context, though.
<danilos> henninge, I know, but I'd still prefer it if you get re-raising UnicodeEncodeError out of the method and instead catch it in the callsite
<danilos> henninge, and then you can rename the method from _try_encode... to _encode... and put it in a try: except: block
<henninge> danilos: well, if it fails to encode using UTF-8, we don't have a plan to handle it.
<henninge> danilos: so I don't want to catch it, let it oops.
<danilos> henninge, I know, so throwing an exception then is fine
<danilos> henninge, right, but what I am saying is that it would be much clearer to me if you didn't catch the exception and conditionally re-raise it in the helper method itself
<henninge> we could, of course change that behaviour, log the incident and produce an empty pofile with a message.
<danilos> henninge, i.e. assume the method will do the right thing, and catch the exception in the callsite and re-try the method with UTF-8
<henninge> but that is too much for this branch.
<danilos> henninge, right, I am not suggesting that
<danilos> henninge, so, my suggestion is simple: 1. get try/except out of _try_encode_file_content
<henninge> danilos: that makes the method quite short and I'd add another method tod do the catching but that's fine.
<danilos> henninge, 2. instead of checking weird value encoded_file_content == None for failure, change that to try...except on 105..122
<danilos> henninge, why would you add another method to do the catching? you change the "_try_encode...(); if encoded_file_content is None:..." to "try: _try_encode() except:..."
<danilos> henninge, that would read naturally to me at least, because this code got me confused for a moment (it's not a big deal, but just want to make sure you understand what I mean before we either agree to ignore me or not :)
* salgado changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue [salgado:<http://paste.ubuntu.com/356150/>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> EdwinGrubbs, I just added a trivial one to the queue.  it's a fix for bug 506581; can you take it?
<mup> Bug #506581: oops template page is out of center <ui> <Launchpad Foundations:New for salgado> <https://launchpad.net/bugs/506581>
<danilos> henninge, you won't have to re-raise exception again, because _try_encode_file will raise it; we will lose the filename though, so that's perhaps an argument against my proposal
<danilos> henninge, (because if we want to keep it, we'll have a nested try..except, which is very ugly)
<henninge> danilos: http://paste.ubuntu.com/356151/
<danilos> henninge, so, I modify my suggestion in light of what I just said :)
<danilos> henninge, yeah, that's what I meant, I like how that makes it much clearer, but we lose the filename now :)
<henninge> danilos: can we live with that?
<danilos> henninge, I can, but if you can't, it's not hard to fix while keeping code sane :)
<danilos> henninge, for instance, reintroduce unconditional exception re-raising in _encode_file_content with the filename
<henninge> danilos: I'd just need to add a try .. except to the second call.
<danilos> henninge, it's not hard, and it's only slightly less performant, and slightly uglier
<danilos> henninge, yeah, but we'd end up with nested try..excepts, I hate those :)
<EdwinGrubbs> salgado: sure
<danilos> henninge, if you don't, you can do it, but whenever I touch that code myself, I am probably going to kill it :)
<henninge> danilos: I don't mind
<henninge> loosing the file name
<henninge> http://paste.ubuntu.com/356154/
<danilos> henninge, ok (that may mean some failing tests, but in general, we'll know enough context already)
<henninge> same as before but with call sites fixed
<danilos> henninge, yep, got me wondering for a minute :)
<danilos> henninge, looks great if you ask me :)
<henninge> danilos: I have another idea
<EdwinGrubbs> salgado: where is your branch?
<danilos> henninge, I am all ears (well, usually as well, but now specifically :)
<salgado> EdwinGrubbs, the branch is lp:~salgado/launchpad/bug-506581.  I also have a pastebin on the topic
<salgado> EdwinGrubbs, I just added a <div class="yui-d0"> around the main content of the page.  the rest is just indentation fixes
<EdwinGrubbs> salgado: r=me
<salgado> thanks EdwinGrubbs!
<danilos> henninge, everything else looks good in the branch, so I am just waiting on your new idea before giving you r=me
<henninge> danilos: wait ...
<danilos> henninge, sure
<danilos> henninge, I need to leave soon... very soon :)
<henninge> danilos: never mind, it got too complicated
<henninge> danilos: let's leave it as it is.
<danilos> henninge, heh, ok, you've got approval for http://paste.ubuntu.com/356154/ approach or just re-introducing error message extension there
<henninge> danilos: thanks.
 * henninge has to leave, too.
<danilos> henninge, good job with it, thanks a lot!
<henninge> danilos: pleasure.
<henninge> Hi EdwinGrubbs, still chewing on my monster branch? ;-)
<thumper> jml: https://code.edge.launchpad.net/~thumper/launchpad/fix-codeimport-failure/+merge/17315 - diff still generating
<EdwinGrubbs> henninge: yep. I don't think there are any serious problems, just minor tweaks, so I'll just put them in the full review as opposed to IRC, since no discussion is really necessary.
<henninge> EdwinGrubbs: cool, thanks ;)
<EdwinGrubbs> henninge: review sent
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> EdwinGrubbs: thank you! :-)
<jml> could someone please review this https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166
<jml> it's ok, you don't have to be an ISD hacker.
<jml> thumper, https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166
 * thumper looks
<thumper> noodles775: https://code.edge.launchpad.net/~thumper/launchpad/review-clamining-fix/+merge/17323
<mwhudson> does someone want to review a REALLY EXCITING branch?
<thumper> mwhudson: ok
<mwhudson> thumper: bzr+ssh://bazaar.launchpad.net/~bzr/bzr-git/trunk/
<mwhudson> no
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/update-bzr-git-dulwich/+merge/17324
<thumper> mwhudson: done
#launchpad-reviews 2010-01-14
<rockstar> BjornT, https://code.edge.launchpad.net/~rockstar/launchpad/fix-windmill-canonicalurl/+merge/17328
<mwhudson> reviewinate pls https://code.edge.launchpad.net/~mwhudson/launchpad/update-loggerhead/+merge/17332
<mwhudson> (it's utterly simple)
 * thumper looks
<thumper> mwhudson: r=me
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/arrargh-no-sourcecode-testtools/+merge/17337
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/code-imports-for-products/+merge/17338
 * mwhudson looks
<mwhudson> thumper: i'm sure there's a bug for that already
<thumper> mwhudson: I thought so too, but couldn't find it
<mwhudson> i think wgrant filed it :-)
<thumper> mwhudson: and neighter did the dupe search
 * thumper shrugs
<mwhudson> thumper: https://bugs.edge.launchpad.net/launchpad-code/+bug/246782
<mup> Bug #246782: "Import for this project page" <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/246782>
<thumper> omg that is old
<mwhudson> it's a pretty crappy bug, i'm not surprised searching didn't find it
<al-maisan> jtv: https://code.edge.launchpad.net/~al-maisan/launchpad/xx-select-job-506617/+merge/17327
<jtv> al-maisan: lp:~jtv/launchpad/bug-500110
<adiroiban> gmb: Hi, any news about https://code.edge.launchpad.net/~adiroiban/launchpad/bug-496352-series-status-refactor/+merge/16108 ?
<leonardr> jml, i never get to do reviews with you, but since my cold woke me up at 3:30 in the morning, you want to review a branch of mine?
<jml> leonardr, sure.
<jml> leonardr, wanna help me figure out why I can't use 'prepare.py' in lazr.yourpkg?
<jml> I guess it's 3:30am, so don't feel obliged.
<leonardr> jml: no, i'm workin'
<leonardr> so i'll help you
<jml> leonardr, thanks. what's the branch?
<leonardr> looking it up now
<leonardr> jml: https://code.edge.launchpad.net/~leonardr/lazr.restful/bleed-through-stack
<leonardr> er, https://code.edge.launchpad.net/~leonardr/lazr.restful/bleed-through-stack/+merge/17298
<jml> leonardr, reviewed.
<leonardr> jml, do you know which specific class in lazr.config is the closest to BleedThroughDict?
<jml> leonardr, no, just that the behaviour -- layered key/value pairs -- is the same
<jml> leonardr, even down to the existence of pushConfig on lp.testing.TestCase
<gmb> adiroiban: It didn't land already? That's weird (I don't get emails about it unless it fails). I'll look into it.
<beuno> adeuring, did my review make sense?
<adeuring> beuno: yeah, sure, i agree with your suggetsions.
<adeuring> beuno: problem is that CHR duties distracted me today...
<beuno> heh, it's ok. I wanted to make sure they made sense to you
<beuno> jumping around projects all the time sometimes makes me slip  :)
<adeuring> beuno: ;)
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/clean-code-windmill-tests/+merge/17360
<al-maisan> rockstar: https://code.edge.launchpad.net/~al-maisan/launchpad/observe-virtualized-507323/+merge/17412
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/branch-scan-job-script/+merge/17429
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/branch-active-reviews/+merge/17433
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/propose-merging-extra-options/+merge/17350
#launchpad-reviews 2010-01-15
<thumper> noodles775: https://code.edge.launchpad.net/~michael.nelson/+activereviews
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/update-review-table-on-comment/+merge/17439
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/js-play/+merge/17193
<rockstar> mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/pedantry-round1/+merge/17441
<mwhudson> +1 with added swearing
<thumper> noodles775: https://code.edge.launchpad.net/~thumper/launchpad/js-play/+merge/17193
<stub> https://code.launchpad.net/~stub/launchpad/bug-504291-disconnection-errors/+merge/17371 is a fix for the High priority Bug #504291
<mup> Bug #504291: DisconnectionErrors (already disconnected) happening again <oops> <Launchpad Foundations:In Progress by stub> <Storm:Invalid> <https://launchpad.net/bugs/504291>
<henninge> Good morning!
<henninge> When ever a reviewer comes around, I'd be glad to get a review for my branch ... ;-)
* henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: https://code.edge.launchpad.net/~henninge/launchpad/buildmaster-failure/+merge/17449
<noodles775> Looking
<noodles775> henninge, done. Thanks for that!
<henninge> noodles775: I saw it, thanks. Already in pqm.
<salgado> henninge, how about exchanging some reviews? ;)
<henninge> salgado: with pleasure!
<salgado> henninge, where's yours?  mine's at https://code.edge.launchpad.net/~salgado/lazr.restful/bug-507447/+merge/17395
<henninge> salgado: https://code.edge.launchpad.net/~henninge/launchpad/bug-488765-oops-translations/+merge/17396
<henninge> salgado: done
<salgado> henninge, I assume danilo was ok with fixing the bug by changing the view?  I ask because I see no comments from him on the bug after you listed the different ways we could fix it
<henninge> salgado: danilo is out of order atm ... ;)
<henninge> salgado: yes, I am sure he is ok with that.
<henninge> salgado: bug 507534 is the real bug that still needs to be fixed.
<mup> Bug #507534: Product.primary_translatable also lists source packages <Launchpad Translations:New> <https://launchpad.net/bugs/507534>
<henninge> and that is what danilo propsed, too.
<henninge> This is mainly to stop the oops from happening and not getting an empty page.
<salgado> ok
* salgado changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: can i get a review for a tiny branch?
<salgado> bac, sure!
<bac> salgado: cool:  https://code.edge.launchpad.net/~bac/launchpad/bug-491320/+merge/17458
<bac> salgado: rebooting but i'll be right back
<henninge> salgado, bac: can you tell me what this TALES statement does exactly?
<henninge> context/required:launchpad.TranslationsAdmin
<henninge> Does it mean "true if the current user has this permission on the current view"?
<salgado> henninge, returns True if the logged in user has the given permission on context
<henninge> context, not view, right.
<henninge> salgado: thanks
<salgado> bac, what was causing the newlines and spaces to be added to the text area?
<bac> salgado, the spaces and newlines between <textarea> and </textarea>
<salgado> oh, of course
* henninge changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> salgado-lunch: I have replied to your review and put another branch up for review. If you could be so kind ... ;)
<henninge> salgado-lunch: oh, and please don't forget to enter "code" as the review type to keep "ec2 land" happy.
<EdwinGrubbs> allenap: did you already start reviewing my branch? I was going to ask salgado to review it otherwise.
<allenap> EdwinGrubbs: I am so sorry, no I haven't. I've been working on this sort-of-critical checkwatches issue and I forgot. Please ask salgado as I'm still working on it.
 * allenap tasers himself in an act of modern auto-flagellation for forgetting.
<EdwinGrubbs> salgado-lunch: can you review this branch. About half of it is simple search and replace on zcml files, so it is actually much easier than the size of the diff would indicate.  https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-451208-subscribing-reveals-email/+merge/17261
<bac> salgado-lunch, thanks for the review
<salgado> EdwinGrubbs, sure, I'll get to it soon
<henninge> Hi salgado, I will not be around for much longer. Do you think you can have a look at my second proposal?
<salgado> henninge, do you know how to enter the review type using the email interface?
<salgado> doing that now
<henninge> yes, just append
<henninge> review approve code
<salgado> cool
<henninge> salgado: thanks a lot!
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: Edwin || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: Edwin || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> salgado: thanks for the review. There is another branch of mine, just a short one.
<henninge> salgado: https://code.edge.launchpad.net/~henninge/launchpad/bug-507498-oops-subset/+merge/17459
<salgado> henninge, ok
<henninge> salgado: I will leave now but if it's easy, please just do.
<henninge> salgado: thank you very much!
<salgado> well, you should tell me if it's easy or not. ;)
<salgado> gary_poster, can we check that an object is security proxied in a way that's less ugly than http://paste.ubuntu.com/357145/?
<henninge> salgado: I meant, if you don't have any questions ... ;)
<henninge> salgado: I am off, ttyl ;-)
<gary_poster> salgado: yes I think so (sorry was on call
<gary_poster> getting ref
<gary_poster> salgado: would queryProxy from zope.security work for you (see http://svn.zope.org/zope.proxy/trunk/src/zope/proxy/interfaces.py?rev=75161&view=auto )
<salgado> gary_poster, I think it should work, let me check with EdwinGrubbs 
<salgado> EdwinGrubbs, ^
<EdwinGrubbs> salgado: I can try that.
<EdwinGrubbs> salgado, gary_poster: I'm getting this error: ComponentLookupError: (<InterfaceClass zope.proxy.interfaces.IProxyIntrospection>, '')
<gary_poster> EdwinGrubbs: it's not a utility
<gary_poster> EdwinGrubbs: just import the desired function from zope.security
<gary_poster> EdwinGrubbs: the interface describes what the package offers
<gary_poster> zope.proxy I mean
<gary_poster> EdwinGrubbs: ^^^
<EdwinGrubbs> gary_poster: thanks, that worked
<gary_poster> EdwinGrubbs: cool, np
<EdwinGrubbs> salgado: did you want get_naked_vocab() defined just in that one doc test or added to the globals in c/l/testing/systemdocs.py?
<salgado> EdwinGrubbs, just that one would be fine, I think
<EdwinGrubbs> salgado: reply sent. I'm going to lunch now.
<salgado> Edwin-lunch, why do you need the "vocab.__module__[:5] != 'zope.'" in your test?
* salgado changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> salgado-afk: because there are a few vocabs that zope registers but not as a secured utility.
<EdwinGrubbs> sinzui: did you want to review my one line fix for deactivating an account or wait until we hear back on how we can add real testing for this?
<sinzui> EdwinGrubbs: I would like to review it. I'll look at the history of the problem if I think we need to do something else
<EdwinGrubbs> sinzui: here is the diff. https://pastebin.canonical.com/26685/
<EdwinGrubbs> sinzui: do you want an MP?
<sinzui> No, I'll just look at blame to see some context
<EdwinGrubbs> sinzui: You can make it lp.registry.tests.test_person.TestPerson.test_deactivateAccount_copes_with_names_already_in_use fail in the same way the oops does just by making this change on launchpad_ftest_template.   REVOKE UPDATE, INSERT ON EmailAddress FROM write;
<sinzui> EdwinGrubbs: r=me. Every instance of this use was added 11 months ago as a part of the DB reconfiguration. There are no tests for this kind if issue
<jamalta> Hi, I'm looking into fixing a trivial bug to see if I can get into helping with Launchpad a bit again
<jamalta> I was looking at bug #253525 and can only find one place that changesfile is still listed
<mup> Bug #253525: changesfile isn't a word <ppa> <trivial> <ui> <Soyuz:Triaged> <https://launchpad.net/bugs/253525>
<jamalta> Anyways, my question is, what would be the best name for it to replace changesfile? The +archive/ppa page now reads "sources.changes"
<jamalta> https://edge.launchpad.net/~schooltool-owners/+archive/ppa/+packages <-- page that still shows changesfile
<sinzui> jamalta: "sources.changes" will do fine
<jamalta> sinzui: thanks :)
<sinzui> jamalta: I think you need to update some stories that verify the text
<sinzui> lp/soyuz/stories/soyuz/xx-distribution-sources.txt for example
<jamalta> sinzui: alright, i will make sure to udpate that as well
<jamalta> also, i found a template that references changesfile but can't figure out where that template is loaded
<jamalta> lp/soyuz/templates//distroseriessourcepackagerelease-index.pt
<sinzui> jamalta: search for the file name in soyuze/browser/configure.zcml
<jamalta> sinzui: ah awesome, thanks
<sinzui> jamalta: configure is not a nice file to visit. That is were the view and template are registered under a name (+something)
<sinzui> jamalta: This looks like the tests that need updating to pass with your changes. I am not certain though: http://pastebin.ubuntu.com/357284/
<jamalta> sinzui: right, makes sense.. i actually was trying to find such file to figure out where the template was used :)
<sinzui> jamalta: `./bin/test -vvc -t soyuz.*stories` will run all the stories to verify you text change.
<jamalta> it's a bit heavy to take in, though, you're right
<bac> sinzui: if you get bored between now and tuesday...  https://code.edge.launchpad.net/~bac/launchpad/bug-499351-batching-dls/+merge/17492
<sinzui> I'll review your branch now
<sinzui> bac: why do you cast the list to a list on line 87 of the diff?
<sinzui>      list(series_and_releases
<bac> sinzui: an abundance of caution?
<bac> stupidity?
<bac> leftover from when s_r used to be something else, i think.
<bac> yeah, i dumbly made it a set to start with.  that was suboptimal for many reasons.
<sinzui> okay.
<sinzui> anyway, r=me I think you can drop the cast
<bac> sinzui: thanks
#launchpad-reviews 2010-01-17
<lp_> hello, I have created a patch for the bug at lp:508722 and have signed the canonical contribution agreement, but am not sure what to do next, can somebody please help me?
<jpds> lp_: Commit the patch to a branch, test it, and then push it a a branch on Launchpad.
<jpds> lp_: https://dev.launchpad.net/PatchSubmission
<lp_> I've pushed it to https://code.launchpad.net/~david-dtwood/+junk/launchpad, but I am unable to find the files needed to test my changes, or the button to propose it as a merge
<jpds> lp_: You have to push it to lp:~david-dtwood/launchpad/fix_508722
<lp_> right ok, is there any way to rename a branch or do I need to push it again?
<jpds> lp_: There should be a "Change branch details" button.
<jpds> lp_: Do you have a local LP instance running?
<lp_> I have a local copy of the bzr branch, if that's what you want?
<jpds> lp_: See: https://dev.launchpad.net/Running
<lp_> no, I haven't done that yet
<lp_> so do I need to set up a local LP instance, then run my tests on that, push it to lp:~david-dtwood/launchpad/fix_508722 and finally propose it as a merge?
<jpds> lp_: Set up the instance see if your changes work, and then run the test suite with: ./bin/test -vvm bugs
<lp_> ok, i'll try to get that working after I can install postgres
<lp_> thanks, goodbye
#launchpad-reviews 2011-01-10
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> moin henninge, fancy a review? https://code.edge.launchpad.net/~adeuring/launchpad/bug-607958/+merge/45675
<henninge> adeuring: moin kollege! ;)
<henninge> adeuring: let me see how big it is ...
<adeuring> henninge: < 200 lines
<henninge> 153 ...
<henninge> adeuring: I don't understand "(Note that the timeouts mentioned in the bug report already disappeared when we switched to PostgesQL 8.3)"
<henninge> adeuring: the bug mentions 8.4 ...
<adeuring> henninge: that means that the query is quite fast under 8.4, compared to 8.3
<henninge> but the bug suggests that the timeouts only started to appear on 8.4
<henninge> "Since 8.4 - boom."
<adeuring> henninge: well, ask lifeless for what he really meant ;) (I htink it was him who added the text "since 8.4 - boom")
<henninge> I see ;)
<henninge> adeuring: but you are saying that it is not currently timing out - but your fix still makes it faster?
<adeuring> henninge: right
<henninge> ok
<adeuring> about twice as fast for ubuntu; for products the gain is probably bigger, though i did not test it
<henninge> adeuring: done
<henninge> r=me
<adeuring> henninge: thanks!
<StevenK> henninge: Hai! Would you have time to review https://code.launchpad.net/~stevenk/launchpad/bpb-currentcomponent-assertion-part-2/+merge/45693 ? I'm sorry it's so large :-(
<leonardr> henninge: i need a really tiny launchpad branch reviewed. tiny enough that i could self-review it. but i don't know the process. can you point me to it?
<benji> leonardr: https://dev.launchpad.net/PolicyAndProcess/OptionalReviews
<leonardr> thanks
<henninge> leonardr: but i can also have a look at it ...
<henninge> ;-)
<leonardr> henninge: https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration-2/+merge/45698
<henninge> leonardr: r=me
<henninge> leonardr: fwiw, you do self-approval by using [rs=leonardr], at least that's what I do.
<leonardr> henning: cool
<henninge> leonardr: the wiki page suggests, though, that lp-land and "ec2 land" do it automagically if you approve your own mp but I have not yet tried that.
<henninge> StevenK: I'll put it on the queue for a little later, if that's ok
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> henninge: At some point today is perfectly fine. :-)
<abentley> mrevell, could you do a UI review of https://code.launchpad.net/~abentley/launchpad/no-queue-rescore/+merge/45560 please?
<bac> hi allenap, would you have time to do a re-review of my branch from last week?  https://code.edge.launchpad.net/~bac/launchpad/bug-5927/+merge/45403
<allenap> bac: Sure.
<bac> thanks
<mrevell> abentley, With pleasure. I'll take a look.
<abentley> mrevell, thanks.
<benji> henninge: I have a launchpadlib branch ready for reivew: https://code.edge.launchpad.net/~benji/launchpadlib/delayed-keyring-import/+merge/45711
* benji changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK, benji]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> thanks for the review allenap
<allenap> bac: You're welcome.
* henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> StevenK: I didn't manage to do yours, sorry. I have to go now.
<StevenK> henninge: Rargh!
<StevenK> henninge: Wait until next week :-P
<henninge> StevenK: Well, it *is* oversized ... :-P
<StevenK> henninge: :-(
<henninge> Sorry
<StevenK> henninge: Don't make me print out the diff and hit you with it :-)
<henninge> Oh no! that'd be a very heavy bat!
<henninge> ;)
#launchpad-reviews 2011-01-11
<thumper> https://code.launchpad.net/~thumper/launchpad/moar-request-build-logging/+merge/45803
<StevenK> thumper: There is no Tuesday morning APAC OCR
<thumper> StevenK: oh well
<StevenK> Which is a bug, and needs to be fixed.
<thumper> no rush tehn
* allenap changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [StevenK, allenap]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Would you be able to review a mostly straightforward branch for a new view? https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-4/+merge/45824
<adeuring> allenap: sure
<allenap> adeuring: Thanks.
<adeuring> allenap: r=me
* adeuring changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Thank you :)
* henninge 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
<henninge> adeuring: I will do StevenK's, I already looked at it yesterday.
<henninge> just in case you were going to pick it up ...
<adeuring> henninge: cool, thanks
<bac> hi allenap, another go? https://code.edge.launchpad.net/~bac/launchpad/bug-5927/+merge/45403
<allenap> bac: Yeah :)
<sinzui> mrevell: can you do a UI review of https://code.launchpad.net/~thumper/launchpad/show-recipe-upload-issue/+merge/45804
<EdwinGrubbs> leonardr, mars: is one of you oncall?
<mars> EdwinGrubbs, I will be
* 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> just tied up some loose ends
<EdwinGrubbs> mars: can you review this branch when you get a chance? https://code.launchpad.net/~edwin-grubbs/launchpad/bug-664828-teammembership-delete-timeout/+merge/45805
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [Edwin]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> EdwinGrubbs, sure
<StevenK> henninge: One assert per test makes me very very sad.
<StevenK> henninge: But I'd argue that each test only touches one concept
<henninge> StevenK: I said I am not insisting on that.
<henninge> ;)
<StevenK> henninge: Right, but it still makes me sad. :-)
<henninge> but breaking them further down would be better.
<StevenK> :-(
<henninge> Sorry to make you sad :(
<StevenK> Like I say, one concept per test
<henninge> StevenK: the problem with many asserts in one test is that not all asserts get executed.
<henninge> StevenK: so, when tests fail, you have to do the "fix-rerrun-fixnext" loop.
<StevenK> Well, yeah. I've had this fun while writing them.
<StevenK> henninge: But if you look at say, line 693 of the diff -- I seriously have to split those nine asserts into seperate tests? Which blows out the line count from 20 lines to 70? :-(
<mwhudson> i don't think that sort of thing is the problem
<mwhudson> the next test, starting at 714 in the diff is worse, because it's setup, exercise, verify, exercise some more and then verify again
<mwhudson> oh actually no it's not
 * mwhudson takes his oar out again
<henninge> mwhudson: I agree that what you describe is the worst case but I did not find that here, either.
<mwhudson> StevenK: did you consider writing a testtools matcher to make these assertions easier to read?
<henninge> StevenK: *I would* split them up, also because that would be easier to read. I am not fussed about line count here.
<StevenK> I am, because I have to write it. You just have to read it. :-)
<henninge> :-)
<henninge> StevenK: I see you already did a  lot of what I asked. Thanks. ;)
<StevenK> henninge: Have I done enough, though?
<henninge> StevenK: is that a stray self.setUpBuilds() at the end of thta test file?
<StevenK> henninge: Er, yes. Well-spotted
<StevenK> henninge: Fixed locally
<henninge> StevenK: thanks. r=me ;)
<StevenK> henninge: Thanks, let me push that fix
<jtv> hi mars!  Can I chuck one on your queue?
<mars> jtv, sure
<jtv> mars: ftr it's https://code.launchpad.net/~jtv/launchpad/bug-611217/+merge/45883
* jtv changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [Edwin, jtv]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> thanks
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [Edwin, jtv]  || 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: EdwinGrubbs || queue: [jtv]  || 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: jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> mars: I've responded to your review
* 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
<jcsackett> anyone available to review https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938?
#launchpad-reviews 2011-01-12
<huwshimi> Hi, is anyone available to review a CSS change? https://code.launchpad.net/~huwshimi/launchpad/highlight-links-367877/+merge/45947
* jtv-afk changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: Ursinha || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> jtv: got room for another in your queue? https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938
<jcsackett> jtv, i think i pinged you on this when you had dropped from the channel. there room in the queue for one more? http://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938
* jcsackett changed the topic of #launchpad-reviews to: On call: jtv || reviewing: Ursinha || queue: [jcsackett]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: jcsackett || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> EdwinGrubbs: are you reviewing today?
<EdwinGrubbs> benji: yes
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> hi Edwin :)
<EdwinGrubbs> jtv: hi
<benji> k
* benji changed the topic of #launchpad-reviews to: On call: jtv, Edwin, benji || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Whoa getting crowded here.  Lemme just fix that.
* jtv changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> jtv: i've replied to your comment on my review. the only question i had was what were you foreseeing where it breaks API?
<jcsackett> snapshotting of the objects are still allowed, but those fields won't be snapshotted when the entire object is. the collections themselves can be snapshotted independently.
<jtv> I was just looking at it.
<jtv> I'm just wondering whether any clients might erroneously be expecting to get the whole collection at once.
<jtv> I may be wrong, but I assumed that collection snapshots aren't batched.
<jcsackett> jtv: hm. so if a client tries to snapshot a product expecting those fields to be in the snapshot?
<jcsackett> jtv: if you do like "thing = product.milestones; Snapshot(thing)" you shouldn't have any problem.
<jtv> (BTW I just ran tests with the setUp methods removed and got the same results as with them still in)
<jcsackett> jtv: that's good news--that didn't used to be the case. i'll kill that then.
<jtv> Once you have a setUp, you do need to call super.  But if you don't, you're good.
<jtv> Anyway, about the snapshots: I guess the real question is whether the compatibility is defined at the launchpadlib API level (or equivalent), or "on the wire."
<jcsackett> jtv: hm. i dunno. i do know that in the error case this is patching the API will explode anyway.
<jtv> Actually, I wouldn't be at all surprised if sinzui had dealt with this question before.
<jtv> True, incompatibility ain't so bad if something goes from nonfunctional to functional.
<sinzui> The snapshot code does work over the api
<sinzui> and you certainly cannot call launchpad_project.lp_save() at the moment
<sinzui> I have tried several times over the past few weeks
<jcsackett> it's blowing up with the same exception, isn't it?
<sinzui> yep
<leonardr> sinzui: what's going on here? is it something i should look at?
<sinzui> leonardr: lazr.lifecycle.snapshot does a deep copy of objects we do not want to copy
<jcsackett> leonardr: it's a shortlist error i've got a branch in review to fix. jtv has raised the question that the branch may break API compatability, as some fields are now marked doNotSnapshot.
<jtv> (with "may" meaning "I don't know" âº )
<jcsackett> jtv: right. :-)
<sinzui> leonardr: API is fine. We have logic to ensure we do not do something stupid. The logic is telling us we did do something stupid
<leonardr> sinzui: ok, let me know if you need anything (such as me re-understanding how lazr.restful uses snapshots)
<jcsackett> so, anyway, i'm probably not understanding the worry, but i don't think this breaks API compatibility. i think it fixes an issue that has hit the API too.
<jcsackett> but i'm about as certain of that as i think you are that it does cause an error, jtv. which is to say: not certain at all. :-P
<jcsackett> er, incompatibility, not error.
<jcsackett> jtv: i'll fix the other problems and pursue the incompatibility question. thanks.
<jtv> jcsackett: great, thanks.  I'm close to EOD, so don't be shy to approach someone else if you want a final stamp of approval.
<jcsackett> jtv: will do. thanks, and have a good evening. :-)
<jtv> Thanks.  Sorry I couldn't contribute more on the compatibility question than to ask it.
<jcsackett> no problem.
<EdwinGrubbs> benji: I have to leave for an appointment, but I'll be back later. Fortunately, the queue is empty.
<benji> ok
<jcsackett> benji: can you take a look at the revised diff on https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938?
<benji> jcsackett: sure
<jcsackett> thanks.
<benji> jcsackett: Approved* with a couple of tiny tweaks. (*I'll ask Edwin to review my review when he gets back.)
<jcsackett> benji: thanks.
<benji> np
<thumper> https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037
<thumper> hello reviewers :)
<thumper> benji: have you got time to look at my review?
<benji> thumper: sure
<thumper> benji: ta
* benji changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: thumper || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> thumper: Approved (but since I'm mentoring, Edwin will have to review my review)
<thumper> benji: ack
<thumper> thansk
<benji> np
<benji> EdwinGrubbs: I've done two small reviews since you were gone: https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037 and the revised diff on https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938
<EdwinGrubbs> I'll look at them
<benji> thanks
* benji changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* benji changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> thumper: why does getViewBrowser() log in as anonymous?
<thumper> EdwinGrubbs: it doesn't it logs in as a new user
<thumper> it logs in as anon iff you specify no login
 * thumper thinks
<EdwinGrubbs> thumper: there is a login(ANONYMOUS) call in that function.
<EdwinGrubbs> that isn't in the if-statement
<thumper> EdwinGrubbs: where exactly, I'll look
<thumper> ah...
<thumper> in the canonical url generation?
<EdwinGrubbs> oh
<thumper> I think it is to get it into a known login state
<thumper> as login effectively logs out anyone else
<thumper> and an interaction is needed for canonical url generation
<thumper> and since we don't want to specify a particular user
<EdwinGrubbs> thumper: ok, can you add a comment for other confused souls? Also, can you make a drive-by cleanup of ",rootsite=" in that method?
<thumper> as it adds confusion...
<StevenK> Yes, so do everything *first* and then call .getViewBrowser
<thumper> ack
<thumper> EdwinGrubbs: yes, I can do that
<StevenK> See first test in the diff at https://code.launchpad.net/~stevenk/launchpad/bpb-currentcomponent-assertion-part-2/+merge/45693
<EdwinGrubbs> I'll update the mp after launchpad is back up
<mwhudson> maybe that method should use restoreInteraction
#launchpad-reviews 2011-01-13
<wgrant> Anyone around for a review?
<wgrant> https://code.launchpad.net/~wgrant/launchpad/rebuilds-without-nai/+merge/46070
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> jcsackett, if you please: https://code.launchpad.net/~leonardr/launchpad/fix-apidoc/+merge/46161
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: leonardr || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> leonardr: sure thing.
* sinzui changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: leonardr || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jcsackett: can I push one onto your queue?
<jcsackett> jtv: sure. link to the MP?
* jtv changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: leonardr || queue: [sinzui, jtv]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jcsackett: https://code.launchpad.net/~jtv/launchpad/bug-702228/+merge/46165
<jcsackett> leonardr: r=me. i've requested follow up from sinzui.
<leonardr> jcsackett,great
<jcsackett> sinzui: i probably should not review your branch, since you're still my mentor.
<sinzui> jcsackett: I think you should because someone needs to know about mailman oddities.
<sinzui> I will find a mentor after your review
<jcsackett> sinzui: fair enough.
<jcsackett> sinzui: okay.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: sinzui || queue: [jtv]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> leonardr: ping
<leonardr> sinzui, yo
<sinzui> leonardr: what is the magic with scalar_value?
<sinzui> ScalarValue I mean
<leonardr> sinzui: it's like HostedFile--a special resource type that we don't want to document
<sinzui> Is there an example in the live docs?
<leonardr> no, it doesn't show up. i'm just making 100% sure it doesn't show up in the future
<sinzui> okay
<StevenK> Can we flag it with @hidden or something that makes WADL generation skip it?
<sinzui> leonardr: r=me. I noted that the copyright needs updating
<leonardr> StevenK: we want it in the WADL, we just don't want it in the HTML docs
<leonardr> it's a resource that the end-user never sees
<StevenK> @undocumented ? :-)
<leonardr> i don't think it's the server side's job to make that kind of distinction
<jcsackett> sinzui: so MailmanTestCase doesn't actually isolate from the real proxy?
<sinzui> I agree with leonardr. We want the xslt public so users can see how we interpret it, but other's may want to make their own interpretation of the data
<sinzui> jcsackett: it does. I was stumped by the impossible timeouts when I sketched out the test you are reading.
<jcsackett> sinzui: okay.
<sinzui> reading XMLRPCRunner.__init__, I realised it already has a reference to the real proxy
<leonardr> jcsackett: https://code.launchpad.net/~leonardr/launchpadlib/remove-xslt/+merge/46167
<jcsackett> sinzui: shouldn't the isolation you do in your test be part of the MailmanTestcase setup?
<jcsackett> sinzui: not asking you to make that change, but if it's something that should be done, perhaps file a bug?
<jcsackett> i acknowledge i don't have a full understanding of what we want in a mailman test, but unplugging from the proxy seems correct.
<sinzui> jcsackett: which setup. I think the test setup id DRY
<jcsackett> sinzui: nevermind, i was misunderstanding what was being done.
<jcsackett> sinzui: i thought that you were patching around something the base class for your test should have done. that is not the case.
<sinzui> yeah, get_mailing_list_api_test_proxy() looks like duplication, but it is changing an instance, where as super() is changing the module
<sinzui> reset_log and get_mark may move to the base class. I am pondering a test in a new branch that may need them
<sinzui> jcsackett: when you are done this my branch, I want to to talk about the implementation of my current bug, the one you looked at this morning
<jcsackett> sinzui: sure.
<jcsackett> sinzui: r=me, with two minor things.
<jcsackett> notes are on on your diff, but it's basically some docs/comments stuff.
<jcsackett> leonardr: your MP shows zero lines diff.
<leonardr> hmm...
<sinzui> jcsackett: I will follow up on you comments
<jcsackett> sinzui: sounds good. feel free to ping me when you want to mumble re: your current branch.
<jcsackett> leonardr: the file you wanted to remove (as i understand it) is still in your branch. http://bazaar.launchpad.net/%7Eleonardr/launchpadlib/remove-xslt/files/head%3A/src/launchpadlib/
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: jtv || queue: [leonardr]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> jcsackett: i didn't commit anything. new version is pushed
<sinzui> jcsackett: can we mumble for a few minutes?
<sinzui> jcsackett: http://pastebin.ubuntu.com/553725/
<pcjc2> Can someone take a look at https://code.launchpad.net/~pcjc2/launchpad/fix-tag-search-bug-501945/+merge/46075 for me?
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: jtv || queue: [leonardr, pcjc2]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> pcjc2: you're on the queue.
<jcsackett> pcjc2: you're a new contributor?
<jcsackett> jtv: r=me, but it looks lifeless hit it a bit earlier. i won't request follow up from sinzui since lifeless already approved it.
<jcsackett> leonardr: r=me.
<pcjc2> newish - this is my second patch
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: pcjc2 || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> pcjc2: cool. so, in the channel info here there is frequently an on call reviewer, who you can usually ping to get into the queue.
<jcsackett> in this instance, that's me. :-)
<pcjc2> I hashed out some of this with lifeless (He got the SQL tested), but he's a bit busy at the moment to review it
<pcjc2> Thanks - I've not been here before
<pcjc2> The other patch I did was with gmb helping, and it related to bug import, not an actual part of the web service.
 * pcjc2 approves of Peter B's config API
<pcjc2> Might want to consider the possibility of freeze and thaw for the callbacks
<pcjc2> Also, possibly add save APIs?
<pcjc2> export either a given context, or a "flattened" (borrowing from gimp layers) context ?
<pcjc2> (OOPS: -E_WRONG_WINDOW)
<sinzui> EdwinGrubbs: thumper: can either of you mentor jcsackett review of my branch? https://code.launchpad.net/~sinzui/launchpad/mailman-heartbeat-0/+merge/46160
<EdwinGrubbs>  sinzui: I can do it
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> pcjc2: there are comments on your MP.
<pcjc2> thanks
 * pcjc2 checsk
<EdwinGrubbs> sinzui: it seems strange to log "--MARK--" since syslogd can be configured to add that to a logfile automatically. Of course, when the xmlrpcrunner does it, it would be preceded by a timestamp.
<pcjc2> "You should flip those back; by convention we put the expected value first in assertEqual statements."
<pcjc2> correct fix is then to swap the parameters passed to each caller then
<pcjc2> (and swap the assert back)
<sinzui> EdwinGrubbs: I was following the suggestion of the losa.
 * sinzui looks at mailman's own logging lib again
<jcsackett> pcjc2: that may be correct, my comment was limited to the scope of your MP.
<pcjc2> I swapped them because every test had the parameters reversed, I didn't know there was a convention
<sinzui> EdwinGrubbs: mailman does not use python batteries. I think it still thinks we live with Python 2.1
<pcjc2> RE: comment - a docstring at the start of the function ok to explain the SQL?
<jcsackett> pcjc2: that should work, yes. please remember that the first line of a docstring should be one line quick explanation of the method, then a blank line, then the longer details.
<jcsackett> and thanks for making these changes. :-)
<pcjc2> like a git commit ;). NP
<jcsackett> exactly like a git commit. :-)
<jcsackett> pcjc2: ping me when you have pushed the changes, i can approve the MP and alert my mentor to follow up.
<jcsackett> even if i'm in the middle of another review i can do that quickly.
<pcjc2> ok, will do. Am merging some git conflicts another project right now, but should look at it tonight
<jcsackett> pcjc2: ok.
<EdwinGrubbs> sinzui: I just checked, and rsyslog puts the timestamp in front of its autogenerated "-- MARK --", so I think it is a bad idea to use that as your heartbeat text. If the losas insist on that, I'll agree to it, but I'm guessing they didn't think about the confusion it could cause.
<sinzui> EdwinGrubbs: I think we want a timestamp so that nagios nows when the heatbeat took place
 * StevenK hands sinzui a 'k'
<EdwinGrubbs> sinzui: yes, but if nagios is grepping for "-- MARK --" with a timestamp, and syslogd is configured to add "-- MARK --" entries every few minutes, and they also have a timestamp, the xmlrpcrunner could be broken and you would never know it.
<sinzui> I am getting rid of silent letters just after I get rid of c, j, q, x
<StevenK> You can't QQ without q
<sinzui> EdwinGrubbs: I do not know what syslogd has to do with this. It is not managing Mailman's logs
<sinzui> Why would the losas request that we add --MARK-- to the queue's log if they could do it themselves
<sinzui> q is a dipthong for kw
<lifeless> I think asking a losa is a good idea at this point :)
<sinzui> lifeless: Mailman's Syslog puts a write lock on the log. As my test shows, some insider knowedege and access is needed to acquire access,
<EdwinGrubbs> sinzui: what is on the receiving end of the syslog() call in the xmlrpcrunner then? Configuring syslogd to log MARK just shows that syslog is working, it doesn't show that the programs using that logfile are working.
<sinzui> And be assured I toppled mailman several times learning the art of stealing the logs
<lifeless> wheeee
<lifeless> EdwinGrubbs: I think the point is that we don't know if xmlrpcrunner is hung or not
<sinzui> Edwin Mailman.logging.syslog is a dict-like object that manages Mailman.logging.StampedLoggers. Each logger manages a file with 'a+b'
<sinzui> Edwin each object in the chain ensures the file is closed if you were to call del on the instance
<sinzui> EdwinGrubbs: Mailman really thinks about logging like it was the 10 years ago. It does not even have bools
<EdwinGrubbs> sinzui: ok, that is bizarro
<sinzui> But it would life to use with `open(ccc) as log:` Mailman 3 might do that
<sinzui> EdwinGrubbs: I believe mailman requires Python2.1 to run
<sinzui> EdwinGrubbs: I was looking for barry to asking about SHUNTING. I will bring up the log issue with him
<sinzui> when I find him
<EdwinGrubbs> sinzui: you can keep using MARK if you want, since mailman's syslog() doesn't log to syslogd. This reminds me of the time a friend created a readFoo() function that had a parameter to turn on writing.
<EdwinGrubbs> I'll approve the mp
 * sinzui is still looking for barry
<sinzui> EdwinGrubbs: thanks. I really want to talk to barry. I an reading an old log and I makes me wonder if his and mthaddon's analysis is complete. I see log entries that imply my change will work by accident, in some cases
<StevenK> sinzui: I can find Barry in Dallas if you wish
<StevenK> (And tempt him onto IRC)
<sinzui> please do
<StevenK> sinzui: Aye
<barry> sinzui: be with you in a few minutes
<sinzui> fab
<StevenK> \o/
<jtv> jcsackett: thanks
<barry> sinzui, okay, i'm all yours
<sinzui> Looking at a mailman error log. I see that messages were shunted because of
<sinzui> XMLROC proxy errors in LPHandler. I do not think I want the message shunted
<sinzui> since Runner implies IncomingRunner or LPHandler should have done something
<sinzui> itself.
<sinzui> I will wrap the proxy call to catch and raise oopses when xmlrpc goes down, but I think I want the message reenqueued
<barry> i can't think of a good way to prevent Runner._oneloop() from shunting when _onefile() throws an exception
<barry> short of hacking the code, or catching the exception in _onefile()
<sinzui> yuck
<barry> well, actually, maybe you can set self._shunt to the retry queue
<sinzui> I hoped for a way to reenque from the handler, or raise a specific error that tell IncomingRunner to do it
 * barry thinks
<sinzui> How will a shunted message ever be processed. No one manages mailman
<barry> it has to be done manually via bin/unshunt
 * barry shudders to think how big the shunt queue is
<sinzui> I think then we have several years in there
<barry> you almost certainly don't want to unshunt the whole queue ;)
<StevenK> How can we see how large the queue is?
<thumper> barry: hi!
<sinzui> So as I was thinking. We just need to pause the queue until connectivity is back. just loop over the message
<barry> thumper, hi!
<barry> StevenK, ls queue/shunt
<barry> er, queues/shunt
<barry> note tho that the queue files have a timestamp encoded in the file names so you could potentially decode that and only unshunt the ones from whatever date you care about
<barry> not sure what you mean by "until connectivity is back"
<sinzui> barry, what would you do if you LPHandler has no connectivity to complete its task. this is like a lock on a list. I see we try to return 1 to imply try again later
<barry> sinzui, right.  just leave the message in the queue and it will be picked up the next time 'round
<barry> or, enqueue it to queues/retry, but all that does is requeue the message every so often
<barry> so it's about equivalent
<sinzui> barry: Sometimes the xmlrpc request times out, but the next try works. But on Dec 1, it went down and mailman stopped trying.
<barry> sinzui, any idea why it stopped trying?
<barry> another idea...
<sinzui> barry: Log looks like Mailman got board shunting
<sinzui> restart worked fine
<barry> ;)
<barry> so, the .pck files have two objects in them: a dict (the message metadata) and the message object
<sinzui> My branch to add a heartbeat will warn us in the future
<sinzui> barry: how would I signal/do an enqueue from a handler
<barry> it would be a bit slow but you can unpickle the .pck file and look in the dict for tell-tale signs that LPHandler is where the failure occurred
<sinzui> barry: I can see where the failure happen. There are only two calls in out monkeypatches that do not try/except arround the proxy. I am sure we can log this. I just want solve how to keep the message alive
<barry> sinzui, you just instantiate the queue you want and call .enqueue() on it.  note tho that that makes a copy of the message, so absent anything else, it continues to get processed by the current runner
<sinzui> barry: from inside the handers except clause? That sounds like recursion
<barry> sinzui, so i think you catch the exception, instantiate the retry queue, enqueue the message, then reraise the exception (or raise DiscardMessage)
<barry> just be sure we're running the retry runner (i don't remember and i don't have the lp code in front of me)
<barry> StevenK, do you have the lp code handy?
<StevenK> barry: Yes
<sinzui> ahh, good point. If I raise the exception, it get shunted. raising discard just removes the copy in in the queue?
<barry> sinzui, yep
<barry> look at IncomingRunner.py
<StevenK> barry: You'll need to come to me, my battery is 4% or so
<barry> StevenK, what room are you in?
<sinzui> barry: I know how to check that and I know how to make it run
<StevenK> barry: Desktop
<barry> StevenK,sec...
<sinzui> barry: we are running the retryrunner. I wrote a test to ensure it is. Thanks I really appreciate your help
<barry> np.  looking at the code now
<barry> it's been a while ;)
<barry> sinzui, which call is failing?
<barry> proxy = proxy = ? ;)
<sinzui>   File "/srv/lists.launchpad.net/production/launchpad-rev-9972/lib/lp/services/mailman/monkeypatches/lphandler.py", line 49, in process
<sinzui>     is_member = proxy.isRegisteredInLaunchpad(sender)
<sinzui> barry: there that is one of two places we call proxy that we do not wrap in try/except
<barry> sinzui, yeah, for some reason i think the preceding comment is a lie ;)
<sinzui> barry: I was going to reuse the except rules an log_exception from xmlrpcrunner on it
 * barry thinks
<sinzui> barry: This is one of many entries in the log http://pastebin.ubuntu.com/553793/
<barry> yeah, it's not a lie because xmlrpcrunner isn't incoming runner
<sinzui> This line and the one in lpstanding is just an oversight. All other calls are safe to use, but this one cold lose the message
 * pcjc2 struggles to think of concise words to describe the SQL statements reasoning in his docstrings
<barry> sinzui, so yeah, i think the best you can do is to catch the exception, enqueue the message to retry, and raise DiscardMessage.  then the retry runner should requeue it, and when it's re-processed by incoming runner it should start back up at the LaunchpadMembers handler
<barry> sinzui, write some tests, but i think that should work ;)
<jcsackett> pcjc2: "Because the final clause only checks the existence of results of the query, subqueries can select true rather than an actual entity, speeding up the process."
<jcsackett> maybe.
<jcsackett> it's a little clunky. :-P
<pcjc2> I'm moving the EXISTS keyword into the build_tag_set_query
<pcjc2> function
<pcjc2> then it returns a TRUE or FALSE, and it is cleaner - as well as easier to explain
<pcjc2> might not even NEED to explain the SQL in detail if that change is made?
<pcjc2> http://pastebin.com/EfHtBNdE
<pcjc2> ?
<pcjc2> THink I will drop the comment about where it is used
<pcjc2> that could get out of date fast
<pcjc2> Will add one about expecting the surrounding SQL to ensure "Bug.id" is in scope
<pcjc2> what about http://pastebin.com/QswKQ2Lg ?
<pcjc2> See the diff: http://pastebin.com/9iViJh6Y
 * pcjc2 re-runs tests to be sure
* jcsackett 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
<jcsackett> pcjc2: i would put quick comments above the string format statements, since you end up with things like "%s %s %s".
<pcjc2> comment to what effect?
<pcjc2> at some point, one has to read the context of the code ;)
<jcsackett> pcjc2: that's an excellent question, actually.
<jcsackett> yeah, this is just an inherently complicated bit.
<jcsackett> looks weird, but you're right, it doesn't need anything more.
<jcsackett> test passing?
<pcjc2> "%s %s NOT %s" is a bit strange, I conceed
<pcjc2> give it a month or two to run...
<jcsackett> yeah, it's odd, but i think it's alright.
<jcsackett> lol.
<pcjc2> (just wanted to check the "testbug.py" test, as it compares SQL output - had to make sure I'd not shifted the way the SQL was braketed
<pcjc2> I'm not going to re-run the other tests as the SQL is the same..
 * jcsackett nods.
<pcjc2> Will need some manual QA though, as I'm not sure how good the "life" test coverage is
<jcsackett> ?
<jcsackett> which test are you referring to by "life" test?
<pcjc2> "live" I meant
<pcjc2> with a big data-set
<pcjc2> it aims to fix a problem which needs a production sized DB to reproduce
<pcjc2>        return "%s" % include_clause
<lifeless> pcjc2: we qa all such things ;)
<pcjc2> should become "return include_clause"
<lifeless> pcjc2: on qastaging.launchpad.net
<lifeless> perhaps
<lifeless> return str(include_clause)
<pcjc2> include_clause is assembled from strings
<lifeless> or even unicode(include_clause)
<pcjc2> we don't prefix the return "(%s %s NOT %s)" % ... case with u"
<lifeless> pcjc2: sure, explicit can be good sometimes (also its a noop if the type matches)
<thumper> sinzui: https://code.launchpad.net/~thumper/launchpad/daily-build-page-text/+merge/46207 for your UI approval :)
<sinzui> okay
<pcjc2> lifeless - name your preference... unicode, str ?
<pcjc2> meh - actually, it is only the one "NOP"  return "%s" % include_clause
<pcjc2> the other cases around it need that syntax, so it is best we keep them consistent
<pcjc2> even if "%s" % ...  is  a NOOP
<pcjc2> what is the SQL'esque way of fixing this comment...
<pcjc2> This SQL is designed to be a sub-query where the parent SQL defines Bug.id.
<pcjc2> "defines" seems wrong
<pcjc2> JOINs/includes/contains/?
<jcsackett> pcjc2: i have to run for the evening. i'll check the MP in the morning for the changes. ttyl.
<pcjc2> "parent query scope includes Bug.id" ?
<pcjc2> @jcsackett: Thanks for the review - with luck the MP will be in good order by tomorrow
<jcsackett> pcjc2: sounds good.
<lifeless> pcjc2: I would leave it as is
<pcjc2> as it was?
<lifeless> pcjc2: it looked ok at a glance
<lifeless> as was, yes
<pcjc2> ok, will push that diff
<lifeless> jcsackett's two comments were good suggestions
<pcjc2> Do I need to mark the MP as needing further review?
<pcjc2> Review comments have been taken into account, new push coming...
<huwshimi> sinzui: Hi. Would you be able to review a CSS bug fix sometime (bug #684911: https://code.launchpad.net/~huwshimi/launchpad/broken-calendar-684911/+merge/46208)?
<_mup_> Bug #684911: calendar overlay widget style broken after lazr-js 191 upgrade <javascript> <lazr-js-upgrade> <lp-web> <regression> <ui> <Launchpad itself:Triaged> < https://launchpad.net/bugs/684911 >
 * sinzui hates that widget
<sinzui> you cruel man.
<huwshimi> sinzui: Is that a no then? :P
<sinzui> ah, so the hack to for the yui-3 skin was lost in the last update?
<huwshimi> sinzui: Looks like something like that. In which case, me fixing the styles this way would only be a temporary fix
<huwshimi> I don't really understand how all the lazr-js stuff works yet but maybe the real fix is something to do with that
<sinzui> We wanted a yui-3 widget, but one does not exist. We could make a lazr one that handles NOW
<sinzui> this is good to land. Thanks huw.
<huwshimi> Yeah ok that makes sense. Thanks
<sinzui> Oh, the fonts are wrong
<huwshimi> Ah yes
<sinzui> huwshimi: I will find the correct font-family string
<sinzui> I remembered that we had to hack the fonts as well as the skin 18 months ago
<pcjc2> @jcsackett: My MP should be ready to go when you get to it tomorrow - thanks
<sinzui> huw: do you have a strong understand of how browsers (last 5 year) handle font-size. YUI requires us to use % to guarantee consistent rendering, but that also means the engineer never knows what the actual font-size will be when all content and rules are mixed together
<sinzui> huw I want to switch back to ems, or even px as suggested by Canonical web guidelines if I could trust consistent sizes
<huwshimi> sinzui: Yeah font sizing gets very confusing very quickly.
<huwshimi> Especially when you won't necessarily have control over third party widgets etc.
<sinzui> I used to avoid px. I was surprised the Canonical insists on it. I think they may be looking a how browser engines handle px though. My phone shows me pages and I know it is ignoring the px rules
<huwshimi> With the calendar I can either set the font to the Ubuntu one or remove the font-family settings altogether (which will then inherit the correct fonts)
<huwshimi> Any preference
<huwshimi> ?
<sinzui> YUI is specifically reconciling Window with every other os/basefont. I am not sure I care about windows
<lifeless> sadly we care
<lifeless> because our customers have customers and suppliers on windows
<sinzui> huwshimi: I think we need to set it. Somewhere in the chain, verdana or tahoma gets added
<sinzui> lifeless: I think the issue windows fonts being a little big is less than the issue that we are not using the font-sizes that the CSS predicts will rendered. % rules are being nested. We cannot avoid that
<huwshimi> sinzui: On a quick test of removing all font-family lines from calendar.css it seems to be working correctly.
<sinzui> lifeless: http://curtis.hovey.name/2010/12/17/launchpad-font-size-broken-by-design/
<sinzui> huwshimi: fab
<huwshimi> sinzui: Should I go ahead and commit without font-family rules then?
<sinzui> huwshimi: yep
<huwshimi> ok great thanks.
<lifeless> sinzui: I can see the brain damage on linux too ;)
<lifeless> sinzui: would love it if we fix this.
<sinzui> lifeless: I am hoping for a short brain storm session + a spike at the thunderdome to find a compromise that will let me close a UI bug
<huwshimi> sinzui: When I try and land the branch it complains that the branch is not approved. On the MP the status is approved. Do I need to do anything else to get it to land?
#launchpad-reviews 2011-01-14
<huwshimi> sinzui: Could it have something to do with me checking in those font changes after you approving it?
<sinzui> huwshimi: sorry. I was summed to make dinner for my family. I approved the MP and I see you pushed your revision, so I expect ec2 land your branch
<huwshimi> sinzui: No problems. The issue is when I run ec2 land it complains that the branch isn't approved. Any ideas?
<sinzui> looks approved to me
<huwshimi> sinzui: yeah, me too!
<sinzui> maybe it thinks it needs a code review too
<sinzui> I will add that
<huwshimi> Ah right that might be it
<sinzui> I changed my review to code and ui
<sinzui> note that I cannot have two separate reviews. That oddness leads to confusion
<huwshimi> Great, it's working now. Thanks for that
<lifeless> there is a bug on that
<allenap> Good morning adeuring :) Mind if I enqueue myself for a review?
<adeuring> allenap: sure, go ahead. Give a few minutes to read mails ;)
<allenap> adeuring: Cheers.
* allenap changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> allenap: r=me
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Thanks!
<wgrant> adeuring: Hi.
<adeuring> wgrant: hi; already looking at your LFA branch :)
<wgrant> adeuring: Thanks!
<wgrant> adeuring: If you end up getting to the others, note that bug-629835-copier-architectures depends on bug-701383-ppa-component-override.
<adeuring> ok
<allenap> adeuring: I've got another one for you, short though. https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-6/+merge/46246
<adeuring> allenap: I'm currently looking at a branch from StevenK; when I've finished that one, I'll look at yours
<allenap> adeuring: Thank you :)
* allenap changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> allenap: r=me
<allenap> adeuring: Thank you!
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring 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
<gmb> allenap, deryck: Could one of you good fellows review my 200-line JS refactoring in https://code.edge.launchpad.net/~gmb/launchpad/prepare-for-alpha-features-bug-699719/+merge/46254?
<gmb> It's incremental towards fixing bug 699719
<_mup_> Bug #699719: Update the subscription JS to use the new subscription widget <story-better-bug-notification> <Launchpad itself:In Progress by gmb> < https://launchpad.net/bugs/699719 >
<deryck> gmb: I can take it.
<gmb> deryck: Thanks.
<deryck> np
<deryck> gmb: so this is a pure code move?  Just moving all the event stuff into a wrapping function?
<gmb> deryck: Yep. It turned out to be the simplest way of doing what I wanted to do. Now I can set a boolean flag before callign setup_bugtask_index and the advanced subscriptions stuff will be enabled.
<gmb> Implementationally speaking it's a hack, but it's not the ugliest hack in the world.
<deryck> gmb: yeah, it's not bad.  it's cleaner code anyway.  at least this part.  r=me.
<gmb> deryck: Thanks.
<deryck> gmb: np
<jtv> Any reviewers about?
<jtv> https://code.launchpad.net/~jtv/launchpad/bug-702832/+merge/46261
<StevenK> Rargh, Abel disappeared without reviewing my branch
<gmb> StevenK: He's travelling to Dallas. How big's your branch?
<StevenK> gmb: 737 lines (+188/-455) 3 files modified
<StevenK> gmb: It is ongoing work to kill a 1,600 line doctest, does that sweeten the deal?
<gmb> StevenK: I'll take a look at it within the next half hour.
<gmb> And yes, that helps :)
<gmb> StevenK: Send me a link and I'll claim the review
<StevenK> gmb: https://code.launchpad.net/~stevenk/launchpad/bpb-currentcomponent-assertion-part-3/+merge/46209
<gmb> Righto. I'll ping you when I'm done
* 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
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [jtv,Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> StevenK: r=me; There are a few instances of "# Test that ..." that need to be turned into a statement of expected behaviour, but I know you'll take care of those.
<gmb> StevenK: Is it worth getting someone with more domain knowledge than I to check that we haven't lost anything in the transition?
<StevenK> gmb: You can if you wish -- do you feel we need to?
<gmb> StevenK: I don't know. I don't know nuffink about Soyuz. That's why I asked you :)
<gmb> StevenK: If you're confident, I'm happy to trust your judgement.
<StevenK> gmb: I'll get bigjools to have a quick glance.
<StevenK> bigjools: *hint*
<bigjools> meh
<StevenK> bigjools: If you don't care, then we'll go with gmb's r=
<bigjools> StevenK: url?
<StevenK> It's only test fixes, what could possibly go wrong
<StevenK> https://code.launchpad.net/~stevenk/launchpad/bpb-currentcomponent-assertion-part-3/+merge/46209
<bigjools> my eyes
<gmb> Woah, hang on...
<gmb> Have I managed to miss a big chunk in the middle here?
<bigjools> those tests are insanely hard to follow
<gmb> Erk, PEBKAC on my part.
<gmb> I've missed part of the diff here.
<bigjools> so from a domain PoV it's fine, but I'm worried about the readability
<bigjools> some comments in test_copy_archive_without_leak would not go amiss
<gmb> Oh, nice.
<gmb> StevenK: So, I agree with bigjools.
<gmb> Also, the setUp() method for your first testcase.
<gmb> Needs comments.
<gmb> And I need to find out what made my browser refuse to show me the end of the page.
<gmb> But that can wait.
<bigjools> I don't know what leak means in that context either
<StevenK> bigjools: So I add two builds to the primary archive, copy them to a .COPY archive, createMissingBuilds() and then verify that looking up getBuildRecords returns *one*, not two
<bigjools> StevenK: so I would recommend changing the comment at the very top to explain that
<bigjools> and then break the code up into sections with a comment at the top of each
<StevenK> Okay
<StevenK> gmb: So, please point out which bits you feel that needs more comments on the MP
<gmb> StevenK: Will do. Sorry for the slack-arsed review. There's a lesson in this: don't ask me to do anything important on a Friday afternoon.
<StevenK> gmb: I didn't, you offered :-)
<gmb> StevenK: Then never let me offer to do anything.
<StevenK> Haha
<leonardr> sinzui, do you approve https://code.launchpad.net/~leonardr/launchpadlib/remove-xslt/+merge/46167 ? the mp is still waiting on your input
<sinzui> okay
<leonardr> sinzui: while you're at it, maybe you can review this one-line change: https://code.launchpad.net/~leonardr/launchpadlib/fix-upload-tarball/+merge/46276
<sinzui> leonardr, I am so sorry i did not update the MP. It is approved
<leonardr> ok, cool
<sinzui> leonardr, does this mean we have bad historic content-types in the db?
<leonardr> sinzui: no, the bad content type is in the script
<sinzui> okay.
<leonardr> launchpad is ignoring the bad type and using text/html instead
<sinzui> r=me
<leonardr> our releases of lazr.restful etc. have the same problem, but nobody installs them using easy_install, so people only complain about beautiful soup
<sinzui> even web forms are hard to use without working menus
<benji> darn, the lack of reviewers for this time of day on Friday always catches me off guard
<salgado> benji, how big is your branch?  maybe I can help
<benji> salgado: here's the branch https://code.launchpad.net/~benji/launchpad/bug-636193 I haven't written a MP yet, but if you have time to look at it I'll write one up.
<salgado> benji, it looks simple enough; I'll take it
<StevenK> gmb: Still around?
<salgado> benji, but there seems to be a conflict between your branch and mainline
<benji> salgado: I'll take a look at that and then get you a MP; thanks
<salgado> benji, wow, the diff is big. by looking at the bug's description I thought it'd be simple.  I don't think I'll be able to do it as I'm on a sprint right now, sorry
<benji> salgado: no problem, I appreciate the attempt :)
<EdwinGrubbs> sinzui: do you want to review my branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-663857-product-packages-timeout/+merge/46222
<sinzui> kay
<sinzui> okay
<StevenK> sinzui: Has Unity given in yet?
<sinzui> StevenK,  No, latest build failed; https://launchpad.net/ubuntu/+source/unity
<StevenK> Yeah, I saw that.
<StevenK> sinzui: Like I said before, I'm happy to bug people in Dallas to help you.
<sinzui> I am sure they will notice
<StevenK> sinzui: Well, are you sure the latest version will fix your problem?
<sinzui> I have a computer behind me that lets me count the places in the menu I cannot see to use menus
<sinzui> Since packages are arriving that lets me restore what went missing, I think upgrading is the solution. I have everything but Unity and some compiz package
<sinzui> still the no-menu situation is very weird. I think it affects every instance of GtkMenu.
<StevenK> It will, it sounds like a bug with dbusmenu
* sinzui changed the topic of #launchpad-reviews to: On call: - || reviewing: Edwin || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> gksu never comes to for foreground. I discovered I can resolve may lockups my typing my password
<sinzui> see approving an MP has a dropdown menu that I cannot see.
<sinzui> EdwinGrubbs, r=me
<EdwinGrubbs> thanks
* sinzui 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
* sinzui 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
