#launchpad-reviews 2009-12-14
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/fix-windmill-reply-test/+merge/16106 one line fix for the windmill test
 * mwhudson waits for the diff
<thumper> it's there
* adiroiban changed the topic of #launchpad-reviews to: on call:  || reviewing: - || queue [adiroiban(bug-435165),adiroiban(bug-496352)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<StevenK> Awww, no on call reviewer. Damn this Australian timezone!
<mwhudson> StevenK: i can probably do it for you if it's simple
<StevenK> mwhudson: It's a patch change for a cronscript that is run by the publisher
<mwhudson> StevenK: let's see it
<mwhudson> StevenK: i may run away screaming, but i'll take a loot
<mwhudson> look
<StevenK> mwhudson: I'll be running it past cjwatson when he wakes, but a second pair of eyes is what I wanted
<StevenK> mwhudson: http://paste.ubuntu.com/340938/ ; in leiu of pushing it as a branch yet
<mwhudson> StevenK: looks fine to me, but hardly my area of expertise
<StevenK> mwhudson: I'll throw it past Colin, and push it as a branch if he says go, if that sounds fine
<mwhudson> StevenK: i don't see why you couldn't push it as a branch now
<StevenK> Point.
 * StevenK pushes
<mwhudson> and propose a merge
<mwhudson> StevenK: once colin has said ok, you should be able to find an eu-timezoned dev to approve it easily enough
<mwhudson> StevenK: is this urgent?
<StevenK> mwhudson: It's a germinate change, so the sooner the better
<mwhudson> StevenK: because launchpad is being released on thursday, we're in pre-release freeze
<mwhudson> danilo[home] is the man to convince to get it past the freeze
<StevenK> mwhudson: My commit message won't appear in PQM's message, since I've noticed in bzr log they have "special" formatting
<StevenK> Sigh, that's supposed to be a question
<mwhudson> StevenK: that's ok, we have magic to add the "specialness" now
<StevenK> mwhudson: The Reviewer should be 'launchpad' ?
<mwhudson> StevenK: the default should be fine
<StevenK> mwhudson: In that case, I've pushed the branch, and proposed it for merging
<mwhudson> StevenK: cool
<StevenK> Except that Launchpad just OOPS'd on it
<mwhudson> StevenK: you can request a review from colin too
<mwhudson> StevenK: :(
<mwhudson> StevenK: what was the oops?
<StevenK> OOPS-1444EA779
<mwhudson> oh except the log syncing is disabled
<StevenK> mwhudson: Shall I try it again, or you're looking/added it to a list to be looked at?
<mwhudson> StevenK: definitely worth trying again
<mwhudson> StevenK: it was a timeout, which is a bit odd, but try again indeed
<StevenK> And it worked \o/
<mwhudson> yay
<jml> hi
* jpds changed the topic of #launchpad-reviews to: on call:  || reviewing: - || queue [adiroiban(bug-435165),adiroiban(bug-496352,jpds(bug-176396))] || 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: - || queue [adiroiban(bug-435165),adiroiban(bug-496352,jpds(bug-176396))] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> henninge: i haven't added my name because i'm chr and also catching up with stuff after a week and a bit of absence. but if things get busy by all means ask me to help out
<henninge> intellectronica: that's fine. Enjoy your CHR week! ;)
<intellectronica> :D
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: adiroiban(bug-435165) || queue [adiroiban(bug-496352,jpds(bug-176396))] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * henninge lunches first, though.
<al-maisan> henninge-lunch: I'd appreciate it if you could review this simple branch as well: https://code.edge.launchpad.net/~al-maisan/launchpad/disable-ppa-495975/+merge/16125
* al-maisan changed the topic of #launchpad-reviews to: on call: henninge || reviewing: adiroiban(bug-435165) || queue [adiroiban(bug-496352,jpds(bug-176396)),al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> henninge-lunch: .. and BTW, we plan to get an RC for the branch and it's tiny .. so, please give it preferential treatment if possible. Thanks!
<danilos> StevenK, bigjools: I assume we will want an RC for https://code.edge.launchpad.net/~stevenk/launchpad/netbook-cron/+merge/16115? bigjools, if so, can you please review it first?
<bigjools> danilos: ok
<bigjools> danilos: approved
<danilos> bigjools, cool, who will be landing this if I RC it? StevenK, can you land it or someone from the LP team needs to do it?
<bigjools> danilos: it will need to be one of us
<bigjools> danilos: maybe you :)
<bigjools> there are no tests, just pqm-submit it
<danilos> bigjools, I have so many things on my hands right now that I am afraid I can't do it :)
<bigjools> join the club :/
<danilos> bigjools, I am IN the club already!
<bigjools> danilos: ok I'll do it, mark it r-c though will you?
<danilos> bigjools, sure
<danilos> bigjools, done
<bigjools> cheers
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [adiroiban(bug-496352,jpds(bug-176396)),al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> abentley: Fancy a quick OOPS fix to review? https://code.edge.launchpad.net/~allenap/launchpad/bugtracker-snapshot-bug-447100/+merge/16130
<abentley> allenap: I can add it to the queue.
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [adiroiban(bug-496352,jpds(bug-176396)),al-maisan, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> abentley: Thanks.
<abentley> adiroiban: I notice you deleted "lp.soyuz.interfaces.archive import ArchivePurpose" from lib/lp/archiveuploader/uploadpolicy.py.  Was this an unneeded import?
<adiroiban> abentley: yes. lint was complaining about it
<abentley> Cool.  Thanks for fixing lint errors as you go.
* al-maisan changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [adiroiban(bug-496352,jpds(bug-176396)), allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> I wanted to fix the other errors ... withoud adding ignore comments... but I don't know how to fix them
<abentley> adiroiban: I don't know how to fix the F0401 for lazr.* without ignore comments.  For the email ones, you could try using the lowercase module names, e.g. "email.encoders"
<adiroiban> ok
<abentley> adiroiban: For email.MIMEText, it looks like the new name is email.mime.text.
<adiroiban> abentley: thanks. I'll fix that
<abentley> adiroiban: approved.  We won't be able to land it until next week because this is a release week.
<adiroiban> abentley: no hurry. Thanks!
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [jpds(bug-176396)), allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> adiroiban: Could you remind me next week, please.  And maybe run the full test suite in advance?
<adiroiban> abentley: yes
<abentley> adiroiban: Great, thanks.
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), jpds(bug-176396)) || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> jpds: Have you run the distributionmirror tests with your change?
<jpds> abentley: Yes.
<jpds> abentley: And I've tested it on a local Launchpad instance.
<abentley> jpds: Normally, I would expect that change to break a test.  Since it didn't, it means this behaviour isn't being tested.  You could add a test for it, or I can if you prefer.
<jpds> abentley: http://pastebin.ubuntu.com/341226/ ?
<abentley> jpds: Exactly.
<jpds> abentley: OK, let me test it locally and then I'll push.
<abentley> jpds: Sounds good.
<jpds> abentley: How strange: http://pastebin.ubuntu.com/341229/
<jpds> Ah, wait, there's already a test there...
<jpds> abentley: My mistakes, pushed new revision with test.
<henninge> adiroiban: do you have a good example on launchpad.dev for the template list?
<adiroiban> henninge: yes. I'm done :)
<henninge> adiroiban: sorry, I am talking about bug 435165
<mup> Bug #435165: Make it easier to navigate to the full list of templates in source packages <trivial> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/435165>
<adiroiban> i'm just toggling the unseen class
<henninge> what I am just reviewing
<adiroiban> ah
<abentley> jpds: Thanks.  Approved.  Since this is a release week, it can't land until we reopen for development.
<henninge> ETOOMUCHCONCURRENCY ;)
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge: I have created some templates in the stories
<adiroiban> also I set the SHOW_RELATED to 1
<henninge> adiroiban: also, you have merge conflicts in the diff ?!?
<adiroiban> and everhing was ok
<jpds> abentley: Thanks, when do we reopen?
<henninge> jpds: Friday night. Late, usually
<adiroiban> henninge: uhh... I'll do
<henninge> adiroiban: xx-potemplate-index.txt is missing when I get your branch. That's where the conflict is.
<henninge> adiroiban: yes, you'll have to fix that first.
<adiroiban> henninge: xx-rosetta-potemplate-index.txt was renamed to xx-potemplate-index.tx
<henninge> adiroiban: yes ...?
<adiroiban> yes.
<adiroiban> in another branch that was recently merged
<adiroiban> i did that
<henninge> adiroiban: I mean, I never mentioned xx-rosetta-potemplate-index.txt
<henninge> adiroiban: something is wrong with your branch then. Have a look at the diff on the MP.
<adiroiban> henninge: anyway, let me fix the branch and then you can review it
<adiroiban> :)
<abentley> allenap: I may have been offline when I posted, so: "Could you explain the import reordering to me?  it's just that it seems to go against PEP8's recommendation of '1. standard library imports, 2. related third party imports, 3. local application/library specific imports'"
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge: I have merged with devel and fixed that conflict
<adiroiban> sorry for the inconveniences
<allenap> abentley: One of us was offline I think :) I think I should put lazr.lifecycle above the lp and canonical imports. The lazr.lifecycle import is new.
<abentley> allenap: I agree.  Approved, with that change.
<allenap> abentley: Thanks :)
* noodles775 changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi henninge, abentley: if either of you has time: https://code.edge.launchpad.net/~michael.nelson/launchpad/475808-unembargoing-oops/+merge/16141
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), noodles775 || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> noodles775: I suspect your F0401 lint errors can be fixed by changing the imports to email.mime.multipart and email.mime.text.  Can you check that?
<noodles775> abentley: will do, thanks.
<abentley> noodles775: On line 163 of the patch, you want "whose", not "who's".
<noodles775> yep, btw, that fixed the lint, thanks!
<abentley> noodles775: Cool.
<abentley> noodles775: Also, you want "its", not "it's" on line 172.
<noodles775> gah.
<abentley> noodles775: On the bright side, the code looks pretty good.
<abentley> noodles775: So for the case where there's no ancestor in the target of the copy, is that a valid case?  Or should we require the first upload to be done without copying?
<noodles775> abentley: it's valid. Anyone can upload a pkg to their PPA (and we'll just override the component to 'main')
<noodles775> It's just when copying that package to a non-ppa archive, we need to ensure the component will be overridden correctly.
<abentley> noodles775: Okay, can you explain why test_do_delayed_copy_wrong_component_no_ancestry should raise an exception?
<noodles775> Well, I didn't change that behavior - any copy for an invalid component raises an exception currently. I'm just otp, but after the call I'll try to give a better answer.
<adiroiban> abentley: can I have a quick pre-implementation review for this bug https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375
<abentley> adiroiban: looking
<abentley> adiroiban: I think it would be better to discuss this with someone from translations.  They have a clearer idea of the kind of changes they want in that area.
<adiroiban> lib/lp/translations/browser/distroseries.py - I don't know if this is the right way to augment the view
<adiroiban> abentley: thanks. no problem.
* adiroiban changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), noodles775 || queue [adiroiban(bug-492375 pre-implementation)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> abentley: ok, back (sorry).
<noodles775> So the behavior of raising an exception isn't something that has changed with this branch, I've just added a test for it (together with the fix for bug 475808).
<mup> Bug #475808: Unembargoing packages via the API doesn't seem to apply overrides correctly <ppa> <soyuz-security> <soyuz-upload> <Soyuz:Triaged by michael.nelson> <https://launchpad.net/bugs/475808>
<abentley> noodles775: Ah, I see.
<abentley> noodles775: So the override to main doesn't happen because the target isn't a PPA?
<noodles775> abentley: yes, for ppas we always override to main, but not for other archives where we do support different components. We do a different override in that case (in SourcePackagePublishingHistory.overrideFromAncestry()), but that can only be done *when* we've created the SPPH.
<noodles775> For delayed copies, this overrideFromAncestry() happens when the delayed copy is realised (see lp.soyuz.model.queue.PackageUpload.realiseUpload()) 
<abentley> noodles775: r=me
<noodles775> Thanks abentley 
* abentley changed the topic of #launchpad-reviews to: on call: henninge, - || reviewing: adiroiban(bug-435165), noodles775 || queue [adiroiban(bug-492375 pre-implementation)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> henninge, I have a RC one; can you take it?
<henninge> salgado: I was about to leave. How big is it?
<salgado> henninge, https://code.edge.launchpad.net/~salgado/launchpad/bug-495544/+merge/16120
<salgado> really small
<salgado> henninge, but it will be easy for me to find somebody else to review, if you'd prefer
<henninge> salgado: no, that's ok
<henninge> salgado: do you know about assertContentEqual?
<salgado> I once knew
<henninge> ;-)
<henninge> it does the sorted() thing for you when comparing lists.
<salgado> yeah, I've even used it in the past, but this time I started with assertEquals as I had a test that was expecting an empty list.  and my test changed I forgot to use assertContentEqual
<salgado> but that's changed already
<henninge> salgado: also, although I know it is not required, I like the (expected, observed) order better. If you could do that it would be nice.
<salgado> sure thing
<henninge> salgado: one last nitpick: TestZopelessTransactionManager_reset_store does not look right.
<salgado> what would look right?
<henninge> salgado: with the underscores in the name and having a Testcase to test just one method.
<henninge> (that's what's not looking right)
<henninge> salgado: Isn't it one test case per class and test_methodName_special_case in that class as a naming convention?
<henninge> salgado: also, it looks like the other test case in that file is the special case as it has no layer
<salgado> I don't know about the one test case per class policy
<salgado> tests with layers should be the exception, ideally
<henninge> so how about calling the first one TestZopelessTransactionManagerNoLayer and yours just TestZopelessTransactionManager?
<salgado> sure, that wfm
<henninge> cool, r=me
<salgado> henninge, thanks!
<henninge> adiroiban: I am sorry, I missed your ping earlier and saw it just now. Since your branch is not release-critical, I hope it's OK I finish the review later or tomorrow?`
<henninge> salgado: oops, I just flicked the mp status back and forth, remembering that you still need an r-c review.
<henninge> just so you don't wonder about the spam hitting your inbox ... ;)
* jelmer changed the topic of #launchpad-reviews to: on call: henninge, - || reviewing: adiroiban(bug-435165), noodles775 || queue [adiroiban(bug-492375 pre-implementation),jelmer]] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), jelmer || queue [adiroiban(bug-492375 pre-implementation),jelmer]] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), jelmer || queue [adiroiban(bug-492375 pre-implementation)]] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [adiroiban(bug-492375 pre-implementation)]] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> abentley: Thanks!
<leonardr> henninge or abentley, will either of you have time today to review a short lazr.restfulclient branch?
<abentley> leonardr: I expect so.
<leonardr> ok, i'll write it up
<leonardr> abentley, henninge: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/anonymous-credential/+merge/16164
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: adiroiban(bug-435165), - || queue [adiroiban(bug-492375 pre-implementation), leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> guess it was the irc connection...
<intellectronica> abentley: care to review a very small js fix which is a release blocker?
<abentley> intellectronica: I have EODed, but since it's a release blocker, go ahead.
<bac> sinzui: got a sec to look at https://code.edge.launchpad.net/~bac/launchpad/bug-496701-mimetypes/+merge/16170 ?
<intellectronica> abentley: you rock. as you can imagine i've EODed quite a few hours ago :) https://code.edge.launchpad.net/~intellectronica/launchpad/manage-official-tags-js-error/+merge/16171
<abentley> intellectronica: r=me
<intellectronica> abentley: thanks a bunch
<intellectronica> danilos: can i land the fix above as rc?
<intellectronica> or flacoste?
<bac> intellectronica: you should add them to the MP with 'release-critical' as the review type
<intellectronica> bac: right. thanks for reminding me
<bac> np, intellectronica
<intellectronica> bac: so is the drill now that i request a review from the release manager with release-critical as the review type?
<bac> intellectronica: indeed
<bac> intellectronica: there may be something else involving bug tags or marking the bug as critical, but i'm not sure
<intellectronica> bac: yes, the bug is tagged 'release-blocker'
<intellectronica> no, 'current-rollout-blocker'
<jml> hello reviewers
<jml> https://code.edge.launchpad.net/~jml/launchpad/fix-release-hot-bugs-486437/+merge/16173
<jml> not urgent, of course, but maybe nice and simple
<bac> hi jml, could you put on your db-reviewer hat and look at https://code.edge.launchpad.net/~bac/launchpad/bug-490505/+merge/15691 ?
<bac> i didnt' think you'd be necessary on it but stub added you.
<jml> bac, sure, will do.
<jml> bac, is it RC?
<bac> jml: good question.  we'd like to get it in this cycle but the review was delayed until yesterday.  so it isn't critical per se, but would be nice to have.
<jml> bac, ok.
<sinzui> bac: am I correct in understanding that your branch change is to the python env that starts lp?
<bac> sinzui: yes
<sinzui> yeah, that cryptic
<bac> sinzui: that bit of voodoo is shown to work
<bac> sinzui: i hate doing it there and hope we get a proper home for such stuff soon
<sinzui> bac: I understand and agree. r=me
<bac> thx
#launchpad-reviews 2009-12-15
<jml> https://code.edge.launchpad.net/~jml/launchpad/fix-release-hot-bugs-486437/+merge/16173 -- could someone please review this?
<thumper> is there a test yet?
<jml> thumper, I don't recall anyone asking for one.
<jml> thumper, or making suggestions as to where said test should go.
<thumper> where is it tested now?
<jml> thumper, there's a page test.
<jml> thumper, which still passes
<jml> thumper, since firefox has no fix released bugs in the sample data.
<thumper> he
<thumper> not much of a test then is it?
<thumper> only saying the things I've learnt to say from you :)
<jml> *grumble grumble*
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/fix-review-comment-field-enablement/+merge/16178
<thumper> now that one is hard to test
<thumper> in fact I have no idea even if it is possible with windmill
<jml> thumper, have you tried?
<thumper> tried to test with windmill, or tried the fix?
<jml> thumper, the former.
<thumper> jml: I've looked at the windmill api
<jml> doesn't it let you record tests?
<thumper> and it isn't obvious how to tab to the next field
<thumper> ah yeah
<thumper> not tried to do that
<thumper> any idea how?
<jml> thumper, nope.
<jml> thumper, happy to say I've avoided the whole mess :)
<jml> thumper, changing the status of a bug is bloody difficult.
<thumper> jml: why?
<thumper> jml: why not just factory.makeBug?
<jml> thumper, because nothing else in the test does that.
<thumper> jml: they have to learn some time :)
 * thumper sniggers
<jml> thumper, well, this patch isn't the place to teach them.
<thumper> :)
<jml> thumper, test added. code fixed.
<thumper> jml: status=any(*UNRESOLVED_BUGTASK_STATUSES)
<jml> thumper, yet?
<jml> yes, rather?
<thumper> does: status=UNRESOLVED_BUGTASK_STATUSES work?
<jml> thumper, no.
<thumper> why?
<jml> thumper, that was the first thing I tried.
<thumper> any is a generator isn't it?
<jml> thumper, no.
<jml> thumper, ha. ha. ha.
<jml> thumper, it's a thing specific to Launchpad
<jml> thumper, from canonical.launchpad.searchbuilder
<thumper> hmm...
<thumper> damn
<jml> thumper, which I bet you didn't know existed
<thumper> nope
<jml> thumper, 'any' is a generator by default
<jml> thumper, me neither :)
<thumper> any becomes a keyword soon isn't it?
<jml> thumper, builtin, I think.
<thumper> ew
<thumper> r=me
<jml> thumper, I don't think searchbuilder is that bad (he says without looking at the code)
<jml> thumper, but to me, it makes more sense to re-use storm primitives
<jml> thumper, thanks.
<jml> I'll land that next Monday!
<thumper> hmm...
<thumper> damn javascript garbage collector
<mrevell> Anyone care to review my branch? It's for an RC -- https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-3112/+merge/16147
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation),adiroiban(bug-121520)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> mrevell: approved
<mrevell> ah, thanks intellectronica
<intellectronica> mrevell: now you need to get an rc, though
<mrevell> intellectronica, yeah, I've got Danilo right where I want him.
<danilo_> mrevell, aha
<mrevell> :)
<intellectronica> mrevell: you do? can you tap him on the shoulder on my behalf?
<mrevell> intellectronica, heh
<danilo_> intellectronica, I've seen yours as well, will be getting to it shortly
<intellectronica> danilo_: great, thanks
<danilo_> intellectronica, ok, so the code looks ok, I've got only some RC-related questions
<intellectronica> yes?
<danilo_> intellectronica, so, how much does this affect our users? is the tag editing completely broken? (I remember you experiencing something along those lines yesterday, but sinzui had no problems editing them for other projects)
<danilo_> intellectronica, and, if it affects only a subset of our users, what subset is it? is it a subset we can direct to edge? how/when do you plan to QA this?
<intellectronica> danilo_: tbh it doesn't affect users all that much. only a subset of users can edit official tags anyway, and those have a workaround if they need to edit urgently
<intellectronica> danilo_: but the fix is small and doesn'ty touch anything else, and oem services have asked that we fix it asap
<danilo_> intellectronica, right, what is that workaround?
<danilo_> intellectronica, that's the other thing: it doesn't touch anything else? should this have a windmill test?
<intellectronica> danilo_: disable javascript and reload the page
<danilo_> (i.e. something else to be touched :)
<danilo_> intellectronica, ok, that sounds nasty, I think it's worth of an RC, but I'd like to see test added as well
<intellectronica> danilo_: ok, let me add a windmill test for this
<danilo_> intellectronica, thanks, if it's going to be a big involved test, I don't want to pressure you, but if it's decently simple, I'd very much like to have it... at least right after the rollout, it doesn't have to be included in this landing
<intellectronica> danilo_: i'll see if i can modify the existing test to just add a dot somewhere and see that it doesn'
<intellectronica> tt fail
<danilo_> intellectronica, sounds good, thanks
<danilo_> allenap, hi, I am looking at https://code.edge.launchpad.net/~allenap/launchpad/bugtracker-snapshot-bug-447100/+merge/16130 â looks very reasonable but the question is why RC? who are the affected users and how many of them are there?
<danilo_> allenap, i.e. can we only fix it on edge after the rollout, would that be good enough for users likely to use it? (we do have both edge and production APIs as well now)
<allenap> danilo_: It probably is enough to just have it on edge.
<allenap> danilo_: There are probably not many affected users.
<allenap> danilo_: And it's not a frequently used page. At least, I hope it isn't; it's configuration.
<danilo_> allenap, right, so I guess we can say no to this RC unless you feel really strongly about it? I don't want you to feel bad about it, but the more RCs we have, the bigger chances of something breaking with this little QA time we've got
<allenap> danilo_: Fair, I'm happy with that. I'll mention it to deryck when he's online. I doubt he'll have a problem with that.
<danilo_> allenap, ok, thanks
<danilo_> allenap, btw, I've marked your MP as 'approved' but that's for regular landing, RC is disapprove :)
<allenap> danilo_: No worries, I got that... but I was tempted to land and use the plausible deniability and ask forgiveness defences ;)
<danilo_> hehe :)
* gmb changed the topic of #launchpad-reviews to: n call: gmb || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation),adiroiban(bug-121520)] || RC candidates take priority || 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:  - || queue [adiroiban(bug-492375 pre-implementation),adiroiban(bug-121520)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> *sigh*
<allenap> danilo_: I can't land anything in PQM. Do you know if it's it in testfix mode? It doesn't look like it should be...
<danilo_> allenap, are you landing to db-devel or devel? (devel is already closed)
<allenap> danilo_: Oh rats.
<allenap> danilo_: When did devel close? I've been trying since yesterday afternoon.
<danilo_> allenap, it was closed together with PQM, I wasn't aware of the option that it could be kept open and that I should have asked spm to do it in such a way
<allenap> danilo_: Okay. Just to check, is it safe to just submit my based-on-devel branch to db-devel?
<danilo_> allenap, should be if all the devel revisions you have are already in stable
<allenap> danilo_: Cool, thanks.
<danilo_> allenap, otherwise, it's still safe if all the tests pass for you :)
<salgado> adiroiban, around?
<adiroiban> salgado: hi. yes.
<StevenK> danilo_, bigjools: Is my netbook-cron branch still going to make it in?
<bigjools> StevenK: yes
<bigjools> it landed already
<StevenK> bigjools: That's odd, I see "Approved revision: not available" on the merge page
<salgado> adiroiban, hi there.  do you have the test failures for that branch handy?  In the merge proposal you said which test failed, but I'd like to see what the failure is
<adiroiban> salgado: yes. let me paste it. 
<bigjools> StevenK: I can assure you it landed
<adiroiban> salgado: http://paste.ubuntu.com/341811/
<StevenK> bigjools: So it's the merge page that's not correct. I can deal with that.
<salgado> adiroiban, is that the only test that failed?
<bigjools> StevenK: what URL are you looking at?
<adiroiban> salgado: the view needs to be refresh, as in that test the translationgroup is initialized with None
<adiroiban> salgado: yes
<StevenK> bigjools: https://code.edge.launchpad.net/~stevenk/launchpad/netbook-cron/+merge/16115
<bigjools> StevenK: I landed a copy of your branch, that's why it didn't set yours
<adiroiban> salgado: now, that we save the translationgroup on view it's like we cache it
<salgado> adiroiban, I see
<adiroiban> salgado: and if meanwhile the group is changed, the view does not know about it.
<adiroiban> but is hard to have that behaviour in a real system
<adiroiban> and with a page reload you will have the problem solved
<adiroiban> i mean you will see the new value
<adiroiban> I  tried to force a refresh in the test, 
<adiroiban> but I don't know what I did wrong, as it did not solve the problem
<salgado> adiroiban, agreed.  I think the best thing is to change the test as it makes no sense for the test to expect that behavior from the view -- specially since it's not documented
<adiroiban> with those changed, I ran a new full test for translations module, and it was ok
<adiroiban> salgado: ok. I will look into that
<adiroiban> since this is not RC , i think there is no hurry
<adiroiban> or do you want to land it now?
<salgado> not really, and even if I wanted, danilo_ wouldn't
<adiroiban> also there is a follow up for that branch to also clean those tests
<danilo_> salgado, that's right :)
<adiroiban> :)
<adiroiban> salgado: thanks for looking into this 
<danilo_> adiroiban, salgado: though, what you are discussing sounds like cachedproperties in a test, though I am just guessing
<adiroiban> danilo_: nope
<adiroiban> is just a = b.c , but c is none
<adiroiban> so then b.c = True
<adiroiban> a is not updated
<salgado> adiroiban, the first thing I'd do is check that self.dsl.distroseries.distribution.translationgroup is not None
<danilo_> adiroiban, I am not sure I follow you :)
<adiroiban> salgado: and if it none ? what shall i do?
<adiroiban> of why that check?
<salgado> adiroiban, then we need to find out why it's not being updated when you assign the group to self.distroseries.distribution.translationgroup
<adiroiban> because view.translationsgroup is None
<adiroiban> so it is not linked with self.distroseries.distribution.translationgroup
<salgado> adiroiban, I meant that after your change to the test, which creates a new view instance in your test method
<adiroiban> salgado: ah :)
<adiroiban> yep
<adiroiban> I will look at that as I still need to learn how all those classed are linked and how does their timeline look like
<salgado> adiroiban, cool. don't hesitate to ask if you have any questions. :)
<adiroiban> salgado: sure. np.
<gmb> salgado, adiroiban: To be clear, which branch are you talking about here? Is it one of the ones currently in the review queue in the /topic?
<adiroiban> gmb: nope
<gmb> adiroiban: Righto, I'll crack on with reviewing those then.
<adiroiban> gmb: thanks. There is no hurry for them
<adiroiban> they are low priority
<gmb> adiroiban: Maybe not, but I'm on-call reviewer and I'm bored ;)
<intellectronica> danilo_: i've added a windmill test and my branch has been re-reviewed. care to take a look again?
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  adiroiban || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilo_> intellectronica, sure, thanks!
<adiroiban> gmb: :D
<gmb> adiroiban: So, to create a merged person, you do the following:
<gmb> person_1 = factory.makePerson()
<gmb> person_2 = factory.makePerson()
<gmb> person_2.merged = person_1
<gmb> That then means that person_2 has been merged into person_1.
<gmb> adiroiban: This branch really needs a test before it lands.
<adiroiban> gmb: ok. np. I will do that
<gmb> adiroiban: Thanks. I'll mark the review as Needs Fixing for now.
<adiroiban> sure
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || 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:  lunch || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing:  lunch || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<StevenK> bigjools: Excuse my complete ignorance about PQM, but it's normal that I can't see the changes in lp:~launchpad-pqm/launchpad/devel
<beuno> StevenK, you should be able to see anything
<beuno> what is it you're trying to see specifically?
<StevenK> beuno: The changes from my netbook-cron branch in what will be released, since I'm slightly paranoid
<beuno> StevenK, it seems to have been landed in db-devel
<beuno> http://bazaar.launchpad.net/~launchpad-pqm/launchpad/db-devel/revision/8790
<beuno> wasn't UNR renamed to UNE?
<StevenK> beuno: Yes, hence the netbook name. It follows precedent from server.
<beuno> ah, missed that
<StevenK> beuno: So in documentation, it's the Ubuntu Server Edition, but the meta package and so on is ubuntu-server. So I'm doing the same thing with netbook.
<beuno> gotcha
<StevenK> beuno: Will db-devel turn into devel? What's the process there?
<beuno> StevenK, db-devel gets rolled out to production directly, not edge daily
<beuno> so, IIRC, it will merge into devel when the roll-out happens
<beuno> if you're worried if it will land, don't worry, if it's in db-devel, it will  :)
<StevenK> When it's scheduled to land?
<beuno> I think roll-out is this week?  tomorrow or Thu?
<beuno> danilos should know
<beuno> or flacoste
<danilos> StevenK, it's going to be on production LP servers on Wednesday 22:00 UTC
<StevenK> danilos: Okay, thanks.
<danilos> StevenK, if you really want the nitty gritty details, db-devel turns into db-stable when all the tests pass, and then we roll out whatever is in db-stable by 12:00 UTC on Wednesday
<StevenK> danilos: Ah!
<danilos> StevenK, your revision (8790) is already in db-stable (up to 8794), so you should be safe
<StevenK> danilos: Excellent, thanks!
<bigjools> StevenK: I told you to trust me ;)
<StevenK> bigjools: You did, yes :-)
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing:  -, - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || 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: bac || reviewing:  -, - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb goes off-call to fix an RC bug.
<gmb> deryck: lp:~gmb/launchpad/windmill-test
<deryck> gmb, got it.  thanks.
<deryck> taking a break from your usually cleverly named branches? :)
<gmb> deryck: Only the dupefinder got Hammer Horror titles ;)
<gmb> I'm saving the next batch for bug heat :)
<deryck> heh
<gmb> Right. Coffee, then hacking.
<leonardr> salgado, do you have time to review https://code.edge.launchpad.net/~leonardr/launchpad/anonymous-oauth/+merge/16199? it shouldn't take long
<salgado> leonardr, otp
<salgado> leonardr, do you mind if I do it after lunch?
* irc.freenode.net changed the topic of #launchpad-reviews to: on call: bac || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> Hi bac, got time for a short+simple review? https://code.edge.launchpad.net/~allenap/launchpad/bugwatch-bugtasks-as-a-list/+merge/16200
<bac> allenap: for you?  sure!
<bac> allenap: is this an RC candidate/
<bac> ?
<allenap> bac: Thanks :) And no, not RC yet, but I might ask.
<allenap> bac: There's probably a bit too much lint cleaning in there - sorry, I got a little enthusiastic - but it's still quite short and understandable.
<gmb> bac: Can you review https://code.edge.launchpad.net/~gmb/launchpad/checkwatches-keyerror-oops-bug-496988/+merge/16204 for me (RC candidate)
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing:  gmb || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> gmb: in your final test it is not clear that either of those bug watches cause a key error.  is it b/c there is a bug 1 but not bug 2?  i think the casual reader might need more info there.
<mup> Bug #1: Microsoft has a majority market share <iso-testing> <ubuntu> <Clubdistro:Invalid> <Computer Science Ubuntu:Invalid by compscibuntu-bugs> <EasyPeasy Overview:In Progress by ramvi> <Ichthux:Invalid by raphink> <JAK LINUX:Invalid> <OpenOffice:Invalid by lh-maviya> <Tabuntu:Invalid by tinarussell> <Tivion:Confirmed for shakaran> <Ubuntu:In Progress by sabdfl> <ubuntu-express (Ubuntu):Invalid by jahyire2006> <Ubuntu Jaunty:In Progress> <
<bac> thanks mup.  helpful!
<gmb> bac: Ah, right. I'll add a comment.
<bac> gmb: thanks, r=bac
<gmb> bac: Ta.
<jpds> Do non-RC MPs get reviews too at the moment?
<bac> jpds: depending on reviewer availability.
<bac> jpds: i can do a review now if the changes are short
<jpds> I have: https://code.edge.launchpad.net/~jpds/launchpad/fix_450262/+merge/16203
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing:  jpds || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jpds: that's pretty short.  i'll do it now.
<bac> jpds: in addition you'll need to get a UI review.  it is always welcome to have screen shots for those added to the bug and/or referenced from the MP
<bac> jpds: also there is no test for this new functionality.  you'll need to add one.  there are plenty of examples of test that check to ensure email is sent or not sent.
<jpds> bac: Something like http://people.canonical.com/~jpds/fix_450262_screen.png ?
<bac> jpds: yep
<jpds> bac: OK, prefect, I'll look for existing test and will add a new one.
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> bac: Tests for fix_450262 added and pushed.
<bac> thanks jpds.  i have to go out now but will look at it later
<jpds> No problem.
<leonardr> salgado, ping re my review?
<salgado> leonardr, did you get it?
<leonardr> salgado: i'm looking on the web
<leonardr> checking email now
<salgado> I sent it 10 minutes ago, but I see the mp hasn't been updated yet
<leonardr> salgado: i haven't gotten the email yet either
<salgado> An error occurred while processing a mail you sent to Launchpad's email
<salgado> interface.
 * salgado resends
<leonardr> salgado, can you point me to example code so i can see how to check that consumer is stored in the database?
<salgado> leonardr, doc/oauth.txt
<leonardr> ok
<salgado> you might need to login() in your test, though
<leonardr> salgado, while you're at it, do you want to look at my launchpadlib branch?
<leonardr> my response to your review shouldn't affect this code
<salgado> leonardr, sure
<leonardr> ok, writing it up
<leonardr> salgado: https://code.edge.launchpad.net/~leonardr/launchpadlib/anonymous-access/+merge/16213
#launchpad-reviews 2009-12-16
* al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation), al-maisan(http://tinyurl.com/y8joo24)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing:  - || queue [adiroiban(bug-492375 pre-implementation), al-maisan(http://tinyurl.com/y8joo24)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing:  adiroiban || queue [al-maisan(http://tinyurl.com/y8joo24)] || RC candidates take priority || 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: jtv, allenap || reviewing: adiroiban, - || queue [al-maisan(http://tinyurl.com/y8joo24)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adiroiban: you're trying to get a pre-implementation call for your branch, right?
<adiroiban> jtv: yep. but there is no hurry
<adiroiban> i just want to know if it't ok to augment a model in the view in that way
<gmb> jtv, allenap: RC branch for your perusal, if you please: https://code.edge.launchpad.net/~gmb/launchpad/ubuntu-filebug-dupefinder-bug-497096/+merge/16230
<jtv> adiroiban: oops, gmb's takes precedence.  I'll let yours lie for the moment, then
<adiroiban> jtv: sure :)
<allenap> jtv: I can take it.
<jtv> allenap: oh, that would solve it too.  :-)
<jtv> hi btw
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: adiroiban, gmb || queue [al-maisan(http://tinyurl.com/y8joo24)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> Hi jtv :)
<allenap> al-maisan: Is your branch an RC candidate?
<jtv> adiroiban: so back to yours...  I'll browse through your diffs a bit first
<adiroiban> jtv: look at the view class
<al-maisan> allenap: no, it's not
<al-maisan> allenap: .. and good morning :)
<adiroiban> jtv: i would like to tag preferred langauages so that it makes it easy and nice to presented them different in the template
<allenap> al-maisan: Morning :)
<jtv> adiroiban: I'm comparing to productseries to get an idea of what danilo is saying
<adiroiban> jtv: I talked with danilo and we agree to do the filtering using css class and javascript
<adiroiban> jtv: the current branch is not ready yet. I just create a filter on a single page
<adiroiban> jtv: here is the nasty code http://bazaar.launchpad.net/~adiroiban/launchpad/bug-492375/revision/10055#lib/lp/translations/browser/distroseries.py
<jtv> adiroiban: thanks
<jtv> oh, that's the same page I was already looking at :)
<adiroiban> :)
<danilos> adiroiban, btw, I think your branch already fix that bug 347... (allowing translation group owners to administer langpacks)
<jtv> hi danilo :)
<danilos> jtv, hi
<adiroiban> danilos: maybe :) i have no idea how lang-packs are handled 
<jtv> I think we have a "link" formatter for languages... makes me wonder if the CSS class for preferred languages should be in there, and the default "seen/unseen" choice for non-preferred ones set in local CSS
<adiroiban> danilos: i just commented on that bug and assigned so that I can look into it
<danilos> adiroiban, it should be pretty simple to test, log in as an ubuntu translation group owner but without any admin or rosetta-experts privileges to launchpad.dev and go to translations.launchpad.dev/ubuntu/hoary/+language-packs and see if the form shows up and you can edit it
<allenap> gmb: Should lines 20 to 33 also be moving?
<danilos> jtv, it should, I'd say!
<danilos> jtv, I mean, it's a great idea :)
<danilos> adiroiban, yep, thanks
<jtv> oh, probably in js, not cssâso that you get all languages when js is disabled
<jtv> (barring cases where we have a clickthrough to a full list I guess)
<gmb> allenap: Eh... Hmm. Well, it passes the tests ;). But no, probably not. I'll move those back.
<danilos> adiroiban, if you find it's fixed by your previous branch, please do assign yourself to it... if it's not, it can be because we are not using the same privilege on distroseries, and then it's still only a simple fix away
<gmb> allenap: Although that gets a bit knotty then...
<adiroiban> danilos: yep. let me start firefox
<danilos> jtv, well, we only set class "preferred" on preferred languages, but language formatter is not very useful here since we are linking to ProductSeriesLanguage or DistroSeriesLanguage (or in the future, ProjectLanguage or SourcePackageLanguage)
<adiroiban> it looks like in webkit based browser I can not see some sprites
<gmb> allenap: No, I'll move it back, because otherwise it will have nasty effects on the other +filebug views when extra data get passed.
<jtv> danilos: facepalm :)
<gmb> allenap: Nice catch.
<danilos> adiroiban, yeah, I've seen that too, I think we have an existing bug on those
<jtv> we have a problem with konqueror with <a> tags that have no text in them...
<adiroiban> danilos: checking in firefox... no new edit stuff. I will look into that as by now I should know how to fix it
<jtv> there's a little workaround for the konqueror problem though
<adiroiban> share it :)
<adiroiban> i'm using chromium and epiphany and I can not see the import queue entry edit icons
 * jtv digs up lazr-js fix
<adiroiban> danilos: as soon as I finish with the current branches I will start hunting the lang-pack bug. Right now I'm looking at import queue permission
<danilos> adiroiban, I am not going to push you at all, it's not like you need pushing :) so, just take your time :))
<adiroiban> danilos: :)
<adiroiban> i'm still learning the LP security mechanism
<allenap> gmb: I have broken your merge proposal.
<gmb> allenap: Fail.
<gmb> allenap: Just do the second review that's listed for you; it should DTRT.
<allenap> gmb: No, I mean it's OOPSing all the time when I go to the page.
<jtv> adiroiban: okay, it's nasty: if you have a link with just an icon in it (nothing else), konqueror sees it as empty and so doesn't render it.  You can work around it in CSS.  To :link and :visited pseudoclasses for the link, you add content: ""
<allenap> gmb: https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1446EB307
<adiroiban> how long does it take to run a full test on ec2? On an 2G AMD, running test -m translations it took me 200minutes
<jtv> adiroiban: that's pretty long...  but a full ec2 test is going to take longer
<jtv> adiroiban: but
<allenap> gmb: I'm filing a bug for it.
<allenap> gmb: In any case, r=me.
<jtv> I'm pretty sure you can fire off an ec2 image with your own arguments for bin/test
<gmb> allenap: Ta. I'll have an incremental diff for you in a second...
<allenap> gmb: Cool.
<adiroiban> jtv: np. I was just courious
<jtv> adiroiban: the old ec2test command would just pass on any extra args
<adiroiban> jtv: so getting back at my branch (sorry for going offtopic)
<gmb> allenap: Inc diff posted to https://code.edge.launchpad.net/~gmb/launchpad/ubuntu-filebug-dupefinder-bug-497096/+merge/16230
 * gmb asks Santa for permalinks to MP comments.
<allenap> gmb: That page is OOPSing for me :) I'll wait for the email.
<gmb> Hah.
<jtv> adiroiban: yes... afaics, if js is turned off, your branch shows only the preferred languages.  Don't we want all languages in that case?
<gmb> allenap: http://pastebin.ubuntu.com/342580/ if you want it quicker ;)
<adiroiban> jtv: ok. sure
<adiroiban> jtv: i was worried about the usage of class DistroSeriesLanguageTagged:
<jtv> adiroiban: yes, I do wonder whether that's really needed
<allenap> gmb: I got it, thanks. Looks good, r=me.
<adiroiban> jtv: adding it as an attribute to SeriesLanguage model is also strange
<gmb> allenap: Thanks.
<adiroiban> as it's only a view attribute
<adiroiban> and the model should not care about it
<allenap> gmb: I'll try and reply to the mp via email to approve it too.
<gmb> Thanks.
<jtv> adiroiban: very true
<adiroiban> jtv: this is my dilemma
<adiroiban> jtv: i could just put that logic in the template... but it would make the template look bad
<adiroiban> s/bad/ugly/
 * jtv wonders if factoring out a SeriesLanguageView would help
<adiroiban> jtv: SeriesLanguageView or ( DistroSeriesView and ProductSeriesView) ?
<jtv> adiroiban: a replacement for ProductSeriesLanguageView & DistroSeriesLanguageView
<jtv> adiroiban: they're practically identical, structurally
<adiroiban> jtv: yes... but I don't know how this can solve the langauge tagging problem
<jtv> but you'd have to render the table rows differently to get to that view
<jtv> well, you'd have a single place to tag a <tr> as being for a preferred language or not
<adiroiban> jtv: yes. but my problem is how to tag it in the view
<jtv> maybe I'm thinking too much about the cleanup part :)
<allenap> gmb: I can't update that mp by email either (bug 497338).
<mup> Bug #497338: OOPS on merge proposal page <oops> <Launchpad Bazaar Integration:New> <https://launchpad.net/bugs/497338>
<adiroiban> jtv: i can do the cleanup... but I still don't know how to tag a preferred langauge in the view
<danilos> jtv, adiroiban: do note that when you are going through a list of those, they are read from an attribute on the model class, but the view class extends them with preferred languages; that's the right place to add "preferred" decorators on them I guess
<adiroiban> jtv: other than the proposed solution
<adiroiban> danilos: I think that's what I'm doing now... or not?
<danilos> adiroiban, I don't know :)
<adiroiban> danilos: ok :)
<jtv> adiroiban: I don't think anyone is saying the basic low-level approach is wrong
<jtv> but we're talking about where to put it
<jtv> since we already have view classes for {product,distro}serieslanguage
<gmb> allenap: What did you do? Honestly... Okay, no worries, I'll add a note to the effect that you broke everything.
<allenap> gmb: Cool :)
<jtv> adiroiban: It may start to matter as the code deals with more special cases: not logged in, no preferences set, no JS enabled.  Don't want to duplicate that code.
<adiroiban> jtv: I can add an interface/baseclass... but I don't know how to avoid the usage of SeriesLanguageTagged
<adiroiban> jtv: or is ok to go with a SeriesLanguageTagged class?
<danilos> adiroiban, jtv: I'd say just put it in the model for now, since there's a lot more refactoring possible there anyway
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: adiroiban, al-maisan || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> danilos: maybe an "is_preferred"?
<jtv> hmmm that still leaves the messy stuff in the tal
<adiroiban> danilos: putting it the model will also imply changing the permission
<danilos> jtv, yeah, it's just a place to hold the value that will be passed in from the construction in the view
<danilos> adiroiban, what permission would need to change?
<adiroiban> for accessing the is_preferred
<adiroiban> as it would be written in the view
<danilos> adiroiban, well, of course, you'd have to allow it, but you can actually pass it in from the constructor/and or (D|P)SLSet instead
<adiroiban> danilos: I don't know... I don't feel we need to have that attribute in the model
<jtv> adi has a point...  it gets ugly
<adiroiban> it is something binded to the user configurations
<danilos> adiroiban, well, you've got two options then: initialise a view for each of the series languages (even uglier, if you ask me: that's what we are doing for +translate page and it's very bad) or...
<jtv> now, I'm very very rusty and vague on the zope part of this, but if we can render the table rows from a separate view, then that view seems the ideal place for thisâand we have the two near-identical *SeriesLanguageViews already
<danilos> adiroiban, or create a new decorated SeriesLanguage class which wraps (Product|Distro)SeriesLanguage and contains such things
<allenap> al-maisan_: Line 332, does getPublishedSources() sort the results?
<al-maisan_> allenap: just a minute please
<danilos> jtv, yes, we can do that in zope
<jtv> but not nice?
<adiroiban> danilos: that's what I'm doing now... a new class that wraps 
<danilos> jtv, that's close to what I proposed above, except that it would again be a view
<adiroiban> danilos: or at least... I think I'm doing it :)
<jtv> adi: yes, you are... and I must admit it's not a lot of overhead
<danilos> adiroiban, the problem with that approach is that it gets ugly faster... DSL and PSL should implement the same interface, and then you could have a wrapper class/row view for that interface
<adiroiban> danilos: i will start with implementing a single interface
<adiroiban> and the do the wrapper
<adiroiban> for a single serieslangauge
<danilos> adiroiban, jtv: and, we should have a shared interface for something that IHasSeriesLanguageObjects which allows you to render these listings, including marking languages as preferred and similar
<jtv> that does get to be more work...
<al-maisan_> allenap: yes, it does.
<danilos> with that, I don't see why we'd need a wrapper interface anyway, so... :)
<allenap> al-maisan_: Cool.
<danilos> jtv, indeed
<jtv> for the DSL/PSL views we don't even need two separate classes AFAICSC, but that's another matter
<danilos> jtv, exactly, the views should be registered on the interface they share
<adiroiban> ok. now it's getting messy :D
<danilos> jtv, that interface should support ProjectLanguage and SourcePackageLanguage in the (close) future
* jpds changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: adiroiban, al-maisan || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adiroiban, heh, naah...
<danilos> adiroiban, jtv: that's why I say let's get this into a model for now, and if we want to clean it up, let's start top-down
<adiroiban> jtv, danilos: the plan: implement the wraper as it is now. and then look look how all these views can be refactored
<danilos> adiroiban, jtv: i.e. first step is to make PSL/DSL use a shared interface (even if that interface includes is_preferred, that's ok for now)
<adiroiban> like you said... it is to much for now
<adiroiban> so we need to break the work
<danilos> adiroiban, jtv: next step would be providing a SLCollection interface to be implemented by ProductSeries, DistroSeries,...
<danilos> adiroiban, exactly, so I'd keep it as simple as possible for now and put it in the model :) if you also refactor PSL/DSL to share the interface and use the same view, that's even better :)
<adiroiban> danilos: but I still need to hold the css_class as an attribute
<danilos> adiroiban, or have an ugly TAL :) perhaps tal:define is not that bad in here because it will simplify TAL and let you get on with the actual feature without being too bad ;)
<adiroiban> so instead of is_preferred, i need to have css_class
<danilos> adiroiban, yep, see above
<adiroiban> right now I don't see to much sense in putting in on the model. it should be in the view or the template
<jtv> we could also have it all in JS, I guess
<adiroiban> jtv: yes :)
<jtv> we'd be giving the JS a list of preferred languages
<adiroiban> jtv: maybe that would be the "cleanest" approach for now
<danilos> jtv, that's an option as well
<jtv> well the dirty little skeleton in that can of worms is how to make the tal pass the list to the js
<allenap> al-maisan_: getChangesFilesForSources() seems to now only be used in lib/lp/soyuz/adapters/archivesourcepublication.py. Is there any way to change that to use your new query?
<jtv> ...if you don't mind me mixing a few metaphors
<adiroiban> jtv: just put it in a json/javascript structure
<jtv> it's not a big skeleton :)
<al-maisan_> allenap: I am not sure, that method is selecting and pre-joining a lot more data and that probably makes sense for the web UI
<jtv> adiroiban: an added advantage (I think) is that the "full view" stays the natural consequence when JS is disabled
<jtv> and afaics there are no changes when reusing between different "serieslanguage" types
<allenap> al-maisan_: Okay. Do you think you could comment in getChangesFilesForSources() that getChangesFileLFA() contains a potentially more efficient query, depending on what you need out of it.
<adiroiban> jtv: http://bazaar.launchpad.net/~adiroiban/launchpad/bug-359180-event-key/revision/8787#lib/lp/translations/templates/pofile-translate.pt line 288
<al-maisan_> allenap: sure, will do that.
<adiroiban> is a javascript that holds a tal loop
<adiroiban> in that way I can write the prefered langauges in a js array
<jtv> danilos, didn't you also do something like what adiroiban just pointed to for PSL?
<danilos> jtv, btw, doing it in JS is going to make some very ugly TAL
<allenap> al-maisan_: There aren't any unit tests for getChangesFileLFA(). I can see that changes_file_url is tested in a few places, but it's not quite the same thing. Unless there's a good reason not to, could you add this?
<al-maisan_> allenap: good point, I'll add unit tests for getChangesFileLFA().
<danilos> jtv, /me looks
<allenap> al-maisan_: Thanks.
<adiroiban> danilos: <tal:block tal:replace="structure string:&lt;script type='text/javascript'&gt;" />
<adiroiban> to trick tal thinking that you are still in HTML/XHTML land
<jtv> danilos: actually, not much like that bit.  :)  What I meant was, each PSL row gets a CSS class with the language code in it
<danilos> adiroiban, the problem is that you want to get preferred languages from the view using TAL and put that into JS
<jtv> a trick for that is to embed a single js function call in the tal that passes the desired arguments
<jtv> really minimizes the tal impact
<adiroiban> jtv: function... or just an array
<jtv> ...or an array.  :)
<danilos> adiroiban, jtv: right, and if it's not clear yet, I find that just doing it in the model is much, much nicer
<danilos> adiroiban, jtv: you can argue sanity of that any day, but I am going to argue sanity of this kind of JS back every time, and I'll still feel like I am right :)
<adiroiban> danilos: ok. then I will add a "css_class" in the model?
<adiroiban> :D
<danilos> adiroiban, no, add is_preferred to the model
<adiroiban> and how can i convert the is_preferred to a css class?
<danilos> adiroiban, and in TAL do something like <tal:preferred condition="lang/is_preferred" define="css_class preferred" /> or something like that
<danilos> adiroiban, using tal:condition
 * jtv stops trying to interfere :-)
<adiroiban> danilos: ok. let me try and see what I can get
<danilos> adiroiban, alternative is to not add anything to the model, and use the similar tal:condition to define CSS class using a view model and something like tal:condition="python:langstats.language in view.preferredlanguages"
<danilos> adiroiban, with above, you won't even have to modify the model, just expose preferredlanguages in the view
<adiroiban> danilos: but I will have to use define="global css_class string:preferred"
<adiroiban> or otherwise there will be a big duplicate code for the row
<adiroiban> is this ok?
<adiroiban> to use global?
<danilos> adiroiban, yes, don't tell me that's uglier TAL than for inserting JS :)
<danilos> adiroiban, yeah, it's ok to use a global but then name it something more unique just in case (i.e. preferred_language_css_class)
<adiroiban> danilos: nope
<jtv> isn't it going to break the matching def for the next row
<jtv> though?
<danilos> jtv, what do you mean? we'll want to define it to an empty string otherwise, that's for sure
<adiroiban> danilos , jtv : thanks! I will go with TAL only
<adiroiban> python and global
<jtv> danilos: don't know what global does there, so wondering whether anything will break if you re-define the same name later
<danilos> adiroiban, instead of doing an in like I suggested above, it's probably marginally better to have a method on the view isPreferredLanguage and call that instead
<adiroiban> danilos: ok
<danilos> jtv, oh, I am sure that'll be fine
<adiroiban> may the TAL be with me
<danilos> adiroiban, you'll still have to call the method with python:view.isPreferredLanguage(langstat.language) but I think that's better
<danilos> adiroiban, :)
<adiroiban> yep
<adiroiban> ok. I think it's enough for now
<jtv> adiroiban: and while we're complicating your life, don't forget the cases where there are no preferred languages.  :-)
<adiroiban> jtv: :)
<adiroiban> jtv, danilos . that should be all for the pre-implementation call. Thanks!
<jtv> adiroiban: good luck, thanks for working on this!
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: -, al-maisan || queue [jpds] || RC candidates take priority || 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: jtv, allenap || reviewing: -, - || queue [jpds] || RC candidates take priority || 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: jtv, allenap || reviewing: -, <release-qa> || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> https://code.edge.launchpad.net/~jpds/launchpad/fix_450262/+merge/16203 â all set for review. :)
<jtv> jpds: shouldn't you follow that one up with bac?
<jpds> jtv: Hmm, yeah, I thought it could do with more eyes too.
<jtv> jpds: makes sense, but it also confuses the situation a bit... like asking your dad for a cookie because your mom already said no.  :-)
<jpds> jtv: But I asked for a biscuit!
<jtv> jpds: I see you have worked on this strategy
<jpds> jtv: I'll wait for bac.
<jtv> jpds: IIRC his review said he'd review the updated MP, right?  He should be around soon, if not already.
<maxb> If you don't mind a slightly different kind of review: https://code.edge.launchpad.net/~maxb/meta-lp-deps/no-pullparser/+merge/16232
<adiroiban> jtv: if a person had not defined yet his/her preferred langauges, LP tries to be smart and provides a list with possible langauges
<adiroiban> so we can not have an empty list of preferred languages
<jtv> adiroiban: IIRC we've sort of been moving away from that, but I can't say I kept much of an eye on it
<jtv> (we had endless fun with a unit test that used Serbia & Montenegro as a geoip example :-)
<BjornT> maxb: approved
<adiroiban> jtv: ok. then if there is no language I will set all launguages as preferred
<maxb> BjornT: Thanks.
<jtv> adiroiban: and don't forget to exclude en/en_US  :-)
<jtv> oh, just "en" I think
<adiroiban> jtv: well I will just use the current list
<adiroiban> jtv: i will finish it and push a branch for review
<jtv> adiroiban: it all depends how many layers of abstraction you include, I suppose.  :)  In call now.
<jtv> great!
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: -, <lunch> || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* BjornT changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: -, <lunch> || queue [jpds, bjornt] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> jtv-eat, allenap: i've added a small branch to the queue. it's not an rc candidate
 * bac enjoying a quick breakfast
<bac> jpds: i'll have a look in a bit
* irc.freenode.net changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: -, <lunch> || queue [jpds] || RC candidates take priority || 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: jtv, allenap || reviewing: -, <release-qa> || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: jtv, allenap, Edwin || reviewing: -, <release-qa> || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> jtv, allenap: I have a branch for you to review: https://code.edge.launchpad.net/~gmb/launchpad/another-checkwatches-keyerror-bug-497414/+merge/16243
<gmb> Argh, netsplit.
<gmb> jtv-eat allenap: I have a branch for you to review: https://code.edge.launchpad.net/~gmb/launchpad/another-checkwatches-keyerror-bug-497414/+merge/16243
<gmb> Sorry if you see this message twice; I got netsplit'd so I don't know who I hit and who I missed
<allenap> jpds: Has someone picked up your branch?
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap, Edwin || reviewing: -, - || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: How would all_remote_ids contain an id not in bug_watches_by_remote_bug?
<jpds> allenap: bac's going to have a look at it.
<gmb> allenap: Well, I haven't figured that out yet; that's a separate bug. But the point is that if we log the error rather than just blowing up when we hit it we can file a bug and investigate later rather than worrying about checkwatches failing all the time.
<allenap> jpds: Cool. Thanks for being patient today :)
<allenap> gmb: Oh, does it fail here too?
 * allenap reads the bug report so as to stop wasting gmb's time :)
<allenap> gmb: Ah, so it does :)
<gmb> allenap: Heh.
<gmb> Yeah, the bug we just fixed was masking this one.
<allenap> gmb: r=me
<gmb> allenap: Awesome, thanks.
<allenap> gmb: checkwatches is getting too complex to understand.
<gmb> allenap: It did that years ago. It's since uploaded itself onto the web as pure energy and is responsible for all the major wars in the world.
<allenap> gmb: Neuromancer, Wintermute... now CHECKWATCHES!
<gmb> allenap: Doesn't *quite* have the same ring to it, but we can work on it.
<EdwinGrubbs> allenap: can you review my branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part1/+merge/16226
<allenap> EdwinGrubbs: Sure, I'll do it now.
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap, Edwin || reviewing: -, Edwin, - || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge_: is there anything I need to do for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-435165/+merge/16084 ?
<EdwinGrubbs> henninge_: do you want me to review your branch?
<adiroiban> there is also this on https://code.edge.launchpad.net/~adiroiban/launchpad/bug-435165/+merge/16084 :)
<adiroiban> sorry. stupid paste
<henninge> EdwinGrubbs: That would be really nice.
<henninge> EdwinGrubbs: That would be really nice. I see you already claimed it.
<EdwinGrubbs> yes, I'm working on it
<leonardr> salgado-lunch: updated https://code.launchpad.net/~leonardr/launchpad/anonymous-oauth/+merge/16199
<salgado> leonardr, ok, will get to it in a minute
* henninge changed the topic of #launchpad-reviews to: on call: jtv, allenap, Edwin || reviewing: -, Edwin, henninge || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> leonardr, did you reply to the review or just pushed a new version of the branch to launchpad?
<leonardr> salgado: i replied with an incremental diff
<leonardr> is it not there?
<salgado> leonardr, nope.  I think attachments are ignored
<leonardr> salgado: i pasted a link to ubuntu pastebin
<leonardr> 1 sec, on phone
* jtv-eat changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: Edwin, henninge || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> henninge: I don't understand why there is both an IAccountModerate and an IAccountModerateStatus. Is that so that the status is viewable by anyone?
<henninge> EdwinGrubbs: Yes, it is in IAccountPublic (and was before I touched it).
<henninge> that is the reason for these two interfaces.
<leonardr> salgado; doh! i never clicked 'save comment'
<salgado> heh
<leonardr> salgado: i found a bug in malone, but while i'm filing it...
<leonardr> http://pastebin.ubuntu.com/342820/
<henninge> EdwinGrubbs: this whole interface-zcml relationship is quite confusing to me, too, and it took me a few attempts to get it right.
<EdwinGrubbs> henninge: I'm wondering if it would be simpler to use <require set_attributes="status status_comment"> instead of <require set_schema="..."> which requires a complicated inheritance structure.
<henninge> EdwinGrubbs: is there some kind of precedence of set_attributes over set_schema?
<henninge> I mean, can an attribute be mentioned in an Interface and also explicitly in set_attribute? I think only that would help.
<EdwinGrubbs> henninge: I've usually seen set_schema used when there are tons and tons of attributes and you want to avoid duplicating them in the zcml. I think using <require interface="..."> for viewing attributes but using set_attributes when set_schema doesn't map to an existing interface might work.
<henninge> EdwinGrubbs: ok, I am happy to try that and get this whole thing simplified.
<EdwinGrubbs> henninge: I also encountered these errors in the tests: http://pastebin.ubuntu.com/342855/
<henninge> EdwinGrubbs: huh, I ran that test all the time. But maybe not after the final changes.
* allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: henninge || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> EdwinGrubbs, can I stick another branch on your queue?
<rockstar> (it's not an RC candidate)
<leonardr> salgado: a quick incremental diff for the launchpadlib branch. i'd like your opinion on a test change
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/anonymous-access/+merge/16213
<EdwinGrubbs> rockstar: sure, I'll start on it after lunch.
<rockstar> EdwinGrubbs, thanks.
<EdwinGrubbs> henninge: I sent you an intermediate review with a few comments on the code besides what we discussed already.
<henninge> EdwinGrubbs: thanks. I got the interfaces figured out.
<leonardr> salgado: about this code
<leonardr> assert self.consumer is not None, (
<leonardr>             "Need a registered consumer key for authenticated requests.")
<leonardr> if i put an assert there, i can't verify that the server rejects an authenticated request with an unknown consumer key
<leonardr> it gets rejected before the request is made
<salgado> oh, ok, I didn't realize that
<salgado> that's testing code, so I guess it's ok to be permissive
<leonardr> i think i can use your structure and it'll be more understandable, at the cost of 1 duplicate line of code
<leonardr> yeah, it's more readable
<leonardr> salgado: any idea why i would get this test failure?
<leonardr> http://pastebin.ubuntu.com/342959/
<leonardr> the second part is my rearrangement of the code in response to your most recent comment
<leonardr> i'm running the same test on a fresh launchpad to see where the error came in
<salgado> leonardr, looks like we have a time bomb in our test suite
<leonardr> salgado: yes, i get the error in a fresh launchpad, so i'm going to be a coward and not deal w/it
<salgado> leonardr, a fresh devel or db-devel branch?
<leonardr> salgado: devel
<salgado> leonardr, it might be fixed in db-devel
<leonardr> salgado: argh, i have to do a new release of launchpadlib before i can land this branch, because the old launchpadlib contains test failures when run against the new launchpad
<leonardr> (because the old launchpadlib has a test that verifies you can't log in to the web service when providing no credentials)
<leonardr> salgado: so if you have an opinion on my new launchpadlib test (http://pastebin.ubuntu.com/342874/), i'd like to hear it
<salgado-afk> leonardr, I think that extra test won't hurt, so I think it's ok to leave it
<leonardr> salgado: cool
<rockstar> EdwinGrubbs, hi.
<EdwinGrubbs> rockstar: hi, I saw that Tim reviewed your branch
<rockstar> EdwinGrubbs, ah, it was Tim, not you.  Okay, maybe I need to chat with him.  He apparently knows something I don't.
<rockstar> EdwinGrubbs, I thought it was you, and so I was going to ask some questions.
#launchpad-reviews 2009-12-17
<mwhudson> jml: https://code.edge.launchpad.net/~mwhudson/launchpad/recipe-model-code/+merge/16272 if by some freak event you're still around
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * noodles775 checks for jpds' mp
<noodles775> Seems to be reviewed already.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [] || RC candidates take priority || 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 [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> noodles775: ui review for the changes would be good.
<noodles775> jpds, ok, will do!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, - || queue [jpds-ui] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> noodles775: Ah, that ubuntu-team thing is... interesting.
<jpds> noodles775: I think we should just be able to remove people silently as the bug report says, but adding should still notify people.
<jpds> Have to fix the template now.
<jpds> Is there a way of having multiple tal:condition='s in one <span> ?
<adiroiban> jpds: nope. but you can use python to handle complex conditions
<adiroiban> tal:condition="python: PYTHON WONDERS LAND"
<adiroiban> and view/something is view.something
<jpds> adiroiban: Neat.
<jpds> noodles775: Thanks for the review, I've fixed the issues you pointed out.
* adiroiban changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, - || queue [adiroiban(bug-492375)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * maxb would like to enqueue https://code.edge.launchpad.net/~maxb/meta-lp-deps/no-sshd/+merge/16280
* maxb changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, - || queue [adiroiban(bug-492375), maxb#495454] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jpds: regarding always notifying on enabling - yes, I think you're right. I was thinking specifically about the case where:
<noodles775> an admin silently deactivates a membership, but then needs to re-enable it as it was a mistake, but in that case, there's no need to do it silently - a mistake can be admitted :)
<noodles775> jpds: and thanks for implementing the other suggestions!
<jpds> noodles775: Interesting situation, althought I'd imagine that this feature would be used with caution.
<jpds> noodles775: Pushed final changes, you can land if you're happy with the changes.
<noodles775> jpds: thanks! I'll leave the landing to bac when he re-reviews the code changes.
<jpds> \o/ Awesome.
<noodles775> maxb: sorry, I'm not so familiar with the lp dependencies - is there a reason why you've not incremented the version to 0.62 and added a new changelog entry, rather than editing the 0.61 entry that you created with r67?
<noodles775> adiroiban: thanks for the details in your MP. Unless I hear from sinzui (he may want to take it) I'll start a UI review in an hour or so, and leave the code to al-maisan.
<adiroiban> noodles775: sure. there's no hurry
<jpds> Hmm, now that the rollout is done, does that mean that we can land https://code.edge.launchpad.net/~jpds/launchpad/fix_176396/+merge/16082 ?
<maxb> noodles775: Because 0.61 hasn't been built/uploaded yet
<noodles775> maxb: ah thanks. I've already approved it.
<leonardr> salgado: can you spare a couple minutes to help me wrap up the launchpad anonymous-access branch?
<salgado> leonardr, sure, what's up?
<leonardr> salgado: http://pastebin.ubuntu.com/343545/
<leonardr> i made a tarball of launchpadlib 1.5.4 (pre-release) and changed launchpad to use it
<leonardr> i ran the tests last night and they all passed
<leonardr> so if you'll approve that branch i can land it
<salgado> sure, looks good to me, but you won't be able to land it as pqm is closed
<leonardr> argh. i always lose track. when does it reopen? next year? and how can i find that information as someone who only does a launchpad branch very occasionally?
<maxb> noodles775: What's the proper [r=] tag for you? I had assumed it was LP username, but I see you're michael.nelson there, but I'm fairly sure I've seen [r=noodles] in the past
<noodles775> maxb: yeah, noodles is it.
<maxb> gah, and after asking that, I got carried away with debcommit and forgot to add it
<al-maisan> noodles775: how does the ui/code review split work?
<noodles775> al-maisan: you just do a normal code review.
<al-maisan> noodles775: OK, thanks!
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, adiroiban(bug-492375) || queue [maxb#495454] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> hello adiroiban : just making sure I understand things properly: should I see the "View all languages" link on https://translations.launchpad.dev/ubuntu when I am *not* logged in?
<adiroiban> al-maisan: yes
<al-maisan> adiroiban: pebkac, I started looking at the page prior to merging your branch :P
<al-maisan> sorry
<adiroiban> al-maisan: well... you lost me :)
<adiroiban> ah.
<al-maisan> adiroiban: don't worry .. I just need to merge your branch ..
<adiroiban> google put me back on track :)
<al-maisan> adiroiban: based on geoip the following languages are offered to me while I am logged off: afrikaans; sotho, southern; xhosa, zulu
<al-maisan> that's a bit funny since I am based in DE
<adiroiban> yep...
<adiroiban> I did not changed that code
<al-maisan> adiroiban: no problem -- just commenting on it..
<adiroiban> adiroiban: yep. I know. Jeroen told me they are thinking to remove that feature
<adiroiban> still, in the current code I'm just using the list
<al-maisan> adiroiban: the page you described in the "Demo and Q/A" works as expected .. now on to the code
<adiroiban> hooray
<al-maisan> I am getting this error when I run a windmill test:
<al-maisan>   Set up lp.translations.windmill.testing.TranslationsWindmillLayer <type 'exceptions.KeyError'>:
<al-maisan> 'MOZILLA_BINARY'
<al-maisan> any idea what it means?
<al-maisan> this is the full paste: http://pastebin.ubuntu.com/343602/
<leonardr> noodles776 or al-maisan, can you put a quick contrib branch in the queue?
<leonardr> https://code.edge.launchpad.net/~rick-rickspencer3/lazr.restfulclient/add_gaurd_for_None_to_eq_functions/+merge/16043
<adiroiban> do you have firefox installed in the right location?
<leonardr> review my revised diff, not the original branch. (i can push another branch if you want, but i'd like to keep the review here)
<al-maisan> adiroiban: ah, that explains it, I don't have firefox installed at all -- this is run in a chroot
 * al-maisan installs firefox
<adiroiban> :)
<adiroiban> This is my first windmill test
<al-maisan> noodles775: could you take the branch that leonardr asked about .. I'll need to attend the other meeting in an hour..
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, adiroiban(bug-492375) || queue [adiroiban-ui, maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, adiroiban(bug-492375) || queue [adiroiban-ui, maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: i've put it in the queue for the moment, so either I or one of the US reviewers will get to it :)
<al-maisan> noodles775: good man :) thanks!
<noodles775> leonardr: I don't understand - haven't you just reviewed that LAZR branch? or at least asked for changes (extra tests)?
<leonardr> noodles775: i pointed out that there were no tests, and went ahead and added the tests because rick isn't on irc and i figured that was the best way to show him how the test system worked
<leonardr> but since my code is now in the branch i don't want to approve my own code
<noodles775> leonardr: right - I was confused because the revision with your tests wasn't yet showing on the branch (on the MP).
<noodles775> Still isn't, but I assume it'll appear soon :)
<leonardr> noodles: i don't think i can push to the branch--it's rickspencer3's branch
<leonardr> that's why i offered to push a different branch, but said i'd prefer that the review happen in the original
<noodles775> leonardr: Sorry - I should read a bit more thoroughly before asking questions. OK, I'll review it with your tests and assume they'll be included.
<leonardr> yeah, my branch is what i'll merge
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: adiroiban-ui, adiroiban(bug-492375) || queue [maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> adiroiban: this windmill test fails on my system (maybe due to my strange setup):
<al-maisan> test_results: ERROR    Test Failure in test {'version': '0.1', 'suite_name': 'SeriesLanguages Tables', 'result': False, 'starttime': '2009-11-17T20:43:5.858Z', 'output': None, 'debug': 'Looking up name loginpage_submit_login, failed.', 'params': {'name': 'loginpage_submit_login', 'uuid': 'acee01d8-eb1e-11de-a198-00218652d375'}, 'endtime': '2009-11-17T20:43:5.859Z', 'method': 'asserts.assertNode'}
<adiroiban> al-maisan: ah... that is part of user.ensure_login(self.client)
<adiroiban> and it fails for all windmill tests
<al-maisan> adiroiban: good to know :) thanks!
<adiroiban> from canonical.launchpad.windmill.testing import lpuser
<adiroiban> and I'm using user = lpuser.TRANSLATIONS_ADMIN
<adiroiban> to get a user for which I know the preferred languages
<adiroiban> I think the windmill test are still very fragile
<adiroiban> for translations
<adiroiban> are are only 2 or 3 other windmill tests
<al-maisan> adiroiban: so, I am wondering: will this also fail after the branch is reviewed and submitted to PQM?
<adiroiban> al-maisan: the translations Windmill test are disabled in pqm
<adiroiban> tests
<al-maisan> adiroiban: ah, yes, that's right!
<al-maisan> thanks again!
<adiroiban> np
<al-maisan> adiroiban: you added isPreferredLanguage() twice .. is there any particular reason for that?
<adiroiban> it should be an error 
<adiroiban> let me check
<adiroiban> on is for distroseries
<adiroiban> and the other is for productseries
<adiroiban> but that should be all
<al-maisan> yes
<al-maisan> was there no good place to put it once and reuse it?
<adiroiban> no
<adiroiban> we talked about that in the pre-implemetnation call
<al-maisan> aha
<adiroiban> in the future we should have a single serieslanguage view
<adiroiban> for both product and distro
<adiroiban> right now also preferred_languages
<adiroiban> implements the same logic
<adiroiban> in both classes
<adiroiban> i think there is a bug about than creating the interface... and if not, I will add it now
<al-maisan> adiroiban: cool .. please provide me with the bug number.
 * adiroiban is searching 
<adiroiban> bug 496361
<mup> Bug #496361: Register a single view (SeriesLanguageView)  for all ISeriesLanguage-implementing objects <Launchpad Translations:New for adiroiban> <https://launchpad.net/bugs/496361>
<al-maisan> adiroiban: thanks !
<adiroiban> but there is a lot of refactoring work in there 
<adiroiban> so maybe after analyzing it, it should be splited in other bugs
<al-maisan> sure, no problem
<al-maisan> adiroiban: there is one little typo (please see the review email), otherwise r=me
<adiroiban> al-maisan: thanks. I'll fix it and push the changes right away... anyway we can not land it now
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: adiroiban-ui, <meeting> || queue [maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> al-maisan: i don't see any info about the typo
<noodles775> adiroiban: ok, I've just sent the ui review too... great work!
<al-maisan> adiroiban: just a sec
<adiroiban> sure
<al-maisan> + * Set up initial the visibility for languages in a serieslanguages table.
<al-maisan> should probably read:
<al-maisan> "Set up the initial visibility.." ?
<al-maisan> adiroiban: ^
<adiroiban> yes. sorry for that. 
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: maxb#495454, <meeting> || queue [leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> adiroiban: take it easy :) erring is human, and, your branch looked great otherwise.
<noodles775> Sure did/does!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: leonardr, <meeting> || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> noodles775, adiroiban: my review email appeared with some delay but it's there now.
<adiroiban> al-maisan: ah. ok. np
<adiroiban> noodles775: regarding word capitalization. I agree
<adiroiban> but I tried to be consistent with the previous implementation
<adiroiban> https://translations.edge.launchpad.net/ubuntu/karmic/+source/gfxboot-theme-ubuntu/
<adiroiban> so then I should also open a bug for that
<adiroiban> ?
<noodles775> adiroiban: yeah, I saw that you'd kept it consistent. I don't think it's worth a bug to fix the capitalization - it's easy enough to do little drive-by cleanups like this when the page is touched. Up to you.
<adiroiban> noodles775: ok. I will change it in this branch
<noodles775> Great, thanks!
<adiroiban> noodles775: I will also hide the âShow all languagesâ when not needed. I was thinking that most translators/visitors will define their preferred languages and I was avoiding to much logic in that view
<noodles775> adiroiban: ah, is "English" the default if a person has never set their preferred language? I didn't realise that. But yes, there could be a lot of people in that situation.
<noodles775> (I mean, as in "English" is actually selected, as opposed to not having any preferred language selected).
<salgado> adiroiban, did you run the test suite after doing the test changes you just added to https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994?
<adiroiban> salgado: is running
<adiroiban> not finished yet
<salgado> adiroiban, I think it won't work because your changes expect view.initialize() to return the view, but it doesn't
<adiroiban> salgado: true
<adiroiban> salgado: i need to do self.view.initialize()
<salgado> right
* al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: leonardr, - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adiroiban, it may be that you don't even need to instantiate the view again -- just calling .initialize() on it when you update stuff might be enough
<adiroiban> salgado: indeed ... now testing
<adiroiban> salgado: is working for @property but not for @cachedproperty
<salgado> right, that's expected.  I'd forgotten there was a cached property
<adiroiban> I'll push the changes and start a full test using that code
* noodles775 changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue [] || RC candidates take priority || 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: - || reviewing: - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gary: https://code.edge.launchpad.net/~leonardr/launchpadlib/use-shorthand-uris-in-all-tests/+merge/16298
<leonardr> the diff is here if you need it:
<leonardr> http://pastebin.ubuntu.com/343631/
<gary_poster> leonardr: on it
<gary_poster> leonardr: my only thought is that it might be nice to explain what that test_dev string is in introduction.txt.  Agree?
<gary_poster> (and where it is defined)
<leonardr> gary, sure
<gary_poster> leonardr: ok, will merge-conditional on that then
<leonardr> gary: it's defined in uris
<gary_poster> leonardr: yeah, I saw that in the diff.  I meant that it would be nice to have an explanation in the text
<leonardr> sure
<leonardr> gary: ok, you should be able to update launchpadlib and do a release now
<gary_poster> leonardr: http://paste.ubuntu.com/343640/
<leonardr> wtf, how did that test not get run
<leonardr> it looks like they haven't been run for a while
<leonardr> that first failure looks like it was caused by a change landed while i was on vacation
<leonardr> gary: ok, the problem is that testing launchpadlib in a launchpad context only runs the doctests, not the unit tests
<gary_poster> leonardr: ah ok
<leonardr> i and others have only been running the doctests. i'd like to fix launchpad to run the unit tests too, but i'll settle for fixing the tests for now
<gary_poster> fair enough
<gary_poster> maybe add a bug for getting launchpad to run the unittests?
<leonardr> yeah
<leonardr> if allenap were around i'd have a better shot at fixing this stuff, he landed the two branches
<gary_poster> urg. :-(
<gary_poster> yeah, the last one was the only one that clearly pointed to your recent changes
<leonardr> gary: there's a chance it might be more fallout from my login_With refactoring
<gary_poster> leonardr: ok.  You agree that we ought to give you a chance to fix these before making a release?
<leonardr> gary: yes
<gary_poster> cool
<leonardr> gary: https://code.edge.launchpad.net/~leonardr/launchpadlib/test-failures/+merge/16303
<gary_poster> leonardr: approved (sorry was on calls)
<gary_poster> leonardr: going to lunch then will make release
<leonardr> gary: np
<leonardr> all right
<abentley> Is there a team for UI reviewers?
<gary_poster> leonardr: if you make the comment fix and merge then I can make the release
<leonardr> gary: i'm having trouble committing
<leonardr> https://bugs.edge.launchpad.net/bzr/+bug/246233 seems to be happening to me
<mup> Bug #246233: TooManyConcurrentRequests error when ssh connection fails (bzr crashes when pulling) <hpss> <5-a-day-data:Invalid> <Bazaar:Fix Released by spiv> <https://launchpad.net/bugs/246233>
<leonardr> ok, it seems to have been transient
<leonardr> gary: commited now
<gary_poster> cool thanks
<allenap> gary_poster, leonardr: Hi, I did run the test suite before landing those branches ;) There was one failing test which I filed a bug for iirc, but obviously it's not good now.
<leonardr> allenap: it turned out to be my fault
<gary_poster> leonardr: did you see my comment about line 39 in the diff?
<allenap> leonardr: Oh, phew :)
<gary_poster> allenap: what he said. :-) thanks for saying hi though
<gary_poster> leonardr: "be returned as is).  :param launchpadlib_dir: The user's" should have a \n in there
<leonardr> gary: missed that, i'll fix it
<gary_poster> thanks
<leonardr> i also fixed a typo that broke a test, but i think i fixed that before you saw the branch
<gary_poster> yes you did
<gary_poster> the review UI is imporving nicely
<gary_poster> improving
<leonardr> gary: i also added instructions on developing lazr.* branches and getting them landed to https://dev.launchpad.net/HackingLazrLibraries
<gary_poster> leonardr: I saw that in your email to jml but had not read the content.  Looks great, thank you.
<leonardr> gary: ok, formatting is fixed
<leonardr> hopefully the release will work now and i can give you the stuff to land my launchpad branch once pqm opens
<gary_poster> leonardr: everything worked.  going to pick up son.
<leonardr> gary: ok, i'll make instructions for you
#launchpad-reviews 2009-12-18
<jml> mwhudson, I'll have a look today
<mwhudson> jml: oh thanks
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [wgrant] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [wgrant, noodles775] || RC candidates take priority || 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: - || reviewing: - || queue [wgrant, noodles775, henninge] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: do you have any idea why the config changes you are implementing in your branch were kept on a wiki page?
<henninge> wgrant: I mean, as opposed to doing what you are doing now from the start. Did the soyuz team expect problems with these changes?
<henninge> noodles775: ^
<wgrant> henninge: I just didn't ever submit them.
<henninge> wgrant: Oh, you wrote those instructions!
<wgrant> henninge: I did.
<wgrant> Well, most of them, at least. Various bits have been added since.
<henninge> wgrant: So it is a well tested setup?
<wgrant> It has been used by only a few people, but it does work.
<henninge> I guess what I want to know is if the soyuz team is aware of that, i.e. if you had a  pre-implementation chat about them.
<henninge> wgrant: ^
<wgrant> henninge: Not really, no.
<henninge> wgrant: don't get me wrong, the changes look fine to me.
<henninge> but I guess since this does not affect production code, I should not be over-paranoid.
<noodles775> I wasn't aware of them, but am all for them.
<henninge> wgrant: r=me ;)
<wgrant> The process-upload.py invocation is stolen from the production configs, actually.
<wgrant> henninge: Thanks. Can you land it?
<henninge> wgrant: sure
<wgrant> henninge: Thanks.
<henninge> wgrant: can you set a commit message, please?
<henninge> noodles775: did you just reserve a spot in the queue or do you actually have  an mp ready ... ;-)
* henninge changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles775, henninge] || RC candidates take priority || 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: - || reviewing: - || queue [noodles775, henninge] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> henninge: Done.
<noodles775> henninge: I've got an MP which was already approved, but I've since added a few revisions:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/475808-unembargoing-oops/+merge/16141
<noodles775> And since the revs are now displayed inline, you can see which ones :) (The bug has the information about what failed during QA on dogfood etc., as well as the new QA which went through df fine).
<noodles775> henninge: are you OCR'ing today?
<noodles775> or just being very helpful :)
<henninge> noodles775: no, I was going to offer to trade reviews again ... ;)
<noodles775> henninge: sure - although I'll not get to it 'til after lunch, so it might be worth waiting for a better prospect ;)
<henninge> noodles775: but I think with your branch I'd feel better if Aaron picked up on it. Or is he on leave already?
<henninge> noodles775: yes, mine is not urgent anyway
<noodles775> henninge: I think he'll be around when it's morning.
 * henninge goes to land wgrant's branch
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [noodles775, henninge] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> noodles775, has your branch been reviewed already?
<noodles775> salgado: no it hasn't. It's actually a re-review (I should have requested a new one), as I've updated it since it was approved: 
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/475808-unembargoing-oops/+merge/16141
<salgado> oh, ok.  I'll get to it in a minute
<noodles775> Thanks.
<salgado> noodles775, what is the ancestry component of a delayed copy?
<noodles775> salgado: An older version of the package that has been published in the destination archive.
<noodles775> It's used to determine the correct component to publish to ('main', 'universe', 'multiverse' etc.)
<salgado> right
<salgado> noodles775, """Return the ancestor of the given parkace
<salgado> in getNearestAncestor
<noodles775> salgado: Right, thanks. I guess my fingers slipped there!
<salgado> noodles775, I think the docstring for that method should also state that it can return None 
<noodles775> Yep, I'll update that too.
<salgado> 172	+            if (is_ppa and binary.component not in
<salgado> 173	+                distroseries.upload_components):
<salgado> 174	                 # Only complain about non-PPA uploads.
<salgado> noodles775, ain't that going to complain only about PPA uploads instead of non-PPA ones, as the comment says?
<noodles775> salgado: yes - I replaced line 155 with 172 - and missed the not, that saves one ec2 failure - thanks!
<salgado> you mean, if we have a test for that. ;)
<noodles775> if? ;)
<adiroiban> henninge: is there anything I need to do for this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-435165/+merge/16084 :D
<noodles775> salgado: just for the pride of soyuz, distroseriesqueue.txt would have failed ;)
<salgado> heh. ok
<henninge> adiroiban: yes, keep poking your reviewer about it ... ;-)
<henninge> adiroiban: or find a new one
<adiroiban> :) no hurry. I just wanted to make sure the ball is not on my side :D
<salgado> noodles775, r=me with those small changes
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [henninge] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> salgado: thanks, already pushed. BTW: I'm planning to land it on devel now, rather than db-devel as the mp says.
<salgado> noodles775, yeah, I guessed that from reading your comment on the bug report
<salgado> henninge, I assume yours is bug-489227-import-status?
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [-] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> salgado: yes, it is
<henninge> salgado: thanks for the review. ;) Yes, background would probably be helpful. Sorry about that, I was lazy and just copied the bug description.
<EdwinGrubbs> salgado: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325
<EdwinGrubbs> salgado: you will find it very familiar
<salgado> that's what I was going to say
<salgado> EdwinGrubbs, how did you split it up?
<EdwinGrubbs> salgado: all of your original changes are in the first branch plus a tiny bit of cleanup.
<EdwinGrubbs> salgado: this branch redid a lot of the javascript because adding teams was more complicated than expected.
<jpds> abentley: Ping.
<abentley> jpds: pong
<jpds> abentley: Hi, would you be able to land https://code.edge.launchpad.net/~jpds/launchpad/fix_176396/+merge/16082 for me now that PQM is opened?
<salgado> EdwinGrubbs, because we had to update different sections of the page or something else?
<abentley> jpds: I think we still have the re-roll to do, don't we?
<jpds> Re-roll?
<abentley> We usually do a second rollout on Fridays to fix any issues we discovered after the release.
<jpds> Ah, right, did not know that.
<EdwinGrubbs> salgado: yes, I added the Recently invited list, since that is the status that new teams should be added to if you admin the super team. If you admin the member team, then it will go into the proposed status, but you have to do that through the "Add my teams" page.
<salgado> EdwinGrubbs, right. we completely forgot about the latter case.  you still have to use the "Add my teams" page, though, if you want to propose one of your teams as member?  or does the new widget handle that as well?
<EdwinGrubbs> salgado: the new widget handles that now. The model's addMember() method now checks whether the reviewer is an admin of the owning team and then sets the status to APPROVED or INVITED accordingly.
<salgado> oh, nice
<EdwinGrubbs> salgado: that's backwards, the reviewer has to be an admin on the new member team. The user has to be an admin of the owning team to even see the "Add member" link.
<salgado> right
<salgado> EdwinGrubbs, I'll step out now to get some lunch but I'll review your branch as soon as I get back
<salgado> and thanks a lot for finishing it
* abentley changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [abentley] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> salgado-lunch: Could you review my proposal too please?  https://code.edge.launchpad.net/~abentley/launchpad/jobstatus/+merge/16309
<salgado> abentley, sure
<EdwinGrubbs> salgado: I'm back
<salgado> EdwinGrubbs, sorry, was on a call.  I'll continue your review now
<salgado> EdwinGrubbs, btw, would you mind asking somebody else to have a look at your windmill test?  I've never written one myself, so can't even think about reviewing it
<EdwinGrubbs> salgado: ok
<EdwinGrubbs> rockstar: can you do a UI review and review a windmill test. Salgado is reviewing the rest of the code in the branch.
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> salgado: will you be here on Monday if I don't finish the changes today?
<salgado> EdwinGrubbs, yep
<EdwinGrubbs> intellectronica: ping
<intellectronica> EdwinGrubbs: yo
<salgado> mars, when you have some time, can you have a quick look at https://code.edge.launchpad.net/~salgado/lazr-js/bug-482235/+merge/16349 for me?  I'd like some feedback (mostly on the XXXs there and the lack of documentation for the plugin) before I polish it and ask for a proper review
<EdwinGrubbs> intellectronica: hi
<intellectronica> EdwinGrubbs: ?
<EdwinGrubbs> intellectronica: you said "yo"
<intellectronica> EdwinGrubbs: you said "ping"
<EdwinGrubbs> intellectronica: oops, sorry, I was going to ask you if you could do a ui review and review a windmill test. Salgado reviewed the rest of the branch.
<EdwinGrubbs> https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325
<intellectronica> EdwinGrubbs: sure. i can review the test. for the ui review you'll have to find one more reviewer
<EdwinGrubbs> intellectronica: thanks, will do
<EdwinGrubbs> intellectronica: I have to reboot, so I'll be away for just a sec
<intellectronica> EdwinGrubbs: i need to go now, but if you still want me to review request it in an mp and i'll do it later
* 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
#launchpad-reviews 2010-12-20
<wgrant> StevenK: https://code.launchpad.net/~wgrant/launchpad/bug-692114-recipes-main-component/+merge/44184 would enjoy your perusal.
<StevenK> Are you sure?
<StevenK> wgrant: Can't you use IArchive.getComponentsForSeries()?
<wgrant> StevenK: I considered that, but how?
<StevenK> IDistribution.current_release ?
<wgrant> Which component do I take from that list?
<StevenK> It's for a PPA, so it should always be main, and it allows us to extend recipes without having to change that bit, right?
<wgrant> StevenK: But if an archive has more than one component, which one do I pick?
<StevenK> And that really does sound like swapping one hard code for another
<wgrant> Yes.
<wgrant> We will need a proper solution eventually.
<wgrant> But it is larger than just getComponentsForSeries.
<StevenK> thumper: ^ when you have a sec
<thumper> done
<wgrant> Thanks StevenK, thumper.
<wgrant> StevenK: Around for another one?
<StevenK> wgrant: Suppose
<wgrant> StevenK: https://code.launchpad.net/~wgrant/launchpad/bug-685401-expired-queue-files/+merge/44190
<wgrant> It will be good when we have five qualified reviewers here.
<StevenK> wgrant: +1, but I'm not sure if thumper is still around
<wgrant> Hopefully not.
<wgrant> Thanks.
<StevenK> wgrant: Looks like your MP is +1 from thumper too
<wgrant> StevenK: Indeed, thanks.
<wgrant> Thanks thumper.
<thumper> wgrant: np
<StevenK> thumper: You could have a look at my +34/-7 MP if you have time -- a no, bugger off is perfectly fine
<thumper> StevenK: link?
<StevenK> thumper: https://code.launchpad.net/~stevenk/launchpad/text_to_html-no-linkify/+merge/44192
<thumper> done
<jcsackett> anyone available to review https://code.launchpad.net/~jcsackett/launchpad/better-bug-linking-531889/+merge/44098
<leonardr> jcsackett, i'll take it
<jcsackett> thanks, leonarder.
<jcsackett> er, leonardr.
 * leonardr is now even leonarder
<leonardr> jcsackett: i'm not sure why you left the ? in the regular expression
<leonardr> eg. you went from number\.? to number?
<leonardr> which i think will match either "number" or "numbe"
<jcsackett> leonardr: you are correct; i should switch that to (number)?, i think.
<jcsackett> no, wait, number i already part of a ?'d group.
<leonardr> yeah
<leonardr> just take out the ?
<jcsackett> okay, i'll just remove that '?'. good spot. :-)
<leonardr> jcsackett: what's the change on line 69 of the diff?
<leonardr> are these just spacing/line formatting chagnes at the end?
<jcsackett> leonardr: just fixing indentation in that broken up string.
<jcsackett> that line should be aligned with the two lines above it that it's part of.
<leonardr> jcsackett: r=me with that change
<jcsackett> leonardr: thanks, and thanks for the review!
<leonardr> np
* sinzui changed the topic of #launchpad-reviews to: On call: -  || Reviewing: - || queue: [ sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> EdwinGrubbs, can you take a look at benji's review of https://code.launchpad.net/~leonardr/launchpadlib/retry-on-invalid-token/+merge/43784 ?
<EdwinGrubbs> sure
* abentley changed the topic of #launchpad-reviews to: On call: abentley  || Reviewing: - || queue: [ sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: abentley  || Reviewing: - || queue: [ ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> leonardr: review sent
<leonardr> thanks
<allenap> abentley: Do you have time for https://code.launchpad.net/~allenap/launchpad/bug-activity-grouping-bug-353890/+merge/44247? It's quite long, but a lot of it is doctest/import change noise.
<abentley> allenap, okay.
<allenap> abentley: Thanks.
* abentley changed the topic of #launchpad-reviews to: On call: abentley  || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> abentley: I need to head off in about 20 minutes, so please kick it back if you'd prefer for me to be available.
<abentley> allenap, there's a lot here.  Maybe it's better to get a proper review tomorrow.
<allenap> abentley: Okay, I'll do that. Thanks.
<allenap> abentley: Okay, I'll do that. Thanks.
<abentley> allenap, tx.
* abentley changed the topic of #launchpad-reviews to: On call: abentley  || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> thumper, https://code.launchpad.net/~abentley/launchpad/move-reviewer-selection/+merge/44250
<abentley> thumper, here's the other: https://code.edge.launchpad.net/~abentley/launchpad/more-diff-update-msg/+merge/44288
<thumper> done
<abentley> thumper, thanks.
<abentley> thumper, that first one was my first use of soupmatchers.  Kinda undocumented, but looks powerful.
<thumper> yeah
<james_w> abentley, what sort of documentation were you looking for?
<abentley> james_w, it seemed to lack documentation of parameter types.  I didn't find anything that told me I needed to use HTMLContains, either.
<james_w> abentley, did you find the README?
<abentley> james_w, no.  I don't usually look for that.
<james_w> ok
<abentley> james_w, I read the source.
<james_w> ok
<abentley> james_w, Within doesn't mention that its inputs need to be tags.  I tried HTML ids first.
<james_w> abentley, do you think that the API should be improved, or it was just confusion from poor documentation?
<abentley> james_w, It would definitely be nicer if the DocumentPart matchers worked on HTML strings.
<james_w> abentley, so you can type '<a class="foo">' and it will parse it to get the match spec?
<abentley> james_w, right.
<james_w> seems feasible
<abentley> james_w, it would also be nice if there was a shorthand for Tag('foo', None, attrs={'id': 'foo'})
<james_w> yeah
<abentley> james_w, this is my test: http://pastebin.ubuntu.com/546111/
<abentley> james_w, ideally it would be assertThat(browser.contents, Not(tag_id('field.reviewer').within(tag_id(''mergeproposal-extra-options))) or something.
<james_w> right
#launchpad-reviews 2010-12-21
<wgrant> thumper: Could you be convinced to review https://code.launchpad.net/~wgrant/launchpad/die-lucilleconfig-die/+merge/44305?
<wgrant> Or abentley, if you are still around.
 * thumper looks
<thumper> you need to get stub to do the patch allocatino
<wgrant> thumper: I already obtained a number from him yesterday.
<wgrant> Thanks.
* abentley changed the topic of #launchpad-reviews to: On call: -  || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> wgrant: is stub off now?
<wgrant> thumper: I don't think so.
<thumper> wgrant: well if you have the nod from stub, feel free to land it
<wgrant> thumper: Oh, I don't actually have a review, just a number.
<StevenK> wgrant: Win
<wgrant> StevenK: Yes.
<StevenK> wgrant: And thanks for tacking on the IDS change
<wgrant> StevenK: It didn't really fit into any of my previous branches :(
<wgrant> But regardless, that 6-year-old temporary hack is gone!
<allenap> deryck: Are you busy (stupid question)? Would you have time for a review or two? Both are quite large branches (701 lines and 1077 lines).
<deryck> allenap: I can work through them, through the day, if that's okay.  I tend to be slow on large diffs.
<allenap> deryck: Thank you. The smaller one is reasonably straightforward: https://code.launchpad.net/~allenap/launchpad/sub-filters-via-api-bug-672619/+merge/44331
<allenap> I'll badger someone else for the bigger branch.
<deryck> heh, ok.
 * deryck goes to warm up the coffee before settling in
<allenap> leonardr: Hi there. Are you on-call reviewing this week? If so, would you be able to review an oversize branch of mine? It's not too complicated actually; a lot of the bulk is doctest and import noise.
<leonardr> allenap: sure
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> give me the mp
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> leonardr: Thanks. It's https://code.launchpad.net/~allenap/launchpad/bug-activity-grouping-bug-353890/+merge/44247
<leonardr> allenap: line 292, i don't see a big readability advantage to defining an expression as a lambda function
<leonardr> versus just using the expression in the list comprehension
<leonardr> what do you think?
<allenap> leonardr: Yeah, you're right, I'll put it into the comprehension.
<leonardr> k
<leonardr> allenap: how about line 66, replacing chain(comments, activity) with comments + activity? is performance super important? i had to look up what chain() does
<allenap> leonardr: Performance in bug comments especially has been quite critical, but I doubt changing it as you suggest would have much or any impact. I used chain() because comments and activity are generators; I didn't nead to materialize them into almost-immediately discarded lists so I didn't. The itertools module has a lot of very nice things in it, and I would argue that it's worth being familiar with.
<allenap> leonardr: I could have done something like:
<allenap> events = []
<leonardr> allenap: ah, i didn't see that they were generators
<leonardr> chain is fine in that case
<allenap> leonardr: Okay, cool :) I won't go on with my "something like" bit then :)
<leonardr> allenap, having trouble wrapping my head around the idea of "first newest comment". what's going on there?
<allenap> leonardr: Yeah, it doesn't read well. When there are many comments, only the first X and last X comments are shown; in the code that's oldest_comments and newest_comments. The first newest comment is the oldest of the most recent comments to be shown.
<leonardr> why is the first newest comment the oldest?
<leonardr> ah, it's first chronologically
<allenap> leonardr: Yep.
<leonardr> oldest_new_comment might make more sense
<allenap> leonardr: I originally had oldest_newest_comment... perhaps I should rename newest_comments as recent_comments, then have oldest_recent_comment.
<leonardr> that sounds good
* sinzui changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: allenap || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> allenap: what happens in the section starting around line 527?
<leonardr> it looks like you used to print out activity_and_comments, then take a look at the most recent comment, and now you just print everything out at once?
<allenap> leonardr: Mmm, I don't remember what I was doing there. I'll get back to you.
<leonardr> ok
 * leonardr thinks you could put the time format you use on lines 602 and 616 into a constant
<leonardr> line 651 "no activities is" => "no activities are"
<leonardr> test_common_actor_over_a_prolonged_time would be easier to understand if you mentioned that the grouping window is 5 minutes
<allenap> leonardr: The time format is only used in the __repr__(), so just for debugging.
<allenap> leonardr: Oh, yes, good idea.
<leonardr> allenap: what would happen to test_interleaved_activity_with_comments_by_common_actor if there was another comment after activity2?
<leonardr> would activity2 be associated with comment or comment2?
<leonardr> (also, the test name is inaccurate since you only have one comment in that test)
<allenap> leonardr: test_two_comments_with_activity_by_common_actor shows that I think; activities are grouped towards the earlier comment.
<leonardr> right, ok
<leonardr> the tests are great
<allenap> leonardr: Cool, thanks :)
<leonardr> allenap: why &\x238594; instead of &#8594;? does the # make your editor think the rest is a comment?
<allenap> leonardr: Yeah, doctest-mode is a little stupid. I'll switch it back for the benefit of others.
<leonardr> ok
<allenap> Although doctest-mode is excellent in many ways :)
<allenap> Ah, the bit around line 527 is exactly as you say: it now prints out everything at once.
<leonardr> allenap: one more question about your interesting_changes_expression
<allenap> Go for it.
<leonardr> bugtask_change_re looks like it matches some optional text, then a colon, then one of assignee, importance, etc.
<leonardr> is that right?
<leonardr> anyhow, there's something, then one of a number of field names
<allenap> leonardr: Yeah, the optional text is the name (according to valid_name()'s rules) of the bugtask target to which the activity pertains.
<leonardr> so interesting activity is a change to a bugtask, or a change to a bug
<leonardr> which is why you don't have that special stuff for the bug fields
<leonardr> right?
<allenap> leonardr: Yeah.
<leonardr> ok, this looks good
<leonardr> i'll summarize the changes we tlaked about in the review
<allenap> leonardr: Cheers, thank you!
<allenap> leonardr: After your comments I thought of a way to make interesting_activity a bit more obvious: http://paste.ubuntu.com/546305/
<leonardr> allenap, i like it
<allenap> leonardr: Cool.
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> sinzui, what is your merge proposal?
 * sinzui looks
<jcsackett> leonardr: for your queue https://code.launchpad.net/~jcsackett/launchpad/autofocus/+merge/44284
* jcsackett changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> leonardr, https://code.launchpad.net/~sinzui/launchpad/buga-buga/+merge/44297
<leonardr> ok
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: sinzui || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> sinzui: in newbug.py you remove a test for 'event_creator is not None'. will that never happen?
<leonardr> line 48, "bug nomiation" => "bug nomination"
<leonardr> line 31, "submit label to" => "submit label"
<sinzui> leonardr, it cannot ever happen
<leonardr> ok
 * leonardr doing final runthrough
<sinzui> leonardr, the code requires use to use a transition method
<sinzui> leonardr, I talked to deryck[lunch] about that because I could not imagine that case to be valid
<leonardr> sinzui, r=me with typo fixes
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: jcsackett|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> thanks
<leonardr> jcsackett: am i right in thinking that the inputs whose focus you set will always be present?
<leonardr> if so, r=me
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> if not, will the javascript cause a problem if that input isn't around?
<leonardr-dentist> i will be back in a few hours and i can take a couple more reviews when i do
* sinzui changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: -|| queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: -|| queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac, jcsackett: can you review a trivial branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-526608-milestone-ajax-plus-icon/+merge/44382
<jcsackett> EdwinGrubbs: i'm happy to look at it.
<jcsackett> EdwinGrubbs: the two "style_for_" things are just to hide the irrelevant controls on milestones, right? so if there's a milestone, don't show add.
<EdwinGrubbs> jcsackett: right
<deryck> sinzui: would you be willing to review my fix removing all the linked js files and not depending on devmode for using launchpad.js?
<jcsackett> EdwinGrubbs: the tool that does the text show hide is already tested in windmill elsewhere, right?
<EdwinGrubbs> jcsackett: the widget in lazr_js actually hides and shows the elements with the classes "addicon", "editicon", and "nulltext", so that has yui tests. I'm not sure how detailed the windmill tests are.
<jcsackett> EdwinGrubbs: dig, that was my intuition. just checking for the lack of new test coverage, but since it's using stuff that's already safely in use elsewhere that seems fine to me.
<jcsackett> EdwinGrubbs: r=me.
<jcsackett> sinzui: can you follow up for me on https://code.launchpad.net/~edwin-grubbs/launchpad/bug-526608-milestone-ajax-plus-icon/+merge/44382?
<sinzui> yes
<jcsackett> anyone available to review a very trivial branch: https://code.launchpad.net/~jcsackett/launchpad/unlink-321675/+merge/44388
<bac> jcsackett: sure
<jcsackett> thanks, bac.
<leonardr> sinzui, what's the other branch you want me to review?
<sinzui> leonardr, https://code.launchpad.net/~sinzui/launchpad/peppermint-sticks/+merge/44378
<leonardr> sinzui: line 301, "by their names" => "by their display names", and show the display names?
<leonardr> r=me apart from that
<sinzui> thanks for pointing that out
#launchpad-reviews 2010-12-22
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr  || Reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* leonardr changed the topic of #launchpad-reviews to: On call: - || Reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: None || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi jelmer!
<henninge> jelmer: I have a nice branch for you ;)
<jelmer> Hi Henning!
<jelmer> That's great, I prefer nice branches over not so nice branches :-P
<henninge> Wonderful! ;)
<henninge> https://code.launchpad.net/~henninge/launchpad/devel-487137-custom-language-codes/+merge/44446
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: henninge  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> I just mailed in incremental diff because most changes have already been approved. It should show up in the mp soon.
<henninge> jelmer: ^
<henninge> thanks ;)
<jelmer> henninge: are there no explicit tests to see that a "normal" user can't add custom language codes?
<henninge> jelmer: is that not what the "Unprivileged access" section shows?
<jelmer> henninge: You're right, sorry!
<jelmer> henninge: not sure how I missed that
<jelmer> henninge: r=me
<henninge> jelmer: thank you :-D
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || eviewing: sinzui ||  queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: jelmer || eviewing: sinzui ||  queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: sinzui ||  queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> jelmer, can i throw another (very, very) small branch on your queue?
<jcsackett> https://code.launchpad.net/~jcsackett/launchpad/autofocus-2-135018/+merge/44472
<jelmer> jcsackett: sure
<jcsackett> thanks.
* jcsackett changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: sinzui ||  queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> jcsackett: I'll do yours first
<jcsackett> jelmer: works for me. :-P
<jelmer> jcsackett: r=me
<jcsackett> jelmer: thanks!
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> sinzui: What is "discluded" ? :-)
<sinzui> opposite of include
<jelmer> excluded?
<sinzui> ah
<sinzui> much better work
<sinzui> word
<jelmer> apparently discluded is a word but it means disclosed
<sinzui> I do not think I mean that. I will change the comment to us exclude
<jelmer> sinzui: r=me
<sinzui> thanks jelmer
* jelmer changed the topic of #launchpad-reviews to: On call: None || reviewing: None || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> anyone available to review a branch that makes a quick change to one template? https://code.launchpad.net/~jcsackett/launchpad/mergeproposal-status-icon-589584/+merge/44531
#launchpad-reviews 2010-12-23
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: None || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: lunch-options || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> allenap: :-)
<jelmer> allenap: When you're done reviewing your lunch options, any chance I can add two more branches to your queue?
<bac> hi sinzui
<sinzui> hi bac
<bac> sinzui: could you have a look at the diff i just pasted to https://code.edge.launchpad.net/~bac/launchpad/bugjam-579502/+merge/44302
<bac> i'd like to send this off to ec2 soon
<sinzui> bac: I see you changed  _known_viewers() to ensure that owner have access
<bac> sinzui: yes, i wanted your feedback specifically on that approach.
<sinzui> bac: I see it always returns a set, but the doc string says dict
<bac> it seemed the least intrusive
<sinzui> I think we should fix the doc string
<bac> good catch!
<sinzui> bac: will that be the same spot to add drivers, and bug supervisors?
<bac> sinzui: with some careful review I think it might.
<sinzui> I think I will look at that method in january
<sinzui> bac; you are aware that the owner can only assign teams he is a member of to the bug supervisor role?
<bac> sinzui: i am but i don't see how that relates.  what am i missing?
<sinzui> Every believes the owner can private bugs. But that is not true since he can leave the team, he could change the team, but that new team will not be subscribed to the bugs...
<sinzui> Your change makes Lp work as users expect...even Canonical employees
<bac> ah, a subtly i missed
<bac> er subtlety
<bac> sinzui: so, landable?
<sinzui> yes please
<allenap> jelmer: Certainly :)
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: lunch-options || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: lunch-options, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, - || queue: [jelmer]*3  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> allenap, jcsackett, i have an launchpadlib branch coming up that's oversize, but mostly due to method renames
<allenap> leonardr: Okay, I can take a look at that.
<leonardr> will let you know once i write mp
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, jelmer || queue: [jelmer]*2  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: can you follow up on https://code.launchpad.net/~jelmer/launchpad/693757-py2.7-compat/+merge/44566 for me?
<bac> jcsackett: sure
<jcsackett> thanks.
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, - || queue: [jelmer]*2  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, - || queue: [ ]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> allenap: https://code.launchpad.net/~leonardr/launchpadlib/separate-login-for-cronjob/+merge/44592
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: leonardr, - || queue: [ ]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> leonardr: It says there's a conflict in src/launchpadlib/tests/test_launchpad.py
<henninge> jcsackett: Hi! I have to leave now but it would be great if you could look at my MP. It's fairly straight forward UI stuff.
<jcsackett> henninge: i can look at the code portion, but i'm not a ui reviewer.
* henninge changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: leonardr, - || queue: [henninge]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jcsackett: that's fine
<henninge> https://code.launchpad.net/~henninge/launchpad/devel-bugjamming-2/+merge/44590
<henninge> thank you!
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: leonardr, henninge || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> henninge: no problem.
<leonardr> allenap: hmmm, i just updated, let me check
<leonardr> ah, i updated but didn't merge
<allenap> leonardr: Cheers.
<leonardr> allenap: should be fixed now
* allenap changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: henninge || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: can you follow up on https://code.launchpad.net/~henninge/launchpad/devel-bugjamming-2/+merge/44590 for me, when you get a chance?
* jcsackett changed the topic of #launchpad-reviews to: On call: - || reviewing: henninge || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: ok
<jcsackett> thanks.
<bac> jcsackett: done
<jcsackett> bac, thanks. :-)
<jcsackett> although, on the MP, i see that sinzui is still doing reviews, so perhaps i can stop bothering you and pester him.
* sidnei changed the topic of #launchpad-reviews to: On call: - || reviewing: henninge || queue: [https://code.launchpad.net/~sidnei/lazr-js/static-resource-prefix/+merge/44271, https://code.launchpad.net/~sidnei/lazr.testing/yeti/+merge/44591]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.launchpad.net/~sidnei/lazr-js/static-resource-prefix/+merge/44271, https://code.launchpad.net/~sidnei/lazr.testing/yeti/+merge/44591]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> sidnei: it works better to get reviewers attention if you leave a message in the room with the MPs and put your name in the queue than just throwing the MPS in the queue in the channel info.
<jcsackett> i've got to run out to lunch, i'll be back here for reviews in a bit.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: late lunch || queue: [https://code.launchpad.net/~sidnei/lazr-js/static-resource-prefix/+merge/44271, https://code.launchpad.net/~sidnei/lazr.testing/yeti/+merge/44591]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sidnei> jcsackett, i  couldn't remember which way it was, thanks!
* sidnei changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: late lunch || queue: [sidnei]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: sidnei || queue: [sidnei]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: sidnei || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: i've done sidnei's first MP: https://code.launchpad.net/~sidnei/lazr-js/static-resource-prefix/+merge/44271. can you follow up?
* jcsackett changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
