#launchpad-reviews 2010-03-15
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: Hi! I only see one unreviewed propposal from you.
<wgrant> henninge: There's one which has two DB reviews but needs a code review, and one which is entirely unreviewed.
<wgrant> Actually, there are two of the former class, but the older one should be disregarded.
 * wgrant finds a link.
<henninge> wgrant: a, I just saw all the green lights
<henninge> ;-)
<wgrant> https://code.edge.launchpad.net/~wgrant/launchpad/bprdc
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: r=me
<henninge> on both accounts
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> henninge: Thanks.
<wgrant> bigjools: No final objections to https://code.edge.launchpad.net/~wgrant/launchpad/bprdc or https://code.edge.launchpad.net/~wgrant/launchpad/get-out-of-here-p-a-s?
<bigjools> wgrant: just looking
<bigjools> wgrant: I don't think pas.py belongs in model, but there's nowhere better either :/
<wgrant> bigjools: That was my thinking.
<bigjools> there's a smattering of stuff in adapters
<wgrant> Right, I looked there.
<wgrant> But it's barely more relevant,.
<bigjools> indeed
<wgrant> It is sort of model.
<wgrant> Just not DB model.
<bigjools> model means db model reallky
<wgrant> Shhh.
<bigjools> it could go under /soyuz
<wgrant> In the root of the app package, like it is in buildmaster?
<bigjools> yep
<wgrant> Hm, some other apps do do that.
<wgrant> Just not Soyuz.
<wgrant> Good idea.
<bigjools> that was my thinking
<wgrant> bigjools: Move pushing.
<wgrant> henninge: Can you please land those two?
<henninge> wgrant: has the prerequisite already filtered through to db-devel?
<wgrant> henninge: It landed Fridayish... let me see.
<wgrant> (landed on devel, that is)
<wgrant> henninge: Yeah, it's in db-devel.
<henninge> wgrant: cool, I'll land them
<wgrant> Thanks.
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> wgrant: having said that, can you please merge the current devel / db-devel respectively and push? I like to do that before landing. ;-)
<wgrant> henninge: Done.
<henninge> wgrant: cheers
<adiroiban> henninge: hi, Do you have time to review this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-359180-take-2/+merge/20122?
<adiroiban> I emailed Graham, and he told me he will not have time do review it and told me to ask the OCR
<henninge> adiroiban: put it on the queue, I or intellectronica will pick it up.
<henninge> s/on/in/
* adiroiban changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> henninge: Those are ec2ing happily?
<henninge> wgrant: sorry, got distracted. I'll start them right now.
<wgrant> henninge: Thanks.
<henninge> wgrant: the "ec2" command is failing for me "ImportError: No module named _pythonpath"
<henninge> any idea
<wgrant> henninge: The branch is unbuilt?
<henninge> wgrant: I thought it wasn't ...
 * henninge builds
<henninge> wgrant: ok, first one's on its way.
<henninge> wgrant: second one, too. Have a good night!
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: lunch, - || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> henninge: Thanks! Have a nice day.
<allenap> jml: Do you have some time this afternoon to finalise a review that Bjorn was doing last week? It's the isolate-tests branch that adapts subunit's IsolatedTestCase to work with zope.testing. I think it might just need a final nod; Bjorn and I went back-and-forth about it last week.
<allenap> https://code.edge.launchpad.net/~allenap/launchpad/isolate-tests/+merge/20916
<jml> allenap, I can't promise anything, but I'll definitely try.
<allenap> jml: Thank you! The comment where Bjorn abstains is probably the place to start if you're happy to skip the earlier conversation.
<jml> allenap, ta
<adiroiban> danilos: hi, Can I add you as a reviewer for this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 or should I as the OCR?
<danilos> adiroiban, I'd like to review it, so yes :)
<danilos> adiroiban, well, someone from translations should, you can OCR it if henninge is around, that's even better :)
<deryck> intellectronica, hi.  Could I get you to review a bug heat flames branch for me?
<adiroiban> danilos: ok. I will see if Henning has time to do it, otherwise I will assign the MP to you.
* henninge_ changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> Have you received the email for POTemplate details API pre-implementation review?
<danilos> adiroiban, thanks, and yes, I've received it, and will look into it
<henninge> adiroiban: I will look at it now.
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adiroiban, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge: ok. in the queue there was another MP
<henninge> adiroiban: oh, I did not see that.
<henninge> adiroiban: which one should I do first, then?
<adiroiban> the MP for bug 201749 needs to be reviewed by someone from the Rosetta team
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/201749>
<adiroiban> henninge: this one https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250
<adiroiban> :)
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adiroiban, - || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> gmb: Hi, Any news about the landing of this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-512307/+merge/20442 ?
<gmb> adiroiban: I think it vanised; I saw no emails about it and the EC2 instance shut down cleanly. I'll re-run it but this probably means there's a test failure somewhere.
<adiroiban> gmb: yes, there must be a failing tests on ec2. I have ran a full translation test suite on my computer and there were no errors.
<gmb> adiroiban: That's discouraging. Well, it's running now and I'm capturing the output, so hopefully we'll get some answers.
<allenap> intellectronica: Are you up for reviewing a branch that was once reviewed and approved by henninge but then underwent some big changes? https://code.edge.launchpad.net/~allenap/launchpad/twisted-threading-bug-491870/+merge/21376
<danilos> gmb, there were problems with ec2land some ~7 days ago or so, it required updating your download-cache and tree for the ec2 land to go headless and succeed :)
<gmb> danilos: This isn't an ec2 land problem, it's an ec2 test run that's just dying a death. Another run started at the same time succeed, however.
<intellectronica> allenap: sure
<allenap> intellectronica: Thank you :)
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adiroiban, allenap || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || 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, intellectronica, abentley || reviewing: adiroiban, allenap, - || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> allenap: fyi i have a ui call, now, so will only start reviewing your branch in ~1h. hope that's ok.
<allenap> intellectronica: That's great.
<deryck> intellectronica, I'll add mine to the queue, too.  https://code.edge.launchpad.net/~deryck/launchpad/bug-page-distro-max-heat-532099/+merge/21370
* deryck changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adiroiban, allenap, - || queue: [adiroiban(http://tinyurl.com/yex2mcx),deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> abentley, can you review https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-apidoc/+merge/21181 ?
<leonardr> i want gary to look at it, but he's busy. if you can do the review i can just get a quick ok from him afterwards
<abentley> leonardr, sure.
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adiroiban, allenap, leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx),deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> leonardr, why are you combining create-lp-wadl.py and apiindex.py ?
<leonardr> abentley: it's simpler for a number of reasons
<leonardr> for instance, generating index.html requires an ordered list of the web service versions
<leonardr> which is only available when launchpad is running
<leonardr> also, changing the makefile to call apiindex.py many times made it very messy
<abentley> leonardr, why are you printing out the raw HTML in apidoc.txt, rather than extracting the text?
<leonardr> abentley: no reason. if you tell me how to extract the text i'll change it
<abentley> leonardr, you call "extract_text(html)"
<leonardr> ok, changing it now
<leonardr> thanks
<abentley> leonardr, no problem.
<abentley> leonardr, you asked if there was a way to put apidoc-index.pt somewhere else.  I think you could do that by subclassing PageTemplate rather than using PageTemplateFile.
<leonardr> abentley, i don't see any examples in launchpad. do you know how to use it off the top of your head or is this something i should bring to gary?
 * leonardr is looking online now
<abentley> leonardr, that's an educated guess.
<leonardr> abentley: ok, i'll keep looking and maybe gary can help, that's the kind of thing i wanted to ask him
<gary_poster> It should be possible.  The question in my mind is effort to benefit.
<gary_poster> But Il'll look later and see if a quick way presents itself to me
<leonardr> gary: thanks
<intellectronica> allenap: r=me
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adiroiban, deryck, leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> intellectronica: Woohoo, thanks :)
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adiroiban, -, leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> leonardr, is "last_version_with_mutator_named_operations" meant to be authoritative?
<leonardr> abentley: yes, in the sense that the value of that setting controls the behavior of lazr.restful
<abentley> leonardr, i.e. is there a new policy against having mutators?
<leonardr> abentley: there's a policy against publishing named operations that do exactly the same thing as a mutator
<abentley> leonardr, where can I find that policy?
<leonardr> abentley: it's something we did by fiat, we didn't think it would cause problems. if you have concerns we can discuss it before i land the branch
<abentley> Okay, so by "exactly the same thing as a mutator", do you mean "exactly the same thing as an existing mutator does", or do you mean "exactly the same thing as a mutator would do, whether or not it exists"?
<leonardr> abentley: i mean that if you publish a named operation as a mutator, it is _only_ available as a mutator, not as a mutator + a named operation
<abentley> leonardr, I see.  That sounds reasonable.
<leonardr> ok, great
<abentley> leonardr, You removed the steps in lib/lp/bugs/stories/webservice/xx-bug.txt that set the bug to New/Low.  Have you verified that the tests which follow are still testing the right thing?
<leonardr> abentley: i think you're reviewing the precursor branch
<leonardr> which is fine, but it's already been reviewed. i put up a diff against the precursor branch in a comment
<leonardr> btw, yes, i have run that branch through ec2 successfully
<abentley> leonardr, I didn't see a huge difference between the diffs, except that the one for the merge proposal is colourized.
<leonardr> abentley: hmm
<leonardr> maybe i pasted a bad diff
<leonardr> abentley: try http://paste.ubuntu.com/395699/
<abentley> leonardr, I wasn't asking whether the tests pass, I was asking whether the tests were still testing the right thing.  For example, if a subsequent test had set the status to "fix committed", and then checked whether it was "fix committed", it would no longer be a good test.
<abentley> Because the starting value is already fix committed.
<leonardr> abentely: i'll double check, but i'm pretty sure they're testing the right thing
<abentley> leonardr, anyhow, I accept that they're out of scope.
<abentley> for this review, I mean.
<leonardr> abentley: i wasn't asked that in the last review so i think it's fair to double check
<abentley> leonardr, when you pastebin a diff, it's nice if you select its syntax as "Diff", so that it gets colourized like this: http://paste.ubuntu.com/395702/
<leonardr> abentley: ok, will do in the future
<abentley> leonardr, so aside from the extract_text thing, this looks fine.
<leonardr> abentley: ok, do you have any suggestions for fixing the ZTL to do a lookup like config/active_versions/$version?
<abentley> leonardr, I don't follow.
<leonardr> abentley: right now i have python code that creates a list of tuples versions_and_descriptions
<leonardr> in the ZTL i loop over that list to do something for every version
<leonardr> if there was some way in ZTL to look up the description for a given version, i wouldn't have to create that list of tuples
<leonardr> but while i can navigate a dictionary from ZTL, I have to know the dictionary key ahead of time
<leonardr> so i can do config/active_versions/beta, but not config/active_versions/$version
<leonardr> since i don't know much about ZTL i wonder if there's something i'm missing
<leonardr> abentley: anyway, that's another thing gary can weigh in on
<abentley> leonardr, if you can access config, you should be able to loop over config/active_versions in the ZTL, and access the descriptions as config/version_descriptions/key
<leonardr> abentley: that won't try to access version_descriptions['key']?
<abentley> leonardr, I don't think so.
<leonardr> ok, i'll try it
<abentley> leonardr, I am assuming 'key' is the repeat variable, of course.
<leonardr> right
<leonardr> abentley: unfortunately that doesn't work
<leonardr> KeyError: 'version'
<abentley> "version" is your repeat variable?
<leonardr> abentley: yes
<leonardr> see http://paste.ubuntu.com/395709/
<leonardr> i know i can do it with a python expression but i also know we're not supposed to do use those
<abentley> leonardr, maybe that needs to be config/version_descriptions/${version}
<leonardr> abentley: KeyError: '${version}'
<abentley> leonardr, sorry I guess I gave bad advice.  Quite surprised that's a problem.
<leonardr> abentley: np
* salgado changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adiroiban, -, leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx),salgado] || 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: intellectronica, abentley || reviewing: -, leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx),salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: -, leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx),salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: leonardr || queue: [adiroiban(http://tinyurl.com/yex2mcx),salgado] || 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: abentley || reviewing: salgado || queue: [adiroiban(http://tinyurl.com/yex2mcx),salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* james_w changed the topic of #launchpad-reviews to: on call: abentley || reviewing: salgado || queue: [adiroiban(http://tinyurl.com/yex2mcx),salgado,james_w(http://is.gd/aIvTS)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> A small branch that will make several people happy just added to the queue: https://code.edge.launchpad.net/~james-w/launchpad/merge-proposal-instructions/+merge/21401
<james_w> abentley: if you can squeeze it in before EOD ^
<rockstar> james_w, could you have the test expand out a bit more than just the 'bzr merge ...'  Maybe 'bzr merge bzr+ssh://~person-name.../fooix/...' or something?
<james_w> rockstar: doesn't the other test cover that?
<rockstar> james_w, I guess it does.  It uses sample data though, which I think is generally icky.
<james_w> it's just echoing what the rest of the test is doing
<rockstar> james_w, okay.  r=me
<james_w> thanks
<james_w> would you submit for me please?
<rockstar> james_w, okay.
<rockstar> abentley, is your queue really reflective of reality?
<abentley> rockstar, no, not really.
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [adiroiban(http://tinyurl.com/yex2mcx)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> james_w, sorry, was on the phone.
<rockstar> abentley, could you possibly do a quick review for me?
<rockstar> (I know we're approaching EOD for you, feel free to tell me "no")
<abentley> rockstar, I'll look at it.
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/branch-upgrade-copy-bzr/+merge/21399
<abentley> rockstar, what happened with db_branch_transport?
<rockstar> abentley, artifact of me messing with the test.  :/  Removed.
<abentley> rockstar, cool.  Actual changes look fine.  What's the problem with testing?
<rockstar> abentley, well, I'm still not entirely sure why the existing test didn't fail in the new way...  :/
<rockstar> i.e. it demonstrated that it fixed the old backup.bzr bug, but it really should have demonstrated the new one.
<rockstar> As it stands now, it looks like I just refactored a bit.
<abentley> rockstar, I think it's because your test isn't authentic enough; remember, you can create a new directory on top of an empty one, and your backup.bzr is an empty directory.
<rockstar> abentley, okay, so maybe I should copy .bzr to backup.bzr - How's that sound?
<abentley> rockstar, sounds good to me.
<rockstar> abentley, so, that seems to give me the error: ReadError: Error reading from '~person-name9/product-name4/branch11/.bzr'
<abentley> rockstar, copying does that?
<rockstar> Even though I can do source_branch_transport.list_dir('.') and it results in ['.bzr']
<rockstar> Yea, no idea what's going on.
<rockstar> source_branch_transport.copy('.bzr', 'backup.bzr')
<abentley> rockstar, I'm not sure that copy is meant to recursively copy trees.  In fact, experience suggests that it's not meant for that.
<rockstar> abentley, yeah, that's what I was just thinking.
<abentley> rockstar, perhaps you could try copy_tree_to_transport again?
<rockstar> abentley, yeah, lemme do that.
<rockstar> abentley, hooray!  It actually found the bug I really needed to squish.
<rockstar> abentley, here's the diff I'm committing: http://pastebin.ubuntu.com/395817/
<abentley> rockstar, lines 9-10 are "look before you leap".  Have you considered doing "easier to ask forgiveness than permission"?
<rockstar> abentley, I could wrap it in try/except.  If I don't check, a test will fail (currently)
<rockstar> I'm not sure try/except is really better than if in this case.
<abentley> rockstar, yes, EAFP is often implemented via try/except blocks.
<abentley> rockstar, it is a better habit to be into.
<rockstar> abentley, so catch the exception and pass?
<abentley> rockstar, right.
<rockstar> abentley, okay.  I've generally avoided just passing, but I'm okay with it if you are.
<abentley> rockstar, as long as you're handling a specific error, that's fine with me.
<rockstar> abentley, cool.
<rockstar> abentley, changes pushed.
<rockstar> ...and the diff shows the changes.
<abentley> rockstar, r=me
<rockstar> abentley, cheers sir, and thanks for the input.
<abentley> rockstar, no problem.
<mwhudson> anyone want to review a branch?
<mwhudson> it's pretty small
<mwhudson> https://code.edge.launchpad.net/~mwhudson/launchpad/back-off-failing-imports-bug-413637/+merge/21408
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [adiroiban(http://tinyurl.com/yex2mcx),adiroiban(bug-522188-take-2)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-03-16
* sidnei changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [adiroiban(http://tinyurl.com/yex2mcx),adiroiban(bug-522188-take-2),sidnei(http://bit.ly/bm7GX4),sidnei(http://bit.ly/b0Sg2e)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> stub, jml, BjornT: Gentle prod about https://code.edge.launchpad.net/~gmb/launchpad/bugwatchhistory-db-bug-538091/+merge/21262
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adiroiban || queue: [adiroiban(bug-522188-take-2),sidnei(http://bit.ly/bm7GX4),sidnei(http://bit.ly/b0Sg2e)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Hmm. This branch requires me to drink coffee.
 * gmb goes to brew some
* wgrant changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adiroiban || queue: [adiroiban(bug-522188-take-2),sidnei(http://bit.ly/bm7GX4),sidnei(http://bit.ly/b0Sg2e),wgrant] || 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: adiroiban || queue: [sidnei(http://bit.ly/bm7GX4),sidnei(http://bit.ly/b0Sg2e)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> gmb: I appear to have disappeared from the queue?
<gmb> wgrant: Argh, sorry. My bad.
<wgrant> np
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adiroiban || queue: [sidnei(http://bit.ly/bm7GX4),sidnei(http://bit.ly/b0Sg2e), wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> wgrant: You may find, over time, that I absolutely suck at IRC.
<wgrant> Heh.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: sidnei || queue: [sidnei(http://bit.ly/b0Sg2e), wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> gary_poster: I've asked for your review on https://code.edge.launchpad.net/~sidnei/lazr-js/newer-buildout/+merge/21417; I don't have the necessary knowledge to be confident in reviewing it. If you don't have time to take a look can you find someone who has the necessary knowledge and reassign it to them?
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: sidnei || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> gmb, ack.  not me either.  I'll reassign to mars.
<gmb> gary_poster: Should this one go to mars too? https://code.edge.launchpad.net/~sidnei/lazr-js/better-module-config-compression/+merge/21427
<gary_poster> gmb, yes
<gmb> thanks.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> wgrant: Is there a bug filed about the lp.registry.interfaces.person circular import?
<wgrant> gmb: I don't know. It happens in lots of places.
<gmb> wgrant: Okay. Please search for / file one against lp-registry and make that comment an XXX. We should track this down (not your problem, though).
<wgrant> gmb: Pushing.
<jtv> gmb: got a big one for you.
<gmb> jtv: Define "big"
<jtv> gmb: "within 10% of the review limit"
<gmb> wgrant: And with that, r=me. Want me to ec2 it for you?
<gmb> jtv: So between 800 and 880 lines?
<jtv> gmb: ...but not actually over the review limit.
<wgrant> gmb: Thanks. Please ec2.
<gmb> jtv: Ah, cool. Bring it on, then.
<gmb> wgrant: Running now.
<jtv> gmb: great, thanks.  https://code.edge.launchpad.net/~jtv/launchpad/bug-536797/+merge/21439
<wgrant> Excellent.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gmb, can i ask you to review a one-line change to an already reviewed branch?
<leonardr> branch: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-apidoc
<leonardr> change: http://paste.ubuntu.com/396114/
<leonardr> the point of the change is to make sure 'make clean' will clean up a situation where there was an error generating the wadl or apidoc
<gmb> leonardr: Looks fine; r=me.
<leonardr> great
<wgrant> leonardr: Why only during clean?
<wgrant> I've struck it several times when terminating a build, and make clean would be excessive for cleaning that up.
<gmb> jtv: In fact I'll go and grab some lunch now before starting your branch.
<wgrant> Is there any problem with removing it immediately before create-lp-wadl is run?
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> gmb: I can't stick around for too long myself
<gmb> jtv: No worries; I'll ask question in the review.
<jtv> thx
<adiroiban> henninge: hi, please let me know when you have some time to talk about the MP for bug 201749
<mup> Bug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/201749>
<henninge> adiroiban: I'd rather do that after lunch. Can you please fix as much stuff as possible in the branch and we can then discuss the remaining issue(s)?
<adiroiban> henninge: sure :)
<henninge> adiroiban: thanks ;-)
<danilos> adiroiban, henninge: I've also made the comment on the most important issue of whether should it behave like that; it's up to the two of you to decide on the proper behaviour, though
<leonardr> gmb, when you come back, https://code.edge.launchpad.net/~leonardr/lazr.restful/multi-part-etag/+merge/21398 would like to join your review queue
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch,jtv || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi gmb -- if you haven't started on jtv's review i'll take it
<gmb> bac: That'd be great, thanks.
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: leonardr,jtv || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> gmb, bac, I have a trivial one; can I add it to the queue?
<bac> sure
* salgado changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: leonardr,jtv || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> gmb, bac: could you please review https://code.edge.launchpad.net/~abentley/launchpad/multiple-series-model/+merge/21260 and https://code.edge.launchpad.net/~abentley/launchpad/fix-qa-ready/+merge/21392 ?
<gmb> abentley: Sure, stick 'em in the queue
* abentley changed the topic of #launchpad-reviews to:  on call: gmb,bac || reviewing: leonardr,jtv || queue: [salgado, abentley, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> leonardr: Your branch looks good (and the doctest was really well written, thank you). r=me
<leonardr> hurrah
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: salgado,jtv || queue: [abentley, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> jml: Gentle nudge re. https://code.edge.launchpad.net/~allenap/launchpad/isolate-tests/+merge/20916 (and a reminder that I'm completely happy if you're too busy and need me to ask someone else).
<jml> allenap, I'll take a look now.
<allenap> jml: Woohoo, thanks :)
<gmb> salgado: r=me
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: abentley,jtv || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> thanks gmb!
<allenap> jml: Thank you for the review. I'll try and turn it around asap.
<jml> allenap, np.
<allenap> jml: Do you have another 10-20 minutes available today? For checkwatches I've written an ensure_no_transaction() decorator to make sure we don't start doing slow things (like network calls) when there's a transaction open. It uses the raw db connection, so I wondered if you'd take a look and give me an early indication if it's going to be okay or not. Or if it's already been done somewhere.
<jml> allenap, I don't know of somewhere else that does that.
<allenap> jml: http://paste.ubuntu.com/396185/ (46 lines)
<jml> allenap, I'll take a look at the patch though.
<gmb> abentley: r=me on your multiple-series-model branch
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: abentley,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> gmb, thanks.
<gmb> abentley: ... as is your fix-qa-ready branch
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb takes a break for bugs standup; will work on the +activereviews queue after.
<bac> hi bigjools
<bigjools> hey
<adiroiban> gmb: hi, can you please try a new ec2-test on this branch lp:~adiroiban/launchpad/bug-512307 ? I have merged it with devel and fixed the failing tests.
<gmb> adiroiban: sure
<gmb> adiroiban: Running.
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb signs off to answer a shedload of emails
<leonardr> bac, can you add https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/test-long-etag/+merge/21455 to your queue?
<bac> leonardr: yes
<leonardr> great
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: your mp showed up as private.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> bac: oops, i'll fix
<leonardr> bac, you were able to review the branch anyway?
<bac> leonardr: yes.  just thought it odd that it was privte
<leonardr> bac: that is odd, i changed it
<rockstar> bac, hi, can I jump on your queue?
<bac> rockstar: sure
* rockstar changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-description/+merge/21479
<bac> rockstar: in your model, do you need to provide any extra params to StringCol() such as default or notNull?
<rockstar> bac, I don't think so.  TEXT is okay being blank I believe.  That's fine.
<bac> rockstar: we generally have all that goo in our classes...but i don't know if it is legacy
<bac> and the sqlobject wrapper in storm doesn't provide much guidance
<james_w> rockstar: hey, did my branch from yesterday disappear in to the ec2 black hole?
<rockstar> james_w, it appears so.  I have a script that kills ec2 instances when they run for too long.  Yours probably got killed that way (I always seem to have unlucky problems with ec2)
<james_w> damn
<james_w> rockstar: could you have another go, or should I ask someone else?
<rockstar> james_w, I'll shepherd it through.
<rockstar> james_w, sorry for my recent unresponsiveness.  I'm on the phone.  :)
<james_w> np
<NCommander> I'm repushing a branch that is proposed for merging to LP; do I need to resubmit a merge request? (I had to resolve a few conflicts)
<james_w> NCommander: the diff will update with your new changes
<NCommander> james_w: ah
<bac> sinzui: https://code.edge.launchpad.net/~bac/launchpad/productseries-js/+merge/21485 -- when you have some time
<sinzui> bac: thanks
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gary, care to review https://code.edge.launchpad.net/~leonardr/launchpadlib/529348-fix/+merge/21494 ?
<gary_poster> leonardr: on it
<gary_poster> leonardr: do I understand correctly that this will change nothing about the change needed now for 529348 in launchpad, because of legacy?
<leonardr> gary: yes, this is 1) to get the tests passing, and 2) to simplify the 529348 fix a little bit many years from now
<gary_poster> leonardr: right.  Getting the test to pass this way is expedient.  We actually have an explicit test to show the hack needed in launchpad.  Understood.  Approving branch.
<gary_poster> leonardr: I was looking at https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-apidoc/+merge/21181 .  I need to run, but I don't understand why you did the pt_getContext call and namespace dance rather than just call the page template with **kwargs as described in zope/pagetemplate/interfaces.py in the zope.pagetemplate egg.  Can talk tomorrow.
<leonardr> gary: i was copying code from lazr.restful
<gary_poster> leonardr: ah.  we should be able to simplify drastically
<gary_poster> talk to you tomorrow
<leonardr> ok, cool
<leonardr> gary: the code i'm working on now looks solid
#launchpad-reviews 2010-03-17
<stub> Need a trivial review - https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/21534
<thumper> stub: done
<thumper> simple move for someone... https://code.edge.launchpad.net/~thumper/launchpad/move-code-events/+merge/21535
<stub> Dun
<thumper> stub: ta
* henninge changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: whenever you start reviewing (or are you done?), the upload-from-slave branch is ready.
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [henninge,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge: cool, I'll do that one as soon as I'm free to!
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [henninge,wgrant,noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: whatever, but if allenap gets here first, he can pick it up, too.
<noodles775> Hey jtv, if/when you've time, could you take a look at https://code.edge.launchpad.net/~michael.nelson/launchpad/529926-partner-override-to-main/+merge/21329
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: henninge || queue: [wgrant,noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: that's the one on the queue?
<henninge> jtv: Pushing a new revision now that merges devel and resolves the conflict.
<jtv> noodles775: maybe we can do a trade...  I apparently need a UI review on the branch that makes the builder UI not oops out.  There really isn't much to look at, but setting up for QA is hard work and I could use the feedback on the documentation for that.
<noodles775> jtv: yep, I've just removed the conflict and re-pushed like henninge !
<noodles775> jtv: I'm sprinting for the next 3 days, but might get a chance to take a look.
<noodles775> jtv: ooh, that sounds really easy though...
<jtv> noodles775: whatânot the part where I said "hard work," I take it?
<henninge> jtv: is "test_dispatchBuildToSlave" your code?
<henninge> lp.translations.tests.test_translationtemplatesbuildbehavior.TestTranslationTemplatesBuildBehavior
<jtv> henninge: yes
<henninge> http://paste.ubuntu.com/396586/
<jtv> what's the problem?
<henninge> but I am doing another make schema now
<jtv> henninge: yes, that's a matter of doing a "make schema"
<henninge> jtv: have you seen this before?
<henninge> http://paste.ubuntu.com/396597/
<henninge> that's in the same output
<jtv> henninge: don't know what that is, but I'd try a rocketfuel-get ; make clean ; ./utilities/link-external-sourcecode ../{devel,trunk} ; make
<henninge> yeah, I am basically doing that ...
<henninge> jtv: yup, all good now. That means the branch you got in review is good, too.
<noodles775> jtv: so in terms of QA'ing, what I usually do for branches like that is use the factory/SoyuzTestPublisher to create any required data to display in the UI... that way you can just provide a pasted script for the reviewer to use.
<noodles775> s/QA'ing/demo-ing
<jtv> henninge: I know it's goodâit went through a full EC2 test.  :)
<henninge> mine didn't yet ...
<jtv> noodles775: I had a look at your demo, but it's very unclear to me (as a non-Soyuz and not-particularly-Soyuz inclined) person how much that would help me for this.  I don't need to do things like package uploads for this, though I do use codehosting.
<noodles775> jtv: couldn't you just use factory.makeSourcePackageRecipeBuild() in a harness?
<jtv> noodles775: true, that'd work for source-package recipe builds.  Good idea.  Then you'd just tell Bob to start work on that one.
<jtv> Or wait, maybe, depending on whether the buildd master will pick it up.
<jtv> (I don't know how far the factory goes in setting up credible data)
<noodles775> jtv: if you just set the builder attribute, and the state to BUILDING, it will display what you want in the UI (assuming no other builds are in that state... but you can manually unset those too).
<jtv> noodles775: that won't set Builder.currentbuild though, will it?
<jtv> noodles775: on a sidenote, AIUI a source package recipe build has an IBuildBase, so it'll present in the UI just like a Soyuz build.  Only new thing is that the icon is now part of the link formatter and so is next to the link instead of the sentence.
<noodles775> jtv: Right, looking at the code, you'd need to grab the associated queue item and set the builder there, so that Builder.currentjob returns your item.
<jtv> henninge: in doctests IIRC I've been doing something like builder.startBuild(buildqueue)
<noodles775> jtv: hangon, I'll take a closer look at your branch.
<jtv> cool, thanks
<henninge> jtv: talking to me? ;)
<jtv> henninge: no
<jtv> sorry
<noodles775> jtv: OK, so you've added a great story which sets up the required data... so a reviewer can just re-use that (or even easier, add a breakpoint and run the webapp using the test database).
<jtv> noodles775: a breakpoint in the doctest and then run the webapp against the test db?  Clever!  Do you do that by something like "env LPCONFIG=test make run"?
<noodles775> jtv: close: https://dev.launchpad.net/Debugging
* wgrant changed the topic of #launchpad-reviews to: on call: jtv || reviewing: henninge || queue: [wgrant,noodles775,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: on call: jtv || reviewing: henninge || queue: [wgrant,noodles775,wgrant,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> https://code.edge.launchpad.net/~stub/launchpad/oops-pruning/+merge/21536 is a trivial one
<stub> two imports, one new line, and shifting some existing code one indentation to the right
<jtv> henninge: review done
<henninge> jtv: thanks
* jtv changed the topic of #launchpad-reviews to: n call: jtv || reviewing: - || queue: [wgrant,noodles775,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: https://dev.launchpad.net/PythonStyleGuide#Multiline%20function%20calls
* adiroiban changed the topic of #launchpad-reviews to: n call: jtv || reviewing: - || queue: [wgrant,noodles775,wgrant,adiroiban(bug-540105)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue: [wgrant,noodles775,wgrant,adiroiban(bug-540105)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gary, care to do a review of https://code.edge.launchpad.net/~leonardr/launchpad/529348-fix/+merge/21508 ? the test suite passed with no problems
<gary_poster> leonardr: sure
<gary_poster> leonardr: line 179 of https://pastebin.canonical.com/29282/ is
<gary_poster> -            # XXX gary 2010-03-15 bug 539156
<mup> Bug #539156: remove exception for +blobstore without a REFERER <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/539156>
<gary_poster> I think that XXX needs to star
<gary_poster> stay
<gary_poster> leonardr: other than that, r=gary
<bac> jtv: ping
<StevenK> jtv: Why did you remove me from the queue?
<adeuring> gmb: you offered a review earlier today. Could you have a look here: https://code.edge.launchpad.net/~adeuring/launchpad/bug-267689-better-checkwatch-oopses/+merge/21558 ?
<gmb> adeuring: Sure.
<gmb> adeuring: Looks perfect; r=me
<adeuring> gmb: thanks!
<jtv> StevenK: did I!?
<jtv> StevenK: maybe our updates crossed.
<jtv> okay, I'll just have to accept that my main machine isn't going to come back to life today.  :-(
<leonardr> jtv, can i get your approval on this trivial diff? it just sets the release date for a new version:
<leonardr> http://paste.ubuntu.com/396730/
<jtv> leonardr: loading...
<jtv> leonardr: I'm sure that's alright.  :)
<leonardr> thanks
<jtv> so yes
<leonardr> fwiw i did check to make sure versions.txt said 0.9.24
<jtv> :)
<EdwinGrubbs> wgrant: I'm looking at your two merge proposals. Who is ScottK? Did you do a pre-impl call with someone from soyuz?
<james_w> EdwinGrubbs: ScottK is an Ubuntu developer, presumably who requested the feature
<EdwinGrubbs> thanks for the info
* james_w changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue: [wgrant,noodles775,wgrant,adiroiban(bug-540105),james_w(package-merge-proposal-permissions)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/package-merge-proposal-permissions/+merge/21561 is ready for review now if someone would be so kind
<james_w> it corrects a discrepancy in source package branches that will remove one of the warts that Ubuntu developers see
<jml> james_w, I'm on it.
<james_w> thanks
<EdwinGrubbs> noodles775: are you still around?
<EdwinGrubbs> adiroiban: ping
<adiroiban> EdwinGrubbs: hi
<jml> james_w, done.
<EdwinGrubbs> adiroiban: ok, I'll start on your merge proposal since you are the only one here.
<adiroiban> EdwinGrubbs: thanks :)
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: adiroiban(bug-540105) || queue: [wgrant,noodles775,wgrant,james_w(package-merge-proposal-permissions)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> EdwinGrubbs: hey, yep I am, just sprinting.
<EdwinGrubbs> ok, I'll take your mp next
<noodles775> No rush, but when you've time it'd be great to get a review :)
<noodles775> Thanks!
<james_w> jml: all fixed, thanks. diff generating now.
<james_w> jml: also, is there a handy method on IBranch to get the product/branch_name pair or the distro/series/sourcepackage/branch_name tuple as necessary?
<james_w> I could use unique_name, but the code I'm replacing doesn't use owner currently.
<jml> james_w, hmm.
<jml> james_w, not _really_, there's (branch.target.name, branch.name), I guess
 * jml double checks the IBranchTarget interface
<jml> james_w, yeah, that would work.
<james_w> what's name for a source package?
<jml> james_w, the question is a bit inexact. branch.target will always return an IBranchTarget. When it's wrapping a sourcepackage, it'll be distro/series/sourcepackage.
<james_w> yeah, that's what I meant :-)
<james_w> thanks
<jml> james_w, np.
<jml> james_w, when you are writing code that uses branch, you almost never need to deal with actual products or packages.
<james_w> I'm cleaning up ICodeImport to have less derived data, so that we can use IBranchTarget rather than IProduct with them
<jml> james_w, that'd be awesome
<james_w> disallowing +junk probably, but still...
<jml> james_w, IBranchNamespace and IBranchCollection are also your friends
<james_w> I have no friends, all around me are just interfaces, I never get to know the *real* person
<jml> james_w, you don't need to perform surgery on someone before you really know them
<EdwinGrubbs> adiroiban: I'm trying to run "./bin/test -t pofile_new_translation_autoselect"    What am I doing wrong?
<adiroiban> EdwinGrubbs: nothing... I'm just stupid. I fogot to add the new testfile. I'm pushing it now
<adiroiban> EdwinGrubbs: test pushed. can you please pull the branch and try it again
<EdwinGrubbs> sure
<leonardr> gary, https://code.edge.launchpad.net/~leonardr/launchpad/multi-part-etag/+merge/21573 is ready for your review
<gary_poster> great, looking
<gary_poster> r=gary leonardr
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing:  -|| queue: [wgrant,noodles775,wgrant,james_w(package-merge-proposal-permissions)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-03-18
* StevenK changed the topic of #launchpad-reviews to: on call: - || reviewing:  -|| queue: [wgrant,noodles775,wgrant,james_w(package-merge-proposal-permissions),StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: on call: - || reviewing:  -|| queue: [wgrant,noodles775,wgrant,james_w(package-merge-proposal-permissions),StevenK,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: [wgrant,noodles775,wgrant,james_w(package-merge-proposal-permissions),StevenK,jelmer,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> sinzui: thanks for your review.  for some reason i didn't get a copy via email so i just now saw it.  bothersome.
<sinzui> bac: I used the UI for my review.
 * sinzui checks his own mail
<bac> sinzui: but an email still should've been sent
<sinzui> yes, and I did get a copy of my review
<adiroiban> EdwinGrubbs: when you have some time, can you please review the latest changes from here https://code.edge.launchpad.net/~adiroiban/launchpad/bug-540105/+merge/21552 ? Thanks!
<EdwinGrubbs> ok
<rockstar> Holy crap, is that seriously the queue?
* rockstar changed the topic of #launchpad-reviews to: rockstar are reviewin' - wgrant - [noodles775,wgrant,james_w(package-merge-proposal-permissions),StevenK,jelmer,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> james_w, I know what's going wrong with your ec2...  I figured it out last night.
<james_w> ORLY?
<rockstar> james_w, basically, I was running it with my personal key credentials which kill all instances after two hours.  I should be using my Canonical credentials.
<james_w> ah
<rockstar> Was a backup restore issue...  Running new ec2 instances of a few things now, one of which is yours.
<james_w> thanks
<salgado> rockstar, any chance I can jump the queue with my one-liner: https://code.launchpad.net/~salgado/launchpad/bug-540697/+merge/21648 ?
<salgado> thanks for the review, rockstar!
<rockstar> salgado, no problem.
<noodles775> Hey rockstar, if/when you've time, the branch referenced in the queue is this one: https://code.edge.launchpad.net/~michael.nelson/launchpad/529926-partner-override-to-main/+merge/21329
<rockstar> noodles775, yeah, I've just been looking through +activereviews and am working through them.
<rockstar> noodles775, did you not take a shift at reviewing today?
<noodles775> rockstar: no, soyuz team is sprinting this week.
<noodles775> rockstar: so I've just been doing swaps :)
<rockstar> noodles775, okay, that makes sense then.  Was wondering why the queue was so monstrous.  :)
* rockstar changed the topic of #launchpad-reviews to: rockstar are reviewin' - noodles775 - [wgrant,james_w(package-merge-proposal-permissions),StevenK,jelmer,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: rockstar are reviewin' - james_w - [StevenK,jelmer,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> james_w, ping
<james_w> hi rockstar
<rockstar> james_w, where's your mp?
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/package-merge-proposal-permissions
<rockstar> james_w, ah, jml has already reviewed it.
* rockstar changed the topic of #launchpad-reviews to: rockstar are reviewin' - LUNCHING - [StevenK,jelmer,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> rockstar: but not with my fixes
<jml> james_w, sorry, I'll get to it.
<james_w> thanks jml
<james_w> should be straightforward, I just did as you told me
<noodles785> Hi rockstar, I see that you processed my MP in some way, but the MP hasn't been updated?
<noodles785> (sorry, I mean I saw in the topic that you processed my MP in some way)
<rockstar> noodles785, bac had already looked at that
<noodles785> rockstar: he just noted there was a conflict, he didn't go any further (see my comment on the MP).
<rockstar> noodles785, ah, okay.  I saw the review and thought "hands off"
<noodles785> Thanks rockstar !
<rockstar> noodles785, just as I thought I was clearing the queue...
<noodles775> rockstar: it's much shorter than when you started :)
<rockstar> noodles775, :)
<EdwinGrubbs> adiroiban: I'll land your branch.
<adiroiban> EdwinGrubbs: thanks! :)
<rockstar> noodles775, your branch, I am approving it.
<jml> james_w, in case you missed it, I got to it.
<jml> james_w, do you need me to land it for you?
<james_w> yes please
<jml> james_w, (would you like commit access?)
<james_w> not really, but I do have a backlog building up :-)
<noodles775> Thanks rockstar
* rockstar changed the topic of #launchpad-reviews to: rockstar are reviewin' - jelmer - [jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> trivial code move review for someone: https://code.edge.launchpad.net/~thumper/launchpad/move-mailout-functions/+merge/21689
<mwhudson> thumper: we try to avoid code in __init__.py i thought, did you think about lp.services.mail.helpers or something?
<thumper> mwhudson: hmm... could do
<thumper> mwhudson: I notice there are many helpers in sendmail.py
<thumper> like format_address
<mwhudson> thumper: mmm, indeed
<mwhudson> thumper: well, fix as much of this as you like i guess :-)
<thumper> :)
<thumper> although I was just thinking
<thumper> the text_delta isn't explicitly mail oriented
<thumper> perhaps it should go somewhere else
<thumper> we could move the append_footer into sendmail.py
<thumper> hmm
<thumper> ObjectDelta and snapshot have been moved into lazr.lifecycle
<mwhudson> thumper: it's hard to find a place for sort of miscellaneous useful stuff in our tree
<mwhudson> i think there's lp.services.util or something like that
<thumper> ah you're back
<thumper> yeah, I've moved append_footer into sendmail.py
<thumper> and text_delta into lp.services.utils
<thumper> pushing now
<thumper> mwhudson: diff updated
<wgrant> rockstar: Thanks for the reviews. I've fixed the issue you mentioned -- can you please land them both?
#launchpad-reviews 2010-03-19
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/bmp-notification-email-job/+merge/21703
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/new-reviewer-email-job/+merge/21610
<thumper> I wish I could edit a merge proposal to add or remove a prerequisite branch
 * thumper adds it to the todo list
<mwhudson> change target branch too pls
<thumper> mwhudson: yeah that's gunna be tone too
<thumper> need it if we are going to automagically create WIP merge proposals
<jtv> rockstar: got time for mine as well?  Very short fix.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: hi, I have a branch in the queue if you can take a look please?
<adeuring> bugjsure
<adeuring> bigjools: sure
<intellectronica> adeuring: if it's ok i'll add myself to the queue too?
<adeuring> intellectronica: sure
<intellectronica> thanks
* intellectronica changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: -|| queue: [intellectronica] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> bigjools: r=me
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: intellectronica || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv-sick> adeuring: I've got a small one
<jtv-sick> could you take a look at it?
<adeuring> jtv-sick: of course -- just put it in the queue
* jtv-sick changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: intellectronica || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv-sick> thanks
<bigjools> adeuring: thanks for the review
<adeuring> intellectronica: what is the purpose of the variable 'that'? Can't we use 'this'?
<intellectronica> adeuring: no, we can't use 'this' because 'this' is a magic keyword in javascript. whereas in python you pass self explicitly, in js you don't and this always binds to the current instance
<intellectronica> adeuring: 'that' is used to capture the 'this' in the environment where the closure is defined, so that we can refer to it later.
<adeuring> intellectronica: Ahhh, right! But the variable name is sightly confusing ;) Coud you rename it to something more meaningful?
<intellectronica> adeuring: actually, using 'that' is kind of an idiom. we use it a lot in our js code.
<adeuring> really? OK, let's leave it as it is then...
<intellectronica> i can change it, but i think it's better to keep it like that. when i see 'that' in js i immediately know what's it about (and now you will too :) )
<intellectronica> cool
<adeuring> though the idiom will become really confusing when you have nesting. "var that = this;{ [...] var something_else = this; {... var something_completely_differet = this ; }}
<intellectronica> i think that's quite rare. nesting more than one closure can make the code very hard to read for many other reasons too
<intellectronica> if i got to the point where i needed such deep nesting i'd probably re-model it using proper objects
<adeuring> intellectronica: r=me, but there are a few lint issues
<intellectronica> adeuring: thanks, and will run lint and fix.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: any chance you could ui-review my change? it's quite simple.
<jml> could I get a review for https://code.edge.launchpad.net/~jml/launchpad/twisted-services/+merge/21716 please?
<adeuring> jml: sure, once I finished the review for Jeroen
<jml> adeuring, thanks.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jml: r=me
<jml> adeuring, thanks.
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: sorry for the delay... sprinting, but yeah, I can take a look soon.
<intellectronica> noodles775: didn't realise you're sprinting. i'll invite you to the mp and add screenies. if you can't look at it today then no big deal.
<noodles775> intellectronica: thanks.
<adiroiban> henninge: hi, when you have some time, can you please add your comments regarding the latest changes for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250? Thanks. I'll go to lunch now.
* salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adeuring, can you review a trivial one for me?
<salgado> https://code.launchpad.net/~salgado/launchpad/remove-crap/+merge/21724
<adeuring> salgado: sure
<adeuring> salgado: r=me
<salgado> thanks adeuring
<NCommand1r> stub: if your around, and have a moment, I'd love if you could review my raw source changelog branch on LP
<abentley> adeuring, salgado: could one of you please review https://code.edge.launchpad.net/~abentley/launchpad/recipe-index/+merge/21730 ?
* abentley changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> abentley, sure, I'll take it
<abentley> salgado, thanks.
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: salgado || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> Hi, salgado.  I have a branch ready and would like to get in the queue.
* deryck changed the topic of #launchpad-reviews to: on call: salgado || reviewing: abentley || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> deryck, sure, I'll take it once I'm done with abentley's
<deryck> salgado, thanks.  See:https://code.edge.launchpad.net/~deryck/launchpad/has-bug-heat-interface-529846/+merge/21731
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: deryck || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* 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
