#launchpad-reviews 2010-03-08
* 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
<adiroiban> henninge: hi. Can you please land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-509252-take-2/+merge/19484 ?
<henninge> adiroiban: yes, sure.
<henninge> Hi adiroiban!
<henninge> ;-)
<adiroiban> gmb: Hi. Do you have time to land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-512307/+merge/20442 ?
<gmb> adiroiban: Sure.
<adiroiban> thanks gmb and henninge ! :)
* wgrant changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue: [wgrant] || 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: henninge || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: looking at your merge proposal now.
<wgrant> henninge: Thanks. It's an old branch, so I just realised I forgot to describe some of the changes.
<wgrant> I've removed some redundant templates and classes.
<henninge> wgrant: yes, I already noticed ... ;)
<jml> https://code.edge.launchpad.net/~jml/launchpad/expose-gpgkeys-bug-389872 <-- would someone please review this branch?
<henninge> jml: put it in the queue, please. I might not get to it because I am not feeling too great and might pack up after wgrant's branch but intellectronica and abentley should be around later.
<henninge> actually, were *is* intellectronica?
<henninge> where
* jml changed the topic of #launchpad-reviews to: on call: henninge || reviewing: wgrant; jml || 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: henninge || reviewing: wgrant || queue: [jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> henninge, I thought it _was_ in the queue by virtue of being ready for review, tbh.
<henninge> jml: hm, we are still using the queue here, too.
<henninge> jml: to indicate that you want an OCR to do the review (and that you are likely to be around for it).
<henninge> that is my understanding
<jml> ok.
<wgrant> bigjools: I like that buildd-manager logging fix.
<bigjools> wgrant: you bet
<bigjools> wgrant: it needs a little more love - like logging when stuff is givenback, etc
<wgrant> bigjools: Yeah, but now it's actually a bit useful.
<bigjools> it could definitely not be more useless than before
<henninge> wgrant: I understand that your branch does not change the looks (apart from page_title and label) of any of the pages, right?
<wgrant> henninge: It moves the heading of https://edge.launchpad.net/builders/thorium/+history to the heading slot, but apart from that no.
<wgrant> (note how it's below the breadcrumbs there)
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: wgrant || queue: [jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: that is standard 3.0 fix. Thanks for spotting that!
<intellectronica> henninge, jml: i can review
<henninge> intellectronica: Hi! cool
<jml> intellectronica, thanks.
<intellectronica> jml: expose-gpgkeys-bug-389872 ?
<jml> intellectronica, thanks.
<intellectronica> i'll take that as a confirmation
<jml> intellectronica, thanks :)
<henninge> wgrant: r=me ;)
<henninge> wgrant: want me to land it?
<wgrant> henninge: Please do.
<wgrant> Thanks.
<intellectronica> jml: the import from launchpad.interfaces in lib/canonical/launchpad/browser/logintoken.py needs to be broken up
<intellectronica> i feel bad for asking you to do that, since you're just driving by, but if you don't who will?
<henninge> wgrant: it's on its way ...
<wgrant> henninge: Great.
<jml> intellectronica, "Daddy, where were you when the interface imports needed breaking up?" etc....
<intellectronica> exactly
<jml> intellectronica, sure, I can do that. (not right now though)
<intellectronica> jml: the docstring for retrieveActiveKey is oddly self-referential
<jml> intellectronica, in a bad way, right?
<intellectronica> jml: when you say 'not right now' do you mean 'not in the next few hours' or 'not as part of this branch'?
<jml> intellectronica, the former.
<intellectronica> cool
<jml> actually, maybe in the next few hours
<jml> but not before lunch, almost certainly
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: [jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> jml: and yeah, the docstring being self-referential is kinda' bad. it's cute, but it makes it impossible to understand.
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, jml || 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: henninge, intellectronica || reviewing: lunch, jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * henninge lunches
<intellectronica> jml: are you supposed to be able to change the keyid and fingerprint attributes via the api?
<jml> uhh no, that sounds kind of horrible
<intellectronica> jml: right. so i think they should be declared read only
<jml> intellectronica, ok, will do.
<intellectronica> jml: also, pendinggpgkeys and gpgkeys should be pending_gpg_keys and gpg_keys
 * jml agrees
<wgrant> intellectronica, jml: Those collections are also readonly=False.
<intellectronica> wgrant: i'm not sure that means you can add items to them, though. in fact that's exactly what i'm trying to find out
<intellectronica> though slapping a readonly on them surely can't hurt
<wgrant> intellectronica: You can't add items to them, no.
<wgrant> But it still seems like a bad idea for something like that.
<wgrant> It probably has no practical effect at the moment.
<intellectronica> wgrant: yeah, i agree, if only for the code to serve as documentation
<wgrant> Right.
<jml> agreed.
<intellectronica> jml: everything else looks fine. i'm leaving the mp in needs fixing for now.
<jml> intellectronica, thanks. Are you going to summarize this conversation there? (If not, I will)
<intellectronica> jml: i was just about to do that. save your energies for all that interfaces work :P
<jml> intellectronica, :D
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: lunch, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: lunch, - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> wgrant: which branch?
<wgrant> intellectronica: https://code.edge.launchpad.net/~wgrant/launchpad/buildqueue-to-buildmaster/+merge/20886
<wgrant> Big, but mostly s/soyuz/buildmaster/
<intellectronica> oh joy
<al-maisan> :)
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: lunch, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> wgrant: r=me
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: lunch, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> intellectronica: Thanks. Can you please (ec2)land it?
<intellectronica> wgrant: sure
 * wgrant throws https://code.edge.launchpad.net/~wgrant/launchpad/fix-build-breadcrumbs/+merge/20888 onto the pile, as a successor to the earlier one.
* wgrant changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: lunch, - || queue: [wgrant] || 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: henninge, intellectronica || reviewing: -, - || queue: [wgrant] || 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: henninge, intellectronica || reviewing: wgrant, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> I have a 1500 line branch which moves an enum, removes a single circular import hack that becomes unnecessary as a result of the move, removes an unused list of a subset of the enum values, and fixes all of the imports. Is anybody game? 90% of it is import changes, and it's not really possible to split it up significantly...
<intellectronica> wgrant: that's fine. i have a call soon, but i can review the branch when i'm done.
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: r=me on the breadcrumbs but I think noodles775 should formally add a ui review, too.
<wgrant> henninge: Thanks.
<noodles775> Sure.
<wgrant> intellectronica: Review requested. I'll hopefully be asleep by the time you get to it -- please land if you're OK with it. Thanks!
<wgrant> noodles775: Thanks.
<intellectronica> wgrant: will do. sweet dreams.
<james_w> could someone land my two approved branches please?
<adiroiban> henninge, danilos : do you think we can schedule a pre-implementation chat for IPOTemplates API ?
<henninge> adiroiban: sure but not today ... ;)
<adiroiban> henninge: sure. no hurry :)
<danilos> adiroiban, sure
<adiroiban> or maybe can do it by email ?
<adiroiban> I have a prototype and maybe we can discuss around it
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> intellectronica: I have a critical issue to handle in rosetta but it looks like smooth sailing here atm.
<intellectronica> henninge: np. i have some calls, but will resume reviewing after that
<danilos> adiroiban, sounds good, I am CHR today so I am unlikely to have time for it, but if you send it by email I can probably take a look at your ideas and your branch, and then we can talk some more tomorrow
<danilos> adiroiban, how does that sound?
<adiroiban> good :)
* abentley changed the topic of #launchpad-reviews to:  on call: intellectronica, abentley || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * kfogel is away: reboot
<jtv> intellectronica: I'm actually asleep, but perhaps you'd like to have a look at my branch?  https://code.launchpad.net/~jtv/launchpad/bug-533668/+merge/20902
<intellectronica> jtv: sorry, but i'm unlikely to get to it today
<jtv> intellectronica: no worries
* intellectronica changed the topic of #launchpad-reviews to:  on call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> abentley: if you're bored, I've got a small branch up for review
<abentley> jtv, sure.
<jtv> thanks
<abentley> jtv, why are you using foo.__repr__ instead of repr(foo)?
<jtv> abentley: ah, I knew I was missing something.  I'll fix that.
<abentley> jtv, how are you expecting boom to appear in the message?  "\u2639"?
<jtv> abentley: I don't much care actually, as long as (1) the script doesn't crash, (2) some message gets out, and (3) the origin of the message isn't completely obscured.
<abentley> jtv, do you know why the original code was special-casing unicode(e) == ''?
<jtv> abentley: IIRC there was some case where the message was empty and the interesting info was in the exception type itself.
<jtv> So this change takes away the need for that.
<abentley> jtv, r=me
<jtv> abentley: cool, thanks
<james_w>  on call: abentley || reviewing: - || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* james_w changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * james_w always seems to get it wrong once
* abentley changed the topic of #launchpad-reviews to:  on call: - || reviewing: - || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-03-09
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/new-code-import-email-show-type/+merge/20938 if you're bored
<mwhudson> thumper: on it
<thumper> mwhudson: so you're bored?
<mwhudson> thumper: a bit
<mwhudson> writing specs is tedious
<mwhudson> also, i thoroughly approve of that feature!
<mwhudson> thumper: do you think having RevisionControlSystems.CVS.title be "Concurrent Versions System" is helping anyone ?
<thumper> mwhudson: no
<thumper> mwhudson: I suppose we could add a little map
<mwhudson> i was thinking this with my +code-imports changes the other day
<thumper> and show CVS, Subversion, git and hg
<thumper> mwhudson: what did you do the other day?
<mwhudson> thumper: well, the +code-imports page will soon have a <select> for rcs type
<mwhudson> and one of the options will be "Concurrent Versions System"
<thumper> mwhudson: heh
<mwhudson> same for "Subversion via bzr-svn" i guess
<mwhudson> for Mercurial/hg, well, don't care
<thumper> mwhudson: yeah
<thumper> mwhudson: also https://code.edge.launchpad.net/~thumper/launchpad/description-change-email/+merge/20939
<thumper> mwhudson: new revision pushed with nicer names
<mwhudson> thumper: i find the description-change-email branch a bit mysterious i have to admit
<thumper> mwhudson: ok
<thumper> mwhudson: I'm going to be afk for a bit
<mwhudson> ok
<mwhudson> i'll figure it out
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad), noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> Can I convince somebody to review https://code.edge.launchpad.net/~wgrant/launchpad/buildstatus-to-buildmaster/+merge/20892? It's supermassive due to an enum move, but trivial.
<noodles775> wgrant: sure
<allenap> BjornT: Would you be able to review my isolate-tests branch? It's kind of "interesting" and I'd like you to ask awkward questions of it. It's short. https://code.edge.launchpad.net/~allenap/launchpad/isolate-tests/+merge/20916
<BjornT> allenap: sure, that looks like something that is best reviewed after lunch :)
<allenap> BjornT: Heh :) Thank you.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad), noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> james_w: Aha, both of your branches are ready to land now, right/
<gmb> ?
<noodles775> Hi gmb, when you get to it, mine is this one: https://code.edge.launchpad.net/~michael.nelson/launchpad/530180-partner-permissions/+merge/20947
<gmb> noodles775: Thanks.
<gmb> james_w: Is there a bug for https://code.edge.launchpad.net/~james-w/launchpad/fix-getRequestedReviews/+merge/20378? Also, can you add a commit message for it before I land it please?
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: noodles775 || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775: BTW; it's 2010. You should update your doctest copyright notice ;). (Bonus points for doing the template, too).
<noodles775> heh, will do.
<gmb> noodles775: Other than that, r=me.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [james_w (https://code.edge.launchpad.net/~james-w/launchpad)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb
<salgado> adiroiban, I just tried landing your branch but there are conflicts in lib/lp/registry/model/distroseries.py
<adiroiban> salgado: I will merge with latest devel and push the changes.
<adiroiban> I'm still trying to fix those randomly failing windmill tests
<adiroiban> the buildd-slavescanner error was fixed in a later branch
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adiroiban, do the windmill tests fail on a mainline branch for you as well?
<adiroiban> yes
<salgado> adiroiban, then it's probably not related to your changes, so I'll just resubmit your branch and see what ec2 says
<adiroiban> doing this change, reduce the probability of failing http://paste.ubuntu.com/391757/
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> gmb: can you please try again to send this branch to ec2-land https://code.edge.launchpad.net/~adiroiban/launchpad/bug-512307/+merge/20442 ?
<adiroiban> can someone please paste me the content from https://lpbuildbot.canonical.com/builders/lp/builds/648 ?
<adiroiban> I got an email from buildbot, but I can not see that link
<salgado> adiroiban, http://paste.ubuntu.com/391781/
<adiroiban> salgado: strange output. I don't know why it arrived in my inbox
<salgado> adiroiban, buildbot ran the testsuite after merging your branch and encountered these failures.  that's why you got it
<adiroiban> which branch (there is no hint in the email)? was it landed or needs to be resubmited?
<salgado> it's landed already.
<salgado> http://paste.ubuntu.com/391784/ has the details about the branch
<gmb> adiroiban: Sure, I'll re-run that.
<adiroiban> many thanks!
<allenap> BjornT: Can I gently nudge you again to take a look at isolate-tests?
<BjornT> allenap: yes, you can :)
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb, another simple soyuz one for ya: https://code.edge.launchpad.net/~michael.nelson/launchpad/527617_aborted_build_job/+merge/20968
<gmb> noodles775: MutualExclusionError: Terms "simple" and "soyuz" cannot exist in the same sentence.
<gmb> But otherwise, sure.
<noodles775> gmb: heh, take a look and see :)
<noodles775> Thanks!
<gmb> Wow.
<gmb> noodles775: I do believe that you may have just done the impossible. I expect your proposal for a perpetual motion machine next week.
<gmb> noodles775: r=me
<noodles775> gmb: ;), thanks.
<leonardr> gmb, need a review of this trivial branch: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/str-or-unicode
<gmb> leonardr: On it.
<leonardr> gmb: i'll write a short mp
<gmb> leonardr: Hah. Jumped the gun there, didn't I?
<leonardr> gmb: as did i
<leonardr> mp is up
<gmb> Thanks.
<gmb> leonardr: r=me
<gmb> gary_poster: Are you happy for me to mark https://code.edge.launchpad.net/~gary/launchpad/bug529348rc/+merge/20776 as Approved? flacoste and salgado have given it the thumbs up but it's still lurking in the active reviews queue.
<gary_poster> gmb, there were some test failures that I'm still talking to salgado about.  That said, if you want to mark it approved to get it out of your way, that's fine. I won't suddenly stop working on it. :-)
<gmb> gary_poster: Well, since it's mostly approved barring fixing things I'll do that.
<gary_poster> cool, thanks
<noodles775> gmb: which template were you refering to (for a new unit-test), I wasn't aware of any?
<gmb> noodles775: standard(_test)_template.py in the root of the tree.
<noodles775> Thanks.
<gmb> noodles775: But it's already been done
<noodles775> ah.
<gmb> (I just assumed you'd used it)
<noodles775> Nope. But will next time.
<gmb> noodles775: So, submit away, and forget I ever said anything.
<sinzui> gmb: I fixed the status of pdds' branch
<gmb> sinzui: Thanks.
<jpds> gmb: Hi, can I have a review of https://code.launchpad.net/~jpds/launchpad/archivepublisher-log-to-file/+merge/20974 ?
* jpds changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  || queue: [noodles775,jpds] || 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 || reviewing: jpds  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> jpds: Sure.
<bac> gmb: i forgot to mention it, but i'm sprinting this week and won't be doing reviews today
<gmb> bac: No worries.
<bac> gmb:  cool.  just wanted to give you a heads up
<gmb> Cheers.
<gmb> jpds: r=me. Wnat me to land it for you?
<jpds> noodles775: Would it be better to let bigjools review tomorrow? ^--
<jpds> s/review/land/
<gmb> jpds: Was that meant to be addressed to me?
<noodles775> Up to you guys, personally I would (anything that affects the pubishing cycle should probably be glanced at by bigjools before landing)
<gmb> WFM.
<jpds> gmb: I think it would be best to let Julian take a final look before landing.
<gmb> jpds: Agreed. You should request a review from him in the MP then.
<jpds> gmb: Done, thanks.
<james_w> gmb: sorry for the delay, my proposals should be ready now
<james_w> both have bugs and commit messages
<james_w> is there a document somewhere on how to get a branch landed?
<james_w> it would be great to link such a thing from the topic to save on round trips
<gmb> james_w: There isn't, AFAIK. You should file a bug on launchpad-documentation so that we remember to take care of that :)
<gmb> james_w: I'll land those for you shortly.
* gmb 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
<james_w> thanks
<james_w> "There are currently no bugs filed against Launchpad Documentation."
<james_w> does that mean that there aren't supposed to be?
<james_w> aha, the overview explains
<EdwinGrubbs> bac: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-534462-project-index-timeout/+merge/20980
<rockstar> abentley, https://code.edge.launchpad.net/~rockstar/launchpad/bug-527450/+merge/20981
<abentley> rockstar, r=me
<salgado> adiroiban, did you get an email from ec2?
<salgado> (about that branch I submitted on your behalf)
<adiroiban> salgado: no
<adiroiban> I am not sure if ec2 emails are working. As Henning has submited one of my branch I did not received any email
<henninge> adiroiban: that's because I submitted it differently
<adiroiban> :)
<henninge> adiroiban: I created a new branch and merged yours
<henninge> I was having problems landing so I thought this would help
#launchpad-reviews 2010-03-10
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jtv: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-507642/+merge/20978 ?
<jtv> adeuring: how dare you ask such a thing
<jtv> especially of an oc...
<jtv> oh, wait, I guess I can
<adeuring> well I like to be a bit bold ;)
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: *adeuring*  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adeuring: bold enough for you
<jtv> ?
<adeuring> jtv: sure ;)
<jtv> adeuring: bug snapshots are new to me... what is that?
<adeuring> jtv: There is a calss Snapsshot somewhere... let me search again where exactly...
<jtv> adeuring: not Schnapps-shot, surely
<adeuring> jtv: erm, yes ;)
<jtv> lazr.lifecycle..?
<adeuring> jtv: yes
<adeuring> jtv: anyway, for POSTs to the webservice API, a snapshot of the changed object is created.
<al-maisan> jtv: is it OK if I enqueue https://code.edge.launchpad.net/~al-maisan/launchpad/partner-expiry-535030/+merge/20990 ?
<adeuring> and this involves creating shortlists of collection fields
<jtv> al-maisan: okay; I'm not too well though so be prepared for me dropping out at some point
<adeuring> and if these collections are not short enough, the request fails.
<jtv> adeuring: ok, I'll try to work with that
<jtv> adeuring: do I understand this correctly?  Snapshots are made of these lists on a bug, and that process runs into the hardlimit.  So you stop snapshots from being made, and to test this, you set a tighter hardlimit for snapshotting and prove that you're not running into it?
<adeuring> jtv: exactly
<jtv> adeuring: that sounds a bit like you're second-guessing doNotSnapshot
<adeuring> jtv: but the limit is reduced for tests only ;)
<jtv> yes, I got that :-)
<adeuring> jtv: and yes, that's the core idea of the test.
<adeuring> jtv: it is a kind of simulation of the situation we have currently when you try to subscribe to bug 1
<mup> Bug #1: Microsoft has a majority market share <iso-testing> <ubuntu> <Clubdistro:Invalid> <Computer Science Ubuntu:Invalid by compscibuntu-bugs> <EasyPeasy Overview:Invalid by ramvi> <Ichthux:Invalid by raphink> <JAK LINUX:Invalid> <OpenOffice:Invalid by lh-maviya> <Tabuntu:Invalid by tinarussell> <Tivion:Invalid by shakaran> <Ubuntu:In Progress by sabdfl> <ubuntu-express (Ubuntu):Invalid by jahyire2006> <Ubuntu Jaunty:In Progress> <ubuntu-e
<jtv> adeuring: let's say I were to have a lot of faith in my fellow developers and reviewers in lazr-lifecycle.  Then I'd sort of expect a very similar test to have happened in their code already.
<jtv> yes thank you mup, we hadn't forgotten that one
<adeuring> jtv: well, perhaps. But what would this mean for the changes in my branch?
<jtv> adeuring: that I think you're doing too much work
<adeuring> maybe...
<jtv> adeuring: I haven't read the full diff yet, but how about just testing that these fields are marked as doNotSnapshot, and saying why they have to be?
* al-maisan changed the topic of #launchpad-reviews to: on call: jtv || reviewing: *adeuring*  || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jtv: let's assume that the necessity arises that we must include  one of the collections again in the snapshot.
<jtv> adeuring: and frankly I'm much more interested in the functional consequences of not snapshotting these things... what differences will it make to users besides not exploding?
<jtv> adeuring: that's a very good assumption; but if your test can just show _that_ the fields are doNotSnapshot, then your test will trip this up.
<adeuring> jtv: I haven't noticed any. Discussed this with other people in the bugs team -- and all we am eup with: "try possibly relevant tests"
<jtv> heh
<adeuring> and none of those I tried did fail
<adeuring> jtv: well, if we simply check a doNotSnapshot "decoration", we do not check if POSTs succeed or fail
<bigjools> stub, gmb: thanks for reviewing my branch to change the update-pkg-cache dbuser, however the diff that LP generated is total garbage!
<gmb> bigjools: Really?
<bigjools> I think a db-devel/devel mismatch
<jtv> bigjools: branched off devel, mp'ed for db-devel?
<gmb> bigjools: Hah.
<gmb> Oh, jow.
<bigjools> jtv: other way around
<gmb> Joy, even.
<jtv> bigjools: then you're an idiot
<jtv> :-P
<bigjools> jtv: naturellement
<bigjools> not sure how I managed it since I targeted the MP quite deliberately :/
<gmb> bigjools: Well, send
<jtv> adeuring: then again, you're dealing with a very specific failure in snapshotting, not one with posts per se
<gmb> er
<gmb> bigjools: ... me the actual diff and I'll take a look in a little bit.
<jtv> adeuring: support for POSTs in general should be integration-tested, but I assume it is.
<bigjools> jtv: no you were right t he first time
<bigjools> gmb: http://pastebin.ubuntu.com/392395/
<adeuring> jtv: fine, but I don't get your point...
<gmb> Hah.
<gmb> bigjools: Yeah, I think you can land that :)
<jtv> adeuring: we know that POSTs normally succeed.  You're dealing with one very specific failure in that process.  I'm saying you could test for the fix to that specific failure, rather than test the full integration chain.
<adeuring> jtv: OK, so, what sort of test would you suggest?
<adeuring> just that these properties are not included in the snapshot?
<bigjools> gmb: ta :)
<adeuring> I think that's not a good idea
<jtv> adeuring: a check that the necessary lists are doNotSnapshot, with a comment saying why this is needed so that people don't mindlessly change the test when it breaks.
<adeuring> jtv: well, ok...
<jtv> adeuring: separately, it could be nice to have the snapshotting mechanism improved for this sort of situation: don't snapshot if the lists get too long, or something.
<adeuring> jtv: perhaps
<jtv> The shortlist is presumably there as a way of detecting that the mechanism doesn't scale up to the things it's being used for, and that this sort of change may be needed.
<jtv> adeuring: if/when the mechanism changes, you'd probably find that you lost your ability to reproduce the failure, and there'd be no test failure to indicate this.
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: *adeuring*, -  || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> Morning jtv :)
<adeuring> jtv: good point
<jtv> hi allenap!
<adeuring> jtv: triggering the failure in the test is easy.
<adeuring> jtv: ah, no...
<jtv> adeuring: it would also be nice AFAICS if you could test for a realistic limit, so e.g. adding 1K subscribers; then once the snapshots are fixed, you could perhaps remove the doNotSnapshot and still have a test that proved that you didn't break things.  But you don't need to repeat that for all those fields AFAICS
<jtv> (And if you're going to create persons in the factory, be sure to use makePersonNoCommit :)
<adeuring> jtv: hrmm... I thoughy you suggested to only test for the "doNotSnapshot" decoration?
<adeuring> s/thoughy/tought/
<jtv> adeuring: I'm trying to meet you halfway here. :)
<jtv> adeuring: actually, I'm just agreeing with you that it does make sense to try and show that the posts will now succeed.  But I'm also saying it makes sense to keep two parts of that separate: (1) doNotSnapshot fixes the problem, and (2) doNotSnapshot is applied in all the necessary places.
<jtv> That way the testing problem goes from (test for cause of problem) Ã (test places where problem can happen) to (test for cause of problem) + (test places where problem can happen)
<jtv> And if (test for cause of problem) involves a hard-limit of 1,000 objects....  :-)
<adeuring> jtv: OK. But I not sure if creating 1000 persons without a commit works here. Doesn't the logout() calls imply a commit?
<jtv> adeuring: I think it does, yes, but that's just onceâI'm saying avoid committing a thousand times inside the loop.  Also, don't do this in the setUp since only one test will be needing it.  If you're lucky you can get away with these persons being flushed to the db but never committed.
<adeuring> jtv: right
<jtv> factory.makePerson secretly commits
<jtv> ...after calling factory.makePersonNoCommit.
<jtv> The commits are usually only needed afaik if you're planning to log in as the new person.
<jtv> al-maisan: before I (or allenap) gets around to an actual review...  you're changing and adding some %s to big queries.  Maybe it's getting to the point where it's better use a dict there instead.  "%(x)s + %(y)s = %(z)s" % {'x': 1, 'y': 2, 'z': 3}
<al-maisan> jtv: good point .. can do that.
<jtv> Helps avoid annoying breakage while editing :)
<al-maisan> yup
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: (adeuring, al-maisan), -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: couldn't help myself, got drawn into muharem's branch :)
<jtv> al-maisan: s/\<but refer\>/but referring/g
<al-maisan> jtv, thanks, will correct that.
<jtv> al-maisan: but is it really useful to enumerate what you're about to test?
<allenap> jtv: That's okay :) I'm still getting my stuff together here.
<adeuring> jtv: I pushed a new version of my branch to LP
<jtv> al-maisan: I think it'd be more useful to explain for each of the test classes that you derive from the common-tests class what is specific to them.
<jtv> adeuring: kuhl, danks
<al-maisan> jtv: I see .. let me do that then :)
<jtv> al-maisan: also, I don't know how runScript goes about its job, but it may be worth checking that it's not forking off a subprocess each time.  Otherwise you spend an eternity re-reading ZCML, and recovering the db afterwards.
<al-maisan> jtv: I'll have a look.
<jtv> thanks
<jtv> al-maisan: I see you're running into complications loading the tests.  I think you can avoid those by not deriving ArchiveExpiryCommonTests from ArchiveExpiryTestBase; instead the specific test classes can multiply-inherit from both of those two.  That way the test loader doesn't come under the impression that ArchiveExpiryCommonTests is a test case.
<jtv> adeuring: oooh, so much tighter!
<al-maisan> jtv: will try that as well, thanks :)
<jtv> adeuring: in test_no_snapshots_for_large_collections, can you add a note saying why there should be no snapshots (i.e. that the lists can get large enough to break the snapshot)?
<jtv> I like the bug references in the test BTW.
<adeuring> jtv: I can do that, but I thinks the the doc sting of the test class already has this note
<jtv> adeuring: ah yes... that's cramming a lot of information into one sentence though.
<adeuring> yeah, I have some bad habits ;)
<jtv> (English is a relatively impatient language :)
<jtv> adeuring: an American once wondered out loud what his country would have looked like if German had become the official language (as it almost did).  I told him people would be more polite in conversation, since you won't know what somebody's really saying until they've finished the last verb at the very end of the sentence.  :)
<adeuring> jtv: could that be Mark Twain? He wrote some really hilarious comments about German
<jtv> adeuring: unfortunately I have never been in the position to make smart-alec remarks to Mark Twain.  The scathing comebacks could have provided some jolly good fun at my expense.
<jtv> This was an American who is alive today.  Or at least, was yesterday.
<jtv> adeuring: "salgado-change-anything"..?
<adeuring> jtv: ???
<jtv> adeuring: hey, it's from _your_ test :)
<adeuring> jtv: Ah, right... That's simple copy&pasted...
<adeuring> jtv:  some modern comments about German and Germany: http://www.amazon.de/die-was-Ein-Amerikaner-Sprachlabyrinth/dp/3499622505/ref=sr_1_1?ie=UTF8&s=books&qid=1268218456&sr=8-1 A really nice read
<jtv> adeuring: ah.  :)  Also in that test, could you turn the range(5) into range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1)?
<adeuring> jtv: sure
<jtv> Labyrinth is feminine in German?
<jtv> ah no, evidently not
<jtv> The URL suggests that, but the page title makes clear that it isn't.
<jtv> "Das Amerikan in das sprach labyrinth"
<adeuring> yes. Really odd stories. He noticed a sign "durchgehend geÃ¶ffnet" at a aupermarket and assumed he could buy stuff in the night...
<jtv> Frankly I wouldn't have known better...
<adeuring> jtv: sure,
<adeuring> that requires strange "context knowledge"
<adeuring> "durchgehend" means "not closed during lunch" ;)
<jtv> ah!
<jtv> adeuring: I think your branch is nearly there...  the setting and resetting of the snapshot limit is a bit ugly.  How about a "with"?
<jtv> Not particularly urgent, but practicing it now may pay off in the future.  :)
<adeuring> jtv: yes, I thought about that. But this fidding with a constant is done just here, so I did not see the point to use "iwth"
<jtv> adeuring: sure, no worries.  If you're updating the range(5), r=me
<adeuring> jtv: thanks!
<jtv> I'll go do the paperwork.
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: al-maisan, -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> jtv: BTW runScript() does not for off sub-processes..
<jtv> al-maisan: good to know, thanks for checking it out.
<al-maisan> jtv: the enhancements you suggested: http://pastebin.ubuntu.com/392443/
<jtv> al-maisan: looking...
<jtv> al-maisan: sorry, got sidetracked for a moment (figuring out why my job wasn't selected as a candidate).  Looking at your pastebin, too much indentation (or use of tabs) in param_names & param_values
<al-maisan> jtv: right, will fix the indentation.
<jtv> al-maisan: why not just create a dict directly?  Less fragile as the code continues to evolve
<al-maisan> jtv: fine.
<jtv> al-maisan: I'm not sure if sqlvalues will work on a dict, but it would be nice if it did.
<jtv> al-maisan: but apart from that, I approve of the branch.
<al-maisan> jtv: I don't think so .. that's why I used the dict(zip()) approach
<jtv> al-maisan: you may want to use quote() on a single value instead of sqlvalues() on a bunch
<al-maisan> jtv: hmmm .. that works for simple params .. in the code in question I have query params that are tuples ('archive_types')  .. would quote work for them as well?
<jtv> al-maisan: I think you need sqlvalues for those
<al-maisan> jtv: in which case I am stuck with dict(zip()) ..
<jtv> al-maisan: or just doing { 'key': quote(value), ...}
<jtv> (Which doesn't look all _that_ bad to me)
<al-maisan> jtv: IIRC there's also an sqlvalue() function .. let me check.
<jtv> al-maisan: sqlvalues will also work on one value though
<al-maisan> jtv: ok .. I can use it like that then.
<jtv> al-maisan: another question... where in the code was the candidate selection query again?
* jtv 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
<jtv> allenap: I'm bowing out in a minute
<al-maisan> jtv: let me look
<allenap> jtv: Okay, have a good day :)
<jtv> thanks, you too
<al-maisan> jtv: lib/lp/buildmaster/model/builder.py, line 475
<al-maisan> .. and the lines preceding it
<jtv> al-maisan: thanks... it's not selecting my arch-independent job because the Builder's virtualized seems to be set to False.
<al-maisan> jtv: actually sqlvalues() does support named parameters and returns a dict in that case.
<al-maisan> that's smart
<al-maisan> jtv: so, here we go: http://pastebin.ubuntu.com/392467/
<jtv> al-maisan: sheer beauty.  I've already approved the MP.
<al-maisan> jtv: thanks :)
<jtv> np
 * jtv rushes off
<adiroiban> danilos, henninge hi, can we schedule a preimplementation chat for bug 201749?
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/201749>
<danilos> adiroiban, sure, we can do it now if you want
<adiroiban> ok. now, if you submit an empty translation, it will add a new translation and link it to the msgset (for a language)
<adiroiban> is this correct?
<adiroiban> and I was thinking that in the translationmessage, insead of adding a reference to an empty string, it should change the "is_current" to false
<adiroiban> so that all msgstr linked with the current msgset are false
* salgado changed the topic of #launchpad-reviews to: on call: allenap || reviewing: -  || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> allenap, care to review https://code.launchpad.net/~salgado/launchpad/edge-redirect-bugs/+merge/21054 for me?
<allenap> salgado: Sure!
<adeuring> allenap: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-528569-api-bug-search-for-linked-branches/+merge/21058 ?
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: salgado || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Sure!
<adeuring> allenap: thanks!
<rockstar> allenap, can I jump on your queue?
<allenap> rockstar: Go for it :)
* rockstar changed the topic of #launchpad-reviews to: on call: allenap || reviewing: salgado || queue: [adeuring, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> allenap, https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025
<rockstar> allenap, can I add another branch to your queue?
<sinzui> EdwinGrubbs: https://code.edge.launchpad.net/~sinzui/launchpad/api-bug-tracker-0/+merge/21061
<danilos> adiroiban, hi
<adiroiban> ok
<adiroiban> I have also added a check in the browser layer
<adiroiban> to check for âforgedâ needs_review or diverge form fields
<danilos> adiroiban, so, what bug was that? (it seems you named the wrong one on #u-t
<adiroiban> bug 201749
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/201749>
<danilos> adiroiban, or maybe not, not sure why it said it was private on #u-t
<adiroiban> ... well.... a bug?
<allenap> rockstar: A combination of a mammoth pre-implementation call with gmb, a car in need of repair, two cinema tickets and some pesky kids means I almost certainly won't get to your review today. I am very sorry, but I am happy to do it first thing in the morning if that's of interest to you.
<rockstar> allenap, I can find someone else.  Thanks.
<danilos> adiroiban, yeah, it was ubotu there, and it's mup here, ubotu might be broken :)
<rockstar> allenap, I know how the EOD reviewer day goes.  :)
<gmb> rockstar: Since since it was my fault that allenap was away for a while, I'll take your review.
<danilos> adiroiban, first off, all comments should be full sentences that end with punctuation, so don't remove a period in one of those: and doing that when discussing things makes the diff larger and harder to read :)
<gmb> rockstar: https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025 ?
<rockstar> gmb, aye.  Thanks a lot.
<danilos> adiroiban, ok, so what you are trying to do will cause a lot of trouble
<danilos> adiroiban, it won't necessarily have the effect you want it to have
<danilos> adiroiban, for instance, if current translation is diverged, and there is a shared one, this will make the shared one current, and the diverged one might show as a suggestion only accidentally (i.e. if it has a date_created newer than the shared one)
<adiroiban> when I tick the âneeds reviewâ box, I do it because I think that the current translation is wrong
<gmb> rockstar: Looks good. r=me.
<rockstar> gmb, ta
<danilos> adiroiban, right, but there is also that 'needs review' side of things that might not really work as desired
<danilos> adiroiban, I agree unsetting it as current is an improvement over existing behaviour, but you can't do that as simply as unsetting a flag either; if it's diverged and there's an existing identical suggestion, one of those needs to be removed
<danilos> adiroiban, finding all that out is pretty expensive, so I think a proper solution would be to introduce a new column on translationmessage table called something like is_reviewed
<adiroiban> danilos: OK. Right now, I have no idea how diverged translations should be handled
<danilos> adiroiban, it's all very, very complex, and there are some intricate bugs as well; doing it this way would just introduce more intricate bugs
<danilos> adiroiban, in short, there's no simple solution to this
<rockstar> gmb, do you think you could do another quick one?  Mostly just mechanical changes for javascript adhere to the style guide.
<gmb> rockstar: Sure.
<danilos> adiroiban, the solution would probably be to create a 'unsetTranslationMessage' on POTMsgSet that takes a template, language, variant as parameters
<adiroiban> danilos: I'm trying to construct a test scenario for the use case described by you above
<danilos> adiroiban, and then we can call that when we figure out that's what a user wants, and it'd be isolated from the rest of the updateTranslation (which is a mess, and should not be extended in any way)
<danilos> adiroiban, look at lib/lp/translations/tests/test_potmsgset.py test_updateTranslation tests and how complex they are
<danilos> adiroiban, making them even more complex is out of the question :) separate method which encapsulates the concrete meaning of the action is the way to go if you want to tackle this
<adiroiban> ok
<adiroiban> danilos: so I will move this logic in a new method
<danilos> adiroiban, then you can worry about all the peculiarities of sharing and divergence for your specific use case in that particular method, because they'll be different
<adiroiban> danilos: unfortunately, I don't know what should I do for divergent and sharing messages
<danilos> adiroiban, right, and then you will have to come up with a good story for divergence and sharing; the biggest complication with all that is that diverged messages might be duplicated so once they are not used anymore, they should be aggregated
<adiroiban> but if those cases have tests
<adiroiban> I guess I can run the tests
<danilos> adiroiban, well, for one, once you unset is_current, you should unset TM.potemplate as well
<adiroiban> and improve the implementation
<danilos> adiroiban, yeah, I'd be happy to help explain as much as possible
<danilos> adiroiban, there are a lot of tests, and they are still not complete
<adiroiban> danilos: translationmessage.potemplate ?
<adiroiban> looking the in DB
<adiroiban> that field is not set for all translations
<danilos> adiroiban, yes, that indicates that that message is a divergence for a particular template (i.e. one coming from jaunty, where the shared version is used in karmic, lucid,...)
<danilos> adiroiban, if potemplate IS NULL, you are looking either at a shared current/imported message, or at a suggestion
<adiroiban> I see
<adiroiban> ok
<adiroiban> alse
<adiroiban> else
<adiroiban> is the ID of the specific potemplate
<danilos> adiroiban, ideally, that would have been pofile, and we'd lose (potemplate, language, variant) on TM, but since we did incremental development, it was impossible to re-use pofile
<danilos> adiroiban, that's right
<adiroiban> ok
<danilos> adiroiban, TM.pofile is basically an old field that's not up-to-date or correct anymore
<adiroiban> I will take care of the TM.potemplate
<adiroiban> ony other exception
<adiroiban> ?
<adiroiban> any other exceptions? :)
<danilos> adiroiban, well, a bunch of corner cases that will pop up, I am sure
<danilos> adiroiban, so, I'd have to ask you to get that branch past a Translations reviewer as well (nobody else would be intimate enough with the code), and we'll have to talk some more
<danilos> adiroiban, I'd also have to carefully think about all of the exceptions and problems
<danilos> adiroiban, basically, a TM is unreviewed suggestion if it has a date_created newer than the current translation; it means that the simplistic approach will just show all old (even those reviewed long ago) suggestions, which would be bad
<danilos> adiroiban, because unsetting a currrent translation will remove the date_reviewed info, and then all suggestions would end up being unreviewed
<danilos> adiroiban, so, that's another case to worry about; and when you introduce message sharing/divergence into it, it gets even "nicer" :)
<adiroiban> danilos: why  is bad if you see all suggetions ?
<danilos> adiroiban, because some have already been reviewed
<danilos> adiroiban, and rejected
<adiroiban> danilos: yes
<adiroiban> but some were rejected by mistake
<danilos> adiroiban, that's a different bug ;)
<adiroiban> or were rejected based on some translation rules
<adiroiban> that was now changed
<danilos> adiroiban, well, different problem
<danilos> adiroiban, we don't want them all to show up on a regular +translate page
<adiroiban> danilos: but we can not tell which suggestion was already reviewed
<danilos> adiroiban, yes we can
<danilos> adiroiban, as I said, there's currently a clear concept of what a reviewed message is
<danilos> adiroiban, anyway, I've got a call now, ttyl
<adiroiban> ok
<adiroiban> I will move the code in a separate logic
<adiroiban> and will consider the cases we discussed here
<adiroiban> and then we can have a new review
<gmb> rockstar: Sorry... is it https://code.edge.launchpad.net/~rockstar/launchpad/code-js-reorg/+merge/20170 you want me to look at?
<rockstar> gmb, yes
<gmb> rockstar: Cool. Looking now.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: adeuring || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> rockstar: Looks good. r=me.
* allenap 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
<rockstar> gmb, so, the first branch that you reviewed - abentley_ and I just talked about it, and he suggested using a decorator to do the try/catch instead of doing it inside the method.
<rockstar> (because what I was doing was crackful)
<danilos> adiroiban, I am about to call it a day, is there anything I can do to help out while I am still here? :)
<adiroiban> yes
<adiroiban> can you please explain
<adiroiban> in simple words
<adiroiban> what should âneeds reviewâ do
<adiroiban> from the point of view of users
<adiroiban> it should just set the translation to empty
<adiroiban> and mark the current translation as needs reviewn ?
<adiroiban> review?
<adiroiban> danilos: ^ :)
<adiroiban> or it should dismiss the current translation?
<adiroiban> and just leave the message untranslated
<danilos> adiroiban, well
<danilos> adiroiban, I think it should do that and leave all the other unreviewed suggestions as unreviewed suggestions
<danilos> adiroiban, the message should end up untranslated, but with the previous active translation as another suggestion added to the existing set of suggestions
<adiroiban> ok
<adiroiban> and then, listing ALL suggestions should be another bug and should be displayed only in the zoom in view
<danilos> adiroiban, btw, look at dismissAllSuggestions for a few ideas
<danilos> adiroiban, that's right, and there's already that "another" bug filed :)
<adiroiban> danilos: yes. I think I am assigned to that bug
<danilos> adiroiban, bug 339507
<mup> Bug #339507: translation page should show old translations <ui> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/339507>
<danilos> adiroiban, right
<adiroiban> but I was thinking we can force showing all suggestions
<danilos> adiroiban, and do take care of the TranslationConflict in a similar way (when someone changes translations after you have loaded the page)
<adiroiban> by ticking the needs review
<danilos> adiroiban, we shouldn't
<adiroiban> danilos: ok :)
<danilos> adiroiban, we'll get a very confusing behaviour of 'needs review'
<adiroiban> danilos: ok. Many thanks!
<danilos> adiroiban, now, you can perhaps say "it would make the implementation much easier, so I'd do that as a first step/branch", and then I'd agree :)
<salgado> allenap, did you see my reply to that review you did for me?
<danilos> adiroiban, you are welcome
<adiroiban> danilos: :)
<adiroiban> I will try to see how I will manage the implementation
<adiroiban> and if I needs to split this job
<danilos> adiroiban, sure, email whenever or we can talk some more tomorrow :) cheers
<adiroiban> by
<adiroiban> have a nice evening
<danilos> adiroiban, thanks, so do you
<rockstar> abentley, check out this (much better) diff.  We get the real exception now.  :)
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025
<leonardr> gary, do you hvae a minute to review https://code.edge.launchpad.net/~leonardr/lazr.restful/version-descriptions/+merge/21075 ? it shouldn't take long
<gary_poster> leonardr: yes
<rockstar> Where have all our OCRs been the last two days?
<gary_poster> leonardr: "[x for x in contents['service_doc']]" -> "list(contents['service_doc']]" ?
<gary_poster> leonardr: lines 189 and 191 of diff
<leonardr> gary: sure
<gary_poster> cool
<gary_poster> leonardr: do you intend to keep pdb around?  line 286
<leonardr> gary, no, that's in by mistake
<gary_poster> cool
<gary_poster> leonardr: otherwise, r=gary.  Will mark as such.
<leonardr> gary: also the mssing = object() needs to go
<gary_poster> ah, ok.  Should have investigated more carefully then.
 * kfogel is away: bbiab
<rockstar> mwhudson, could you do a small review for me, mostly mechanical changes?
<rockstar> (it's javascript)
<mwhudson> rockstar: i can try!
<rockstar> mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/code-js-file-reorg/+merge/21085
<rockstar> mwhudson, sorry it's so boring.  :(
<rockstar> mwhudson, as a matter of fact, the next branch is the branch that moves the javascript into lib/lp/code/javascript
<mwhudson> rockstar: :)
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/new-import-link-move/+merge/21088 for someone
<thumper> diff should be along shortly
<thumper> diff is up
<rockstar> thumper, looking now.
<thumper> rockstar: ta
<rockstar> thumper, r=rockstar
 * rockstar runs all the Windmill tests again.
#launchpad-reviews 2010-03-11
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> noodles775: consider yourself hassled! https://code.edge.launchpad.net/~al-maisan/launchpad/unembargo-oops-526645/+merge/21126
<noodles775> ha!
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, - || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775, al-maisan: I've got 3 small ones for you... interested?
<jtv> tiny: https://code.edge.launchpad.net/~jtv/launchpad/bug-535661/+merge/21045
<al-maisan> jtv: otp, should be available in a few minutes..
 * jtv is patient
<al-maisan> jtv: that's very much appreciated.
<jtv> :)
<noodles775> jtv: sure! Just pop them in the queue.
<jtv> pop or push, that is the question...
<jtv> on call: noodles775, al-maisan || reviewing: -, - || queue: [al-maisan, jtv3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> noodles775: I can start on jtv's stuff ..
 * jtv nods frantically :)
<noodles775> al-maisan: yep, I'll be starting on yours in a tick, just updating some bugs.
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, jtv || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: there were three for jtv right?
<jtv> right
<noodles775> jelmer was going to grab one too.
<al-maisan> great
 * al-maisan is looking at https://code.edge.launchpad.net/~jtv/launchpad/bug-535661/+merge/21045
<jelmer> I'll take https://code.edge.launchpad.net/~jtv/launchpad/bug-536769/+merge/21068 in that case
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: al-maisan, jtv || queue: [jtv*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> jelmer: dankjewel
<al-maisan> hmm .. the one I picked is a hard nut ;)
 * al-maisan moves on to https://code.edge.launchpad.net/~jtv/launchpad/cleanups-475435/+merge/21070
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: al-maisan, jtv2 || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan, jtv: do you guys know where the old mentat guidelines are (things to watch out for as a new reviewer).
 * al-maisan digs around for the guidelines
<al-maisan> https://dev.launchpad.net/PreMergeReviews
<jtv> noodles775: the old ones were on the old internal LP wiki IIRC
<noodles775> jtv: yeah, that's where I'm searching.
<noodles775> (without success)
<jtv> but someone or other went to some effort to reorganize a lot of that stuff on the new one
<al-maisan> https://dev.launchpad.net/PythonStyleGuide
<noodles775> al-maisan: yeah, the styleguide and review preparation notes are on the new wiki, but there used to be a page specifically for new reviewers (with lots of suggestions etc.)
<al-maisan> https://dev.launchpad.net/WorkingWithReviews
<jtv> https://launchpad.canonical.com/MentorProcess
<jtv> https://launchpad.canonical.com/TipsForReviewers
<noodles775> jtv: gold! THanks!
<jelmer> jtv: thanks
<al-maisan> jtv: looking at lp:~jtv/launchpad/cleanups-475435, line 23 of the diff: why the question mark at the end of the line?
<jtv> al-maisan: I was just looking for a way to express the meaning of the parameter... if you have a better idea, I'd be happy to change it.
<al-maisan> ah
<al-maisan> jtv: your cover letter does not specify any tests to run .. how did you test the change locally?
<jtv> al-maisan: tbh I don't remember...  I know I didn't run the windmill tests, but other than that, really don't know now
<jtv> I have a vague recollection of running the pagetests only
 * jtv tries running those now
<al-maisan> jtv: please paste the command here ..
<jtv> al-maisan: ./bin/test -vv -t 'translations.*stories'
<al-maisan> thanks!
<jtv> al-maisan: I think I broke something, though it may also be the buildfarm stuff I'm running on the side.  Want me to retract this one, have EC2 run a full test, and try again later?
<al-maisan> jtv: FWIW I am seeing test failures on my side as well
<al-maisan> with fresh trunk + your branch merged
 * al-maisan waits for the test run to finish
<jtv> maybe I just missed the test results between reboots or something...  I'll retract this one.
<al-maisan> jtv: fine.
<al-maisan> jtv:
<al-maisan>   Ran 863 tests with 7 failures and 0 errors in 4 minutes 34.474 seconds.
<al-maisan> Tests with failures:
<al-maisan>    lib/lp/translations/tests/../stories/standalone/xx-person-activity.txt
<al-maisan>    lib/lp/translations/tests/../stories/standalone/xx-pofile-details.txt
<al-maisan>    lib/lp/translations/tests/../stories/standalone/xx-series-templates.txt
<al-maisan>    lib/lp/translations/tests/../stories/standalone/xx-translationmessage-translate.txt
<al-maisan>    lib/lp/translations/tests/../stories/standalone/xx-translations-to-complete.txt
<al-maisan>    lib/lp/translations/tests/../stories/standalone/xx-translations-to-review.txt
<al-maisan>    lib/lp/translations/tests/../stories/importqueue/xx-translation-import-queue.txt
<jtv> al-maisan: I think the easiest is just to discard the branch...  it was really just leftovers from exploring a different problem.
<jtv> Sorry for putting you to the trouble.
<al-maisan> fair enough .. I'll stop looking at it then.
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: al-maisan, jtv2 || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * al-maisan moves on to https://code.edge.launchpad.net/~jtv/launchpad/bug-536769/+merge/21068
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: al-maisan, jtv3 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: I'm not sure if jelmer already started that one before he had to pop out?
<jtv> noodles775: I think he did, yes
<al-maisan> noodles775: it will need another review anyway
<al-maisan> since jelmer has not graduated as a reviewer yet
<al-maisan> right?
 * al-maisan can hold off
<noodles775> al-maisan: True, it will, but that should happen *after* his.... yeah, take an ocr break ;)
<al-maisan> ssure
<noodles775> al-maisan: btw, I'm seeing if your queries can be done simply with storm.
<al-maisan> noodles775: IMHO there is no advantage in doing so since the queries are not fetching full objects that can be cached
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: al-maisan, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: you can use result_set.values() to just get specific columns, but it's not for an advantage that I'm looking at it, but because (whether I like it or not) it's the ORM we're trying to use.
<noodles775> Assuming there is no disadvantage for the query of course.
<noodles775> al-maisan: sheesh, the required imports for doing the storm version are a pain in and of themselves! ;)
<al-maisan> hmm..
<stub> https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/21130 (trivial)
<noodles775> stub: r=me, I was wondering why we're not using the title attribute for the abbr tags though?
* jelmer changed the topic of #launchpad-reviews to:  on call: noodles775, al-maisan || reviewing: al-maisan, - || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> noodles775: No idea on title. Ta for the review though :)
<stub> noodles775: Should I add the titles to the abbreviations since I'm here?
<noodles775> stub: yeah, that'd be great... Thanks.
<stub> What is the title for CVS?
<noodles775> Concurrent Versioning System?
 * noodles775 checks
<stub> Concurrent Versions System
<stub> Oh - we don't use title cause it works crap in the UI, double mouse overs because we are in an <a> tag.
<stub> I can spell SVN Subversion easily enough
<noodles775> urg, there you go.
<noodles775> What's the mouse over for the <a> though? There's no title on it? Weird.
<stub> We underline on hover and the mouse pointer changes
<stub> But if you mouse over the abbr you get the ? icon and not the finger icon
<noodles775> I see, yeah, that sux.
<stub> I'll land it as is since I've already run the tests ;)
<noodles775> Great.
<adiroiban> danilos: hi. do you have time for talking about bug 201749?
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/201749>
<danilos> adiroiban, some, sure
<adiroiban> browser/translationmessage.py 419-420
<adiroiban> danilos: from here http://bazaar.launchpad.net/~adiroiban/launchpad/bug-201749/revision/10467
<adiroiban> I think they are needes, as if user is not official translator
<adiroiban> those values will not be set to false
<danilos> adiroiban, how come?
<danilos> adiroiban, they won't be present in the form, thus .get() will use the default value of False
<adiroiban> danilos: but maybe someone will forge the form
<adiroiban> and inject those values
<danilos> adiroiban, so?
<adiroiban> and so force_diverge can be set to True
<adiroiban> even if the user is not a reviewer
<danilos> adiroiban, privileges are still checked in updateTranslation if I am not mistaken
<danilos> adiroiban, so, it doesn't really matter
<adiroiban> danilos: ok. my bad. But I didn't find the place in updateTranslation
<danilos> adiroiban, (I may have mentioned how updateTranslation is a nasty behemoth; well, this is one part of it :)
<danilos> adiroiban, look for force_diverged and where just_a_suggestion is set; updateTranslation *is* complex
<adiroiban> force_diverges is used in _makeTranslationMessageCurrent
<adiroiban> and I can not see the security check there
<adiroiban> http://paste.ubuntu.com/393247/
<adiroiban> danilos: but we can talk about this later and I will use staging to test if this is a security problem
<danilos> adiroiban, that path is not entered unless just_a_suggestion is False
<danilos> adiroiban, you can spend time checking it, but I can assure you it's not ;)
<adiroiban> danilos: ok :)
<danilos> adiroiban, if you want, read the code to confirm it (_isTranslationMessageASuggestion is used to set just_a_suggestion)
<adiroiban> danilos: thanks :)
<adiroiban> danilos: I still don't know how diverged messages should be handled in this case http://paste.ubuntu.com/393253/
<jelmer> noodles775, al-maisan: can either of you have a look at my branches?
<adiroiban> I don't understand why the new translations are going to be diverged by default
<danilos> adiroiban, not always, only if the existing one is already diverged
<adiroiban> danilos: true. but why, or how are they going to be diverged?
<noodles775> jelmer: after lunch I can sure.
<danilos> adiroiban, we've got thousands of those already, we can't just close our eyes on it
<adiroiban> danilos: is this a valid usecase for this bug https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/14 ?
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/201749>
<danilos> adiroiban, sorry, on the phone
* al-maisan changed the topic of #launchpad-reviews to:  on call: noodles775, al-maisan || reviewing: al-maisan, jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> noodles775, jelmer: I am looking at https://code.edge.launchpad.net/~jelmer/launchpad/bug531985/+merge/21038
<noodles775> al-maisan: great, I'm off to lunch :)
<al-maisan> noodles775: did you rewrite my branch yet? ;)
<al-maisan> noodles775: ah, just saw the review email, thanks!
<al-maisan> jelmer: how was lp:~jelmer/launchpad/bug531985 tested?
* noodles775 changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: on call: al-maisan, rockstar || reviewing: jelmer,  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> [15:25] <al-maisan> jelmer: how was lp:~jelmer/launchpad/bug531985 tested?
<jelmer> al-maisan: sorry
<jelmer> al-maisan: I tested by adding myself to commercial-admins
<al-maisan> jelmer: on your local system?
<al-maisan> s/local/development/
<al-maisan> jelmer: r=me
<jelmer> al-maisan: dogfood only
<jelmer> al-maisan: sorry, too much conversations happening at once :_/
<al-maisan> jelmer: that's even better :-)
<jelmer> al-maisan: thanks
 * kfogel is away: switching consoles for a bit, back soon
<leonardr> gary, this isn't quite ready for review, but could i get your feedback on https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-apidoc ?
<leonardr> the incremental diff is http://pastebin.ubuntu.com/393449/
<leonardr> i'm specifically wondering if you have any suggestions on how to put the template file somewhere else while still keeping it accessible from utilities/ (i tried pkg_resources loading from canonical.launchpad.templates but it didn't work, probably because c.l.t isn't a module)
<leonardr> and if you have any ideas on how to get rid of versions_and_descriptions in the namespace--i'm not very good at tales
<leonardr> i'm going to lunch soon, so there's no rush
<gary_poster> leonardr: I'll look in just a few
<leonardr> gary, np
<salgado> adiroiban, did you get a failure email from ec2 about that branch I submitted for you?
<adiroiban> salgado: no.
<adiroiban> but I don't think ec2 email notification is working
<salgado> it is, I got one yesterday
<adiroiban> there were some other branches landed on my behalf
<adiroiban> and I did not get any notification for them
<salgado> ok, then it's possible this one failed because PQM was in testfix mode
 * salgado submits once again
<adiroiban> for the branches landed by Graham, he has forwarded those emails
<adiroiban> but if the branch was not landed
<adiroiban> it probably means it has errors
<salgado> gmb, when landing adiroiban's branches, did you use 'ec2 land'?
<henninge> rockstar: Hi!
<rockstar> henninge, hi there.
<henninge> rockstar: hey, will you be at UDS?
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> henninge, yes, I believe I will.
<henninge> rockstar: cool, you still got my bag?
<henninge> ;-)
<rockstar> henninge, I hope I still have your bag.  :)
 * henninge imagines rockstar scrambling round his room, looking for it ...
<rockstar> (we're in the process of remodeling the downstairs)
<henninge> rockstar: if you find it by May, bring it along, please.... ;)
<henninge> ... under the snow.
<rockstar> It's probably in the pile of stuff from when we emptied my office.
<rockstar> henninge, snow's almost melted actually...  first time since November.
<henninge> rockstar: ah, better than here.
<henninge> March came along and a new coat of snow with it.
<henninge> rockstar: anyway, I do have a branch ready for review, too.
<henninge> ;)
<henninge> but I am about to leave.
<henninge> rockstar: do you want to give it a go or shall I ask abel in the morning?
<rockstar> henninge, if it's complicated, it might have to wait.  If it's simple, I can probably take it.
<henninge> rockstar: complicated depends on if you have been to Wellington, I guess.
<rockstar> henninge, UNLESS you write me a novel description of the patch and why it's so complicated.
<henninge> build farm stuff
<rockstar> henninge, okay, I'll take a look.
<henninge> rockstar: it *does* have a long cover letter.
<henninge> rockstar: cheers
* henninge changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> rockstar: oops, I never pasted this: https://code.edge.launchpad.net/~henninge/launchpad/bug-509557-invoke-pottery/+merge/21172
<rockstar> henninge, I already found it.  :)
 * henninge makes a mental note to remember to press 'enter' after input
<henninge> rockstar: ok, I am outta here. If you get too confused, drop it ... ;-)
<rockstar> henninge, oh, I'll just make comments in the proposal for you to get to in the morning.
<henninge> rockstar: thanks and good night
* jelmer changed the topic of #launchpad-reviews to: rockstar || reviewing: || queue: [henninge,jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gary, it's now a formal review request: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-apidoc/+merge/21178
<gary_poster> ok
<leonardr> if you're too busy, maybe flacoste could look?
<gary_poster> leonardr: that's fine.  I will be on call until I finish talking to you
<leonardr> rockstar, i need to rev lazr.restful, can you review this trivial branch?
<leonardr> http://pastebin.ubuntu.com/393541/
<rockstar> leonardr, looking
<rockstar> leonardr, r=me
<leonardr> great
<henninge> rockstar: I am here, btw.
* NCommander changed the topic of #launchpad-reviews to: rockstar || reviewing: || queue: [henninge,jelmer,NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jelmer, NCommander, I generally don't notice changes to the topic unless the changes in topic are requested first.  :)
<jelmer> rockstar: oops, sorry :-)
<NCommander> rockstar: sorry, first review :-)
<rockstar> jelmer, NCommander, where are you merge proposals?
<jelmer> rockstar: https://code.edge.launchpad.net/~jelmer/launchpad/noversion/+merge/21059
<NCommander> rockstar: https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog/+merge/21179
<jelmer> (it's a lot of lines, but mostly removing obsolete stuff)
<rockstar> jelmer, NCommander, ack.  I'll get to them after henninge
<jelmer> rockstar: thanks!
 * kfogel is away: machine fan cleanup; back later
#launchpad-reviews 2010-03-12
<mwhudson> thumper: can you review a branch for me?
<mwhudson> it's about 1400 lines
<thumper> sure
<mwhudson> thumper: you should have mail shortly
<mwhudson> thumper: having fun yet?
<thumper> mwhudson: trying to keep my eyes open
<mwhudson> heh
<thumper> mwhudson: just finished going through it all
<mwhudson> it wasn't any more fun to write...
<thumper> I think that the code_import_helpers should move out of canonical.launchpad.testing
<mwhudson> _everything_ should move out of canonical.launchpad.testing
<mwhudson> so yeah
<thumper> well yeah
<mwhudson> thumper: i have another branch to review too, its much easier...
<thumper> ok
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/incremental-bzr-svn-imports/+merge/21209
<thumper> I haven't got to any coding this afternoon :-|
<thumper> mwhudson: both r=thumper
<mwhudson> thumper: yeah, sorrya bout that :-)
<mwhudson> i'm very glad to get that branch under way though
<thumper> it isn't (entirely) your fault
<mwhudson> thanks for the reviews
<thumper> :)
<thumper> I spent quite a bit of time talking to lifeless
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [henninge,jelmer,NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [henninge,jelmer,NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> henninge: where is the branch you want reviewed?
* henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [jelmer,NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adeuring: gone ;-)
<henninge> adeuring: I guess rockstar did a bad job at cleaning out the queue ...
<adeuring> henninge: no problem
<wgrant> https://code.launchpad.net/~wgrant/launchpad/get-bpr-by-filename/+merge/21211 is mine.
<adeuring> wgrant: ok, noted. But let me follow the FIFO principle, so I'll start with jelmer's branch ;)
<wgrant> adeuring: Right, I was just putting it there for future reference.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue: [NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue: [NCommander,wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: NCommander || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jelmer_: review sent
<jelmer_> adeuring: thanks!
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adiroiban, you didn't get any email about that bug-531261 branch, did you?
<adeuring> wgrant: r=me, one suggetsion
<wgrant> adeuring: I don't think there are any open at the moment.
<adeuring> wgrant: OK, the I'll start the ec2 test
<wgrant> adeuring: Thanks.
<adeuring> wgrant: welcome :) And thanks for all your hard work!
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> wgrant, do you have another one in the queue?
<wgrant> salgado: https://code.edge.launchpad.net/~wgrant/launchpad/anonymous-irc-nicks-and-wiki-names/+merge/21219
<wgrant> Nice tiny one.
<adeuring> salgado: can you review it? I need a lunch break ;)
<salgado> adeuring, sure
<wgrant> Thanks salgado.
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,wgrant || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adeuring, but I'll have one waiting for you to get back from lunch. :)
<adeuring> salgado: ok,
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> salgado: Thanks. Will you land it?
<salgado> wgrant, I'll try, but ec2test is not behaving these days...
<wgrant> salgado: I guess we'll see.
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: salgado,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: I've got some buildfarm fixes, some of them generic (based gratefully on discussion with wgrant) that you might like to review!  https://code.launchpad.net/~jtv/launchpad/bug-537866/+merge/21238
<allenap> BjornT: Do you think you'll have some time to revisit my isolate-tests branch. I think it's in pretty good shape now.
<noodles775> jtv: I'm certainly keen to look through it, but probably over the weekend. It might be best to get the OCRs to review it, and I'll take a look later, if that's ok.
<jtv> noodles775: sure, I'll try that
<noodles775> jtv: btw, there's a tiny conflict there too.
<jtv> adeuring, salgado: either you free for a buildfarm branch?  https://code.launchpad.net/~jtv/launchpad/bug-537866/+merge/21238
<jtv> grr
<jtv> thanks for spotting that
<adeuring> jtv: sure
<jtv> adeuring: michael just pointed out a small conflict in a block of imports; take it as read that those will be fixed.
<salgado> adeuring, I can take jtv's so that you can take mine; that ok with you?
<adeuring> salgado: sure :)
<adeuring> BTW, I'm already reading you changes
<jtv> cool
<BjornT> allenap: yes, i think i'll have time today
<allenap> BjornT: Thanks. Do you also have 30s to approve a db query in LPS please? I need to run it ASAP.
* salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: salgado,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> salgado: r=me
<salgado> thanks adeuring
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> allenap: why is the query needed? won't checkwatches choose a subset of bug watches to update anyway?
<allenap> BjornT: It will, but there are 15k watches, there have been problems with batching before, and I'm not 100% sure they're all solved (I think they are, but every time I've thought that something else happens). I just want to be safer than sorry.
<BjornT> allenap: you probably shouldn't update the ones that are NULL, though, should you?
<allenap> BjornT: There are probably a lot of those because this tracker has been disabled for weeks, but I guess not. I'll update the query.
<NCommander> allenap: thanks for the review this morning
<allenap> NCommander: Did you mean someone else?
<NCommander> allenap: er, whopos, autocomplete got the wrong person :-/
 * NCommander goes to find his coffee pop
<allenap> :)
<NCommander> *pot
<NCommander> wow
<NCommander> adeuring: thanks for the review this morning
<adeuring> NCommander: welcome :)
<adeuring> NCommander: plase ping me when/if BjornT and stub approved the branch, then I'll land it
<NCommander> adeuring: Thank you, I was wondering if you could help me understand something first though: I'm not very knowledgable about LIbrarian ATM, how much of a hit is it to query librarian for a file then parse its data directly in a UI method?
<NCommander> abentley: the other thing that was discussed was the possibility of moving the changelogs to a new table then enforcing a forgein key restriction (and then possibly moving copyright files at some point in the future)
<adeuring> NCommander: I have no real clue. But IIRC the Soyuz folks did something like this
<adeuring> NCommander: yes, having another table would address most of BjornT's  concerns, sounds like a good idea
<abentley> NCommander, you mean me?
<adeuring> abentley: I think he meant me
 * NCommander isn't so sure anymore
<NCommander> I'm welcome to accept all opinions :-)
<adeuring> NCommander: Can the stuff the needs to be displayed from changlog change without a change in the changelog data?
<adeuring> garrr... that's unreadable... let me try to avoid the repetiton of "change"....
<adeuring> NCommander: can it happen that different things from the changelog must be displayed without any modification in the changelog data? If not, the parsed data could be stored instead in the DB
<NCommander> adeuring: that's what we currently do (sorta, we pull changelog exerts from the .changes file).
<adeuring> ok...
<adiroiban> salgado: sorry for the delay. No, no email. Most probably some tests are failing, but unfortunalty I can not run the full LP test suite on my computer
<salgado> adiroiban, yeah, I'm just running the full test suite now.  once it finishes I'll check if anything failed and let you know
<adiroiban> salgado: thanks!
<adiroiban> danilos: can you please take a quick look at this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250
<adiroiban> it is not ready yet
<adiroiban> but I would like to know your comments for the potmsgset.resetCurrentTranslation()
<danilos> adiroiban, sure, I'll take a look in a bit
<NCommander> BjornT: would using a separate table for changelog information help with your concerns on increased memory usage?
<jtv> salgado-lunch: yup, "make lint" catches that bad character that got in...  it must have happened when I thought I was done and closed the window.  :-(
<jtv> huh wait, there's more lint now... wtf?
<danilos> adiroiban, hey, looking at your branch now
<danilos> adiroiban, updateTranslations is off limits: no changes allowed there ;)
<danilos> adiroiban, and date_created should probably not be modified
<adiroiban> danilos: a I agree with date_created
<adiroiban> but I don't know how to modify a potmsgset to be listed again as needs review
<danilos> adiroiban, logic to determine which method to call should be in the browser code
<danilos> adiroiban, potmsgset shouldn't be modified at all
<danilos> adiroiban, it's a TM you are modifying, and https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/13 gives you a suggestion
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/201749>
<adiroiban> danilos: ok I will move the logic to browser code
<danilos> adiroiban, and now, on to the resetCurrentTranslation method :)
<danilos> adiroiban, the way you do it won't do: we don't have the luxury of fetching a bunch of messages for every single message on +translate page
<danilos> adiroiban, you need to get only the matching message if it exists
<adiroiban> ok. but the logic is ok ?
<danilos> adiroiban, and the thing you are doing there is not what you should be doing
<danilos> adiroiban, no, sorry
<adiroiban> ok
<adiroiban> I still don't know how diverged messages shoudl be handled
<danilos> adiroiban, basically, your code does this: "current, diverged translation 'blah' needs review, let's find all other diverged translations 'blah' and make them 'shared'
<adiroiban> and what it should do?
<danilos> adiroiban, well, the only thing code should do is find an identical (to current) suggestion (meaning .is_current=False, .potemplate=None) if it exists, and then only discard the older one
<danilos> adiroiban, let me prepare an example
<danilos> adiroiban, http://paste.ubuntu.com/394099/
<danilos> adiroiban, fwiw, removing a message is impossible from web UI anyway, so you really can't fix this bug properly
<danilos> adiroiban, another example to be wary of: http://paste.ubuntu.com/394100/
<adiroiban> how we should proceed with the removal of a message?
<danilos> adiroiban, well, the best way would be to somehow tag it an then remove it later
<adiroiban> danilos: and when tag it... we should tag it in a wait it will not be selected by any other existing query
<danilos> adiroiban, what do you mean?
<danilos> adiroiban, perhaps we'll have to allow removal from the web UI as well, though that sounds very, very bad
<danilos> adiroiban, there's also the transfer of all the flags, like is_imported, from the deleted message to the preserved one
<adiroiban> danilos: i guess there are many SELECT queries that will need to be modified to take into acount the tag
<danilos> adiroiban, yeah, it'd end up being a pretty big change, with many performance implications
<danilos> adiroiban, as such, I wouldn't tackle this before many other things are cleaned up first
<adiroiban> danilos: but this is a big issues if someone is using Launchpad translations for reviewing or doing QA related tasks
<danilos> adiroiban, I am not disagreeing, I am just saying that it'll take a lot of work to get this done properly; or, we can ignore some of the issues and live with some problems we introduce, but we've got to be aware of them first
<danilos> adiroiban, for example, perhaps it's not a big deal if we've got duplicate identical suggestions in the DB, so let's not block on that
<adiroiban> danilos: for the duplicate suggestions
<danilos> adiroiban, we'll get some weird behaviour occasionally, but let's say they are of lower priority
<adiroiban> maybe we can have a cron job that will do the cleaning
<danilos> adiroiban, now, there's still some things to worry about
<danilos> adiroiban, maybe, but that'll be very complex as well
<adiroiban> danilos: what are are the other issues?
<danilos> adiroiban, and it won't stop any weirdness
<danilos> adiroiban, the other issue is that we need to carefully consider what will happen when somebody asks a diverged translation to be reviewed, and there is an existing shared one
<danilos> adiroiban, i.e. http://paste.ubuntu.com/394130/
<danilos> adiroiban, also, note that you can't unset potemplate if is_imported is true either
<adiroiban> danilos: for lates example, I think result 1 is fine
<adiroiban> and then another reviewer can decide if the shared suggestion is better
<adiroiban> or it will need a new diverged translation
<danilos> adiroiban, right
<danilos> adiroiban, ok, so, then it won't be that complicated
<adiroiban> danilos: what are the consequences of setting potemplate = None for a msgset with is_imported = True ?
<danilos> adiroiban, basically, we'll have to watch if current message is diverged or not; if it is, and it isn't is_imported, then you unset is_current and .potemplate and how you don't hit a DB constraint :)
<danilos> adiroiban, it won't be diverged is_imported message, and if there's another shared is_imported message, you'd be in for a ride
<danilos> adiroiban, basically, many places where only one is expected would assert/traceback
<danilos> adiroiban, provided you don't hit a DB constraint first (I am not sure if we have one like that)
<adiroiban> danilos: I see
<adiroiban> danilos: ok. thanks. one more question regarding potmsgset.updateTranslation()
<danilos> adiroiban, sure (though, you still can't touch it: nobody can, functionality needs to be slowly factored out of it :)
<adiroiban> should I add a new bug for the case when you tick âneedsâ for a already reviewd translation?
<adiroiban> or we already have a bug for that?
<danilos> adiroiban, what do you mean?
<adiroiban> if you add a new suggestion
<danilos> adiroiban, ok?
<adiroiban> and you tick âneeds reviewâ
<adiroiban> it will be listed as a new suggestion
<danilos> adiroiban, right, that's how it should work
<adiroiban> but if in fact it is not a new suggesion
<jtv> salgado-lunch: I've fixed up the weirdness in my branch.  I'm going to reboot now, hoping that that'll fix it.
<adiroiban> and _findTranslationMessage
<adiroiban> finds that this new suggestion
<danilos> adiroiban, ah, right, it's an already discarded suggestion
<adiroiban> is in fact an already existing suggestion
<adiroiban> it will not mark it as needs review
<adiroiban> this is why I touched the updateTranslation code
<danilos> adiroiban, I am pretty sure we have a bug about that as well
<adiroiban> ok
<danilos> adiroiban, if you ever want to fix that, you'll have to provide a new method addASuggestion() or something, similar to what you did for this bug, and move the functionality out of updateTranslation method and add new functionality you want :)
<danilos> adiroiban, and again, moved the logic to decide what to do out into browser code
<adiroiban> danilos: yes. I will move that logic.
<danilos> s/moved/move
<adiroiban> that was the initial implimentation
<adiroiban> but then I didn't know where to put the tests
<adiroiban> for that code
<danilos> adiroiban, right, I am talking about the other bug where discared suggestions don't show up as suggestions afterwards
<adiroiban> danilos: ah. ok... do you have any suggestion for where should I put the integration tests for resetCurrentTranslation ?
<adiroiban> lib/lp/translations/browser/tests/translationmessage-views.txt ?
<adiroiban> I am very puzzled by the way unit tests and integration tests are structured
<danilos> adiroiban, integration test? that would probably be very tricky, I'd look for existing needs review tests
<danilos> adiroiban, good unit tests are most important here; single integration test is good enough
<adiroiban> danilos: lib/lp/translations/tests/potmsgset-update-translation.txt
<danilos> adiroiban, the problem is that all these translationmessage and pofile views are a mess, just like updateTranslation is: there is no clear separation of anything
<adiroiban> danilos: ok
<adiroiban> thanks!
<danilos> adiroiban, that'd be for documentation on updateTranslation method, do it in potmsgset.txt with a single example for how to use resetCurrentTranslation (and please make it accept identical parameters as the rest of the methods: i.e. don't use pofile if other methods are passing potemplate, language and variant)
<adiroiban> I will try to fix the current problems and come back with more questions
<danilos> adiroiban, sure, I am likely to be out, but I can probably find some time over the weekend (email me :)
<adiroiban> danilos: no emails for the weekend... and I don't know if I will have time over the weekend
<danilos> adiroiban, heh, ok :)
<danilos> adiroiban, anyway, to be honest, this should probably be a new checkbox closer to the current translation; hopefully redesign of +translate views will fix all this
<adiroiban> danilos: I agree, as this is not âneeds reviewâ , but reset reviewed state
<adiroiban> danilos: did you have time to look over the POTemplate API ?
<salgado> adiroiban, did you get a failure email this time?
<adiroiban> salgado: yes. thanks
<adiroiban> salgado: are the same windmill tests that on my computer are also failing on mainline
<salgado> adiroiban, are you sure your mainline branch is pristine?
<salgado> because these tests don't fail in other branches I've submitted to ec2
<salgado> e.g. the one I submitted for wgrant which is on PQM right now
<adiroiban> salgado: yes , the mainline is pristine... but my computer is slow and all waits.forElemets are exceded
<adiroiban> but most probably, my changes are introducing more delays
<adiroiban> and this is why those tests are failing on ec2
<salgado> could be
<adiroiban> and they are not failing all the time
<adiroiban> on my computer, there are 50% chances of a windmill test to pass
<adiroiban> and this was since the beginning of my hacking on launchpad
<danilos> adiroiban, sorry, I don't remember seeing anything about the potemplate API, where was that?
<adiroiban> danilos: hm... I have added them as comment to the bug
<adiroiban> and I remember sending an notification email to you, henning and jtv
<adiroiban> the subject was POTemplate Details API
<adiroiban> https://bugs.edge.launchpad.net/rosetta/+bug/525371
<mup> Bug #525371: API for reading POTemplates details <api> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/525371>
<rockstar> salgado, care to look at a branch really quick?
<salgado> rockstar, sure, where's it?
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/recipe-canonical_url/+merge/21271
<rockstar> It's really little.
<salgado> rockstar, I wonder if isn't there a better place for that test...
<salgado> also, you don't really need to add that empty file, do you?
<salgado> not really empty, but with no actual code
<rockstar> salgado, I do because abentley and I both need it very soon, and we wanted to avoid conflicts.
<salgado> oh, ok.  can you s/2009/2010 there, then?
<rockstar> Huh, I thought I did...
<salgado> rockstar, how about moving the test to ./lib/lp/code/tests/test_sourcepackagerecipe.py ?
<rockstar> salgado, according to https://dev.launchpad.net/Web/URLTraversal that's where the test should go.  I originally had a unit test there.
<salgado> oh, didn't know about that
<salgado> r=me
<rockstar> salgado, cheers!
<jtv> salgado: I fixed those weird problems with my branch, but now I found an unintended consequence: too much is working now, meaning that a doctest is actually trying to talk to build-farm slaves, which of course we can't do in tests.  I'm fixing that up now.
<salgado> jtv, ok, I've just sent you a second round of feedback
<jtv> thanks
<salgado> jtv, will you let me know once you're done with that branch?
<jtv> salgado: yes
<jtv> salgado: over to you.  It's deep night here, so I won't be around for much longer
<jtv> whoops, maybe I should push those changes first :)
<salgado> jtv, that's a good idea. :)
<jtv> salgado: it's taking a while for the diff to update...
<jtv> salgado: updated now.
<salgado> cool
* 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
<jtv> salgado: thanks!
<wgrant> Anybody around for a review of https://code.edge.launchpad.net/~wgrant/launchpad/bprdc/+merge/21213?
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
