#launchpad-reviews 2010-05-03
<thumper> mwhudson: care to take a look at a change for me?
<thumper> mwhudson: it is in resposne to BjornT's comments on the job runner oops tests
<thumper> mwhudson: I'd feel happier if someone else took a look at the changes before I submit
<mwhudson> thumper: ok
<thumper> mwhudson: I've replied to the original review with a diff, which you should get
<thumper> RSN
<thumper> hmm, a spike in scanner load?
<thumper> my branch isn't being scanned
<thumper> heh
<mwhudson> it's the distro branches
<thumper> yeah
<thumper> mwhudson: you should see the email now
<mwhudson> yep
<thumper> mwhudson: what oops related apis?
<mwhudson> thumper: getLastOopsReport, maybe?
<thumper> mwhudson: it is still used
<mwhudson> oh ok
<jtv> mwhudson, are you free for a tiny but urgent review?  https://code.launchpad.net/~jtv/launchpad/bug-572497/+merge/24514
<jtv> Hmm... why isn't that diff updated?
<mwhudson> jtv: the diff isn't updated yet, probably because the scanner is behind with all the new distro branches
<jtv> mwhudson: all I did was make the unit tests that exercise the failing code switch to the right db user.  Maybe the diffs should be updated SJF.  :-)
<mwhudson> jtv: well it generally sounds fine to me
<mwhudson> (apart from 'fiera' being a terrible username)
<jtv> Yes, but it made me use the config instead of a hard-coded name.  :)
<jtv> mwhudson: at last, my diff has updated!
<mwhudson> jtv: approved
<jtv> mwhudson: ta
<noodles775> intellectronica: /w 4
<noodles775> hrm... sorry intellectronica, I meant to say, very simple RC MP if you've got a minute: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-deletion-oops/+merge/24580
<noodles775> Or anyone else who fancies a simple review ^^ :)
<thumper> noodles775: You'll probably find the diff takes many hours to generate as the scanner tries to catch up with 36k branch scans
<thumper> due to mavrick
<thumper> noodles775: I suggest pastebin
<noodles775> thumper: thanks, doing so now.
<thumper> please pass the information on to the europeans
<thumper> I expect the scanner to catch up sometime in the next day or two :-|
<thumper> good thing we've fixed this for next time
* noodles775 changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanner issue ** On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* thumper changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> And here's mine: http://pastebin.ubuntu.com/426871/
<thumper> noodles775: I'd strip the _bug_574246 from the method name
<thumper> noodles775: but other than that r=me
<noodles775> thumper: Done.. thanks :)
<noodles775> BjornT: when you've time, a simple RC: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-deletion-oops/+merge/24580
<BjornT> noodles775: approved
<noodles775> BjornT: Ta.
<thumper> jml: ping
* noodles775 changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: - || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to:  **Paste your diffs for review due to scanning 18k mavrick branches ** On call: abentley || reviewing: - || queue: [noodles] || 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/lazr.restfulclient/delete-entry/+merge/24421 ? diff is in mp
<abentley> leonardr, okay.
* abentley changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: abentley || reviewing: noodles || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> leonardr, what happened with your Vary header branch?
<leonardr> abentley: i was able to land it on thursday
<abentley> Cool.  I'm eager to see how my lp clients perform with that changed.
<abentley> noodles775, your branch looks okay on its own merits, but it is part of an effort that I am concerned may duplicate or contradict our own efforts.
<noodles775> abentley: ok, good to know... in what way?
<abentley> noodles775, well, I want to have a system that provides an overview of jobs that are running, have run, etc.  I termed it the "Job console".
<noodles775> Aha (btw: we're not getting rid of the link to services.job with this work, contrary to the diagrams).
<abentley> This seems to be basically the same goal as https://dev.launchpad.net/LEP/GeneralBuildHistories
<abentley> I tried to start an LEP for it a couple of  months ago, but jml decided we don't need that feature yet.
<abentley> I am glad that the link to Job hasn't been severed.
<noodles775> abentley: I think it might be different... this is just ensuring that in the context of a PPA, we can show different types of builds (binary, recipe etc.)
<noodles775> There may be convergence in the future though... currently what I see happening after this current work:
<noodles775> 1) getting rid of our special tables that link 'builds' to services jobs and instead just having a reference to the job directly from BuildFarmJob, and then:
<abentley> noodles775, the LEP discusses using the context of the builders
<noodles775> 2) eventually merging as much of BuildFarmJob off (as it duplicates what you've already got on Job).
<noodles775> abentley: yes, sorry, and teh buildmaster builders context.
<abentley> noodles775, But we already have a display in the context of the builders, and I'm working right now on a home page for individual sourcepackagerecipe builds.
<noodles775> (but (1) and (2) are just speculation on my part atm., the current focus is just to enable general builds to display in those contexts).
<noodles775> abentley: a display that presents both binary and recipe builds batched together?
<abentley> noodles775, yes.  AFAICT, It Just Works.
<noodles775> Where can I see it?
<abentley> noodles775, dogfood.
<noodles775> abentley: hrm, which branch should I merge in?
<abentley> I don't think you need to merge anything in.  When I started creating sourcepackagerecipe builds, they just started showing up in that listing.
<noodles775> abentley: I expected them to show up in this listing: https://edge.launchpad.net/builders
<noodles775> but am surprised that they've shown up in the build history: https://edge.launchpad.net/builders/rothera/+history
<noodles775> as it currently is a list of Soyuz binary package builds afaik.
<abentley> noodles775, it seems like the results of my earlier experiments on dogfood have been destroyed, so I can't be certain either way, but that's what I recall.
<noodles775> abentley: the results will still be there, I just had to disable the config as per email, because it was expecting a SourcePackageRecipeNavigation class which isn't present in db-devel?
<noodles775> (which is why I assumed a branch needed to be merged in that includes that class?)
<abentley> noodles775, Yes, Julian had merged in my recipe-build-index branch.
<noodles775> (it could just be that it was renamed to SourcePackageRecipeNavigationMenu and the config on dogfood hadn't been updated?)
<noodles775> Ah.
<noodles775> abentley: if you look at lib/lp/soyuz/browser/builder:BuilderHistoryView and follow it, you'll end up at IBuilder.getBuildRecords() which currently returns IBinaryPackageBuildSet.getBuildsForBuilder() (ie. only soyuz binary builds), so I'm guessing what you saw was just https://edge.launchpad.net/builders displaying your recipe builds (which I'd hoped Just Worked and am glad it did :) ).
<abentley> Okay.
<abentley> So you suggested that my Job Console and your General Build History are different because the build history is filtered by PPA or Builder context.
<abentley> I think that the filtering isn't terribly important.
<abentley> I have no problem imagining filtering the Job Console per builder or per archive.
<abentley> The Job Console meant to be filterable by branch, and General Build History would fall down a bit there, because it would show only Jobs that were also Builds.
<abentley> noodles775, so I see the Job Console as a more general idea.  I would hate for us to end up with duplicate systems: a Job Console and a "Job Console for Builds".
<noodles775> abentley: no, I was thinking they were different because currently (in devel/db-devel), builds have nothing to do with service jobs. They are only temporarily related while they are queued.
<noodles775> The generalised build history doesn't change that, it is just refactoring all the common info on the different build classes, so that a builder .... actually, do you mind calling? It might be easier to find out where the convergence might be?
<abentley> noodles775, sure.
<abentley> noodles775, did you mean Skype?
<noodles775> abentley: yep, just call when ready.
<abentley> noodles775, I am ready, but I don't see you online.
<noodles775> Skype lag ;)
<noodles775> abentley: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-build-generalisation-db-changes/+merge/22527
<abentley> noodles775, could distro_series go on IPackageBuild instead of IBinaryPackageBuildView?
<noodles775> abentley: it's based on the db-column, BinaryPacakgeBuild.distro_arch_series?
<abentley> noodles775, it's a field that SourcePackageRecipeBuild and BinaryPackageBuild will have in common.
<abentley> noodles775, of course, the implementation on SourcePackageRecipeBuild will not be based on distro_arch_series.
<noodles775> Well, SPRecipeBuild has a db-column for distro_series, where as BinaryPackageBuild has a db-column for distro_arch_series.
<noodles775> Yeah.
<noodles775> We looked at adding distro_series to PackageBuild, but couldn't see how to do it without redundant info for a binary package build (I was chatting with Julian about it the other day).
<noodles775> We'd thought we could instead store something on BPB that just represented "architecture", but that didn't seem right either (as it would have to be a text arch_tag, afaics).
<abentley> noodles775, this seems to be a similar problem to the IPackageBuildDerived ones.
<abentley> noodles775, basically, it's interfaces being limited by the implementation.
<abentley> noodles775, diff lines 342-343 need to be re-indented.
<noodles775> abentley: Right. So do you mean that we should provide IPackageBuild.distro_series (even if it raises NotImplementedError), and just have implementations on BPB/SPRecipeBuild?
<abentley> noodles775, I think that would make sense.
<noodles775> Great, will do (you'll update the MP with all this right?)
 * noodles775 has to run out the door.
<abentley> noodles775, Okay.
<abentley> leonardr, what does the "lp" in lp_delete stand for?
<leonardr> abentley: shamefully, it stands for 'launchpad'
<leonardr> or rather, 'launchpadlib'
<leonardr> it's a naming convention used to distinguish lazr.restfulclient (formerly launchpadlib) methods from the named operations defined by the web service
<abentley> leonardr, I see.  If it's already established  as a convention, I'll let that be.
<leonardr> abentley: i wish you could tell me to change it and i'd be able to change it, but it's too entrenched
<abentley> leonardr, r=me
<leonardr> cool
<leonardr> i've got a wadllib branch for you in a minute
* abentley changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> can i get someone to review my and james's wadllib branch?
<leonardr> https://code.edge.launchpad.net/~leonardr/wadllib/fix-bind-to-anon-collection/+merge/24613
<leonardr> gary, maybe you'll take it?
<gary_poster> leonardr: On lunch now, and a bit swamped when I get back.  Maybe I'll get a chance to look at it later today, but if you can get someone else to do it that would probably be better for you.
<leonardr> gary, ok
<leonardr> bac: can you do a quick review for me?
<leonardr> or maybe barry or jml?
<leonardr> https://code.edge.launchpad.net/~leonardr/wadllib/fix-bind-to-anon-collection
<sinzui> flacoste, I have an RC request. BjornT agreed it was RCable. bac reviewed the change
<flacoste> sinzui: merge proposal?
<sinzui> flacoste, https://code.edge.launchpad.net/~sinzui/launchpad/enable-project-suggetions/+merge/24608
<bac> flacoste: i have an RC request too.  i believe sinzui already ran it by bjorn and sinzui has reviewed it: https://code.edge.launchpad.net/~bac/launchpad/bug-574142/+merge/24625
<flacoste> sinzui, bac: unless you already know ran it through EC2, it will have to be cherry-picked though
<bac> flacoste: it has not been through ec2
<sinzui> bac: flacoste: I did discuss the bug with BjornT The oopses over the weekend were caused by Edwin's landing on Friday that wrongly reverted the ProductSeries branch link
<flacoste> sinzui: is your branch passing all tests?
<flacoste> now, i mean
<sinzui> flacoste, yes mine does
<flacoste> sinzui: ok, land it now
<flacoste> bac: your's will have to wait tomorrow
#launchpad-reviews 2010-05-04
* bac changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> henninge: you reviewing today, nominal as it is?
<henninge> bac: oops, forgot ;)
<bac> henninge: i suspect its a quiet day
<bac> interrupted by panic
<henninge> ;-)
* henninge changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: bac, henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<matsubara> bac, hi, available for a review?
<bac> matsubara: yes, of course
<matsubara> bac, https://code.edge.launchpad.net/~matsubara/launchpad/bug-553361-no-referrer/+merge/24506
<bac> matsubara: r=bac, with one tiny fix
<matsubara> thanks bac
<leonardr> bac, henninge: i have a wadllib branch (based on james_w code) that didn't get reviewed yesterday. can you take it? https://code.edge.launchpad.net/~leonardr/wadllib/fix-bind-to-anon-collection/+merge/24613
<henninge> bac: can you take that please? I am in the middle of something else.
 * henninge guesses that bac is in the middle of something, too ... ;)
<leonardr> there's no huge rush, but i would like to get it reviewed today
<henninge> leonardr: I will give it a try later ;)
<noodles775> abentley: hi! I've sent two incrementals to the branch you reviewed for me yesterday... but understand if you don't have time atm :)
<abentley> noodles775, I'm having a look at one of them right now.
<bac> henninge, leonardr: sorry i was on our stand up call.  leonardr i can review your branch now.
<leonardr> bac, thanks
<noodles775> abentley: great, thanks! Just note: I realised with the second one that I don't need the Mixin that I created on the first.
<abentley> noodles775, ah, okay.
<noodles775> Hi bac, I've got one here if you've time, but I won't be around to do the review interactively: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-2/+merge/24688
<bac> noodles775: ok
* bac changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: bac, henninge || reviewing: leonardr || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> thanks bac  (or henninge) :)
<bac> leonardr-afk: done
* bac changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: bac, henninge || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: bac, henninge || reviewing: noodles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-05-06
* EdwinGrubbs changed the topic of #launchpad-reviews to: **Paste your diffs for review due to scanning 18k mavrick branches ** On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [wgrant] || 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 || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> noodles775: Thanks.
<noodles775> Always a pleasure wgrant :)
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: bryce || queue: [] || 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: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar, thanks!
<rockstar> abentley, no problem sir.
<mars> rockstar, ping, have room for a quick review?  a dozen new lines of code, some refactoring of the windmill test layer setup: https://code.edge.launchpad.net/~mars/launchpad/turn-on-windmill-debug-logging/+merge/24839
<rockstar> mars, please add yourself to the queue.
<mars> !take_ticket
<mars> ;)
<rockstar> mars, every time I'm on call reviewer, I always think that we need a bot to handle the queue...  I just never actually get around to making one.
<mars> likewise
* mars changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: abentley || queue: [mars] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> rockstar, I'm off to lunch, back in an hour and a bit.
<rockstar> abentley, the UI looks good for the SPRB.  Is there a way I can walk through actually building a source package recipe, or do I just have to mess with the database?
<abentley> rockstar, you just have to mess with the db.
<rockstar> abentley, okay.
<rockstar> abentley, So I guess that means that I won't have a build log I can access from +files then?
<abentley> rockstar, I just create an LFA manually.
<rockstar> abentley, your indentation on lines 428-431 of the diff is wonky.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: mars || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> mars, does reset_logging undo all of this logger buggery you're doing here?
<mars> rockstar, it does
<mars> rockstar, BaseLayer calls it as well, but I did this because it is more visible and explicit
<rockstar> mars, what does safe_hasattr do?
<mars> rockstar, doesn't eat exceptions.  Builtin hasattr does (fun fun!)
<rockstar> mars, why not just use getattr?
<mars> rockstar, what line?
<mars> 96
<rockstar> mars, yes.
<mars> rockstar, it might be because hasattr will tell you that an attribute exists.  getattr may return None, but you can't tell if that is the attribute's value, or the default argument.
<mars> or the default argument to getattr()
<rockstar> mars, okay.  As long as you feel you're using the right call here, I'm happy with that.
<bac> rockstar: can i add one to your queue?
<rockstar> bac, absolutely.
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-574493/+merge/24853
<mars> rockstar, not my code, so it should work :)
<mars> rockstar, that is one of the code blocks I extracted into its own method
<mars> personal choice: I dislike 300 and 400 line method definitions, even if they are linear.
<mars> if there are comments that say "# now we do this" followed by "# then we do that", then it is pretty good sign that you can extract a method
<mars> but, like I said, that's personal preference
<rockstar> mars, I think you've made the code clearer by breaking it up.
<mars> cool, thanks :)
<rockstar> mars, I'd like to do your review and then move on to bac's, but launchpad is apparently down right now.
<mars> :(
<mars> rockstar, page is back up.  Do I have r=rockstar?
<rockstar> mars, indeed you did as of a few minutes ago.
<mars> rockstar, cool, thank you
<rockstar> bac, is this a private branch?
<bac> rockstar: it is
<abentley> rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/no-default-powerpc/+merge/24855 ?
<rockstar> abentley, sure.
* abentley changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: mars || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> abentley, I'm going to eat lunch, then I'll get to your branch immediately following.
<abentley> rockstar, sure, no rush.  Thanks for your other review.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: LUNCH || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> rockstar, dumb question: ec2 land does run the test suite before submitting to PQM, right?
<mars> bac, ^ ?
<bac> yes
<mars> bac, ok, thanks.  Updating the docs: if you can believe it, no one actually bothered to mention that fact, anywhere.
<bac> really?  since it evolved out of 'ec2 test' i guess it was just assumed
<mars> 'ec2 help land' doesn't mention it.  I just updated LandingChanges so it does say that.
<abentley> mars, however, if you want to land without testing, "bzr lp-land" from the bzr-pqm plugin will do that for you.
<mars> abentley, yep, thanks.  I've used that before, and it is very convenient.
<abentley> mars, cool.  It's basically the same code.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> abentley, had I known it was that small, I would have done that before lunch!  :)
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar, :-)
<abentley> I prefer my rockstars happy and well-fed.
<abentley> rockstar, chat soon?
<rockstar> abentley, well-fed == sleepy  :)
<rockstar> abentley, sure, lemme send this email or thumper will lynch me.
<abentley> rockstar, okie.
<jtv> barry: see the update for the lazr.batchnavigator review?  https://code.launchpad.net/~jtv/lazr.batchnavigator/bug-574159/+merge/24577
* rockstar 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-05-07
* 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
<henninge> adeuring: Moin!
<henninge> adeuring: I got onen for you if you llike ;-)
<adeuring> henninge: sure
<adeuring> henninge: this one: https://code.edge.launchpad.net/~henninge/launchpad/bug-568355-package-name/+merge/24844 ?
<henninge> adeuring: yes, sorry. ;)
<henninge> I was a still looking through it.
<henninge> adeuring: that's the one
<henninge> danke
 * henninge needs to reboot
<henninge> adeuring: any questions so far?
<adeuring> EdwinGrubbs: not yet ;)
<henninge> I will be relocating now, back in 20 min.
<henninge> adeuring: and my name is henninge ;)
<adeuring> henninge. EdwinGrubbs sorry
<jtv> adeuring: mind finishing a small lazr.batchnavigator review?  https://code.edge.launchpad.net/~jtv/lazr.batchnavigator/bug-574159/+merge/24577
<bryceh> would anyone be willing to review my branch at lp:~bryceharrington/launchpad/api-doc-fixes ?  This is my first launchpad merge proposal so apologies if I'm not doing something right
<adeuring> jtv: sure; let me first finish a review for hennig
<adeuring> bryceh: I can do it, after henning's and jtv's branches
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: henninge || queue: [jtv, bryceh] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adeuring: thx
<bryceh> adeuring, ok thanks.
<bryceh> adeuring, bedtime for me, I'll check back in the morning.
<adeuring> henninge: lines 51-58 of the diff change the order of locations. The doc string of the method says "This function goes through the ordered list of these possible locations, the order having been copied from intltool-update". My question: Was the old order plain wrong or did the intltools chnage. IOW: Is the new ordering always correct?
<adeuring> or can it be wrong for some versions of intltools?
<henninge> adeuring: the ordering is correct, the comment is not valid any more.
<adeuring> henninge: so, the old ordering was simply based on a misinterpretation of what intltools are doing?
<henninge> adeuring: no, the original order was taken from intltool-update
<henninge> adeuring: basically, what is says "use GETTEXT_PACKGE from Makefile.in.in or else from configure.ac or else from configure.in"
<adeuring> henninge: ok... but the substitution order changed, right?
<henninge> adeuring: this is now reflected in the "keep_trying" value.
<adeuring> henninge: ah, I see. So, can you update the comment? otherwise, r=me
<henninge> adeuring: yes, thanks
<henninge> I am still trying to answer the question about the substitution ... ;)
<henninge> give up )
<henninge> :)
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jtv || queue: [bryceh] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bryceh || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> bryceh: I am not sure if we should use the term "patch attachment" as the title for the checkbox. The main bug page has two portlets "patches" and "bug attachments", thus distiguishing between patches and ohter attachments.
<adeuring> Having the two words in the title for one search option might confuse users
<adeuring> (on the "advanced search" page)
<noodles775> Hi adeuring, if you can ignore the conflicts on the MP (and use the linked diff instead), would you be able to look at: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-3/+merge/24814
<adeuring> noodles775: sure
<noodles775> (for the same reason, to run the tests, you'll have to branch it rather than merge).
<noodles775> Thanks!
* noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bryceh || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> :)
<adeuring> noodles775: regarding the removeSecurityProxy() calls: I think the properties estimated_duration and status are simply missing in a set_attributes directive.
<noodles775> adeuring: Yes, but I haven't changed the set_attributes (other than to change attribute names). That is, it was being set before without either the set_attributes or removeSecurityProxy.
<noodles775> Unless I missed it somewhere else in the zcml.
<adeuring> noodles775: that's odd.
<noodles775> odd, or more likely something obvious that I missed in one of the 10 other branches in this pipeline :/
<adeuring> noodles775: but whatever the reason is that it worked before: what about adding ZCML directives instead of using removeSecurityProxy?
<noodles775> adeuring: but in most of those cases, I'm using it to setup a test situation which shouldn't normally be possible (like, for example, updating the distro_arch_series of a build, or the build's status).
<noodles775> Or did you mean specifically for the buildqueue's estimated_duration? That would be ok.
<adeuring> noodles775: right, I am not that much concerned about tests
<adeuring> but I believe I spotted one other location, but can't find it again. (for estimated_duration)
 * noodles775 looks to see where I've used rSP in the code... it's just for the temporary static methods (ie. that used to use self.attrname)
<noodles775> adeuring: in buildbase.py, with the XXX and comment explaining why.
<adeuring> noodles775: right, sorry, I needed some time to find it again locally ;)
<adeuring> noodles775: otherwise, no complaints, so r=me
<noodles775> Thanks adeuring. I've got another one ready too: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-generate-key-failure/+merge/24871
<noodles775> wgrant: did you have any comment on the change I added for non-ascii chars^^
<adeuring> noodles775: ok, but let me have lunch first ;)
<noodles775> adeuring: of course... enjoy!
* noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> noodles775: Ah, true, it was only reasonably safe things. But that looks much better (besides the lack of whoami on that revision).
<wgrant> I don't think str() on a unicode is ever right.
<noodles775> Yeah. (And yes, just setting up a new vm for dev work, I'll re-commit that rev).
<noodles775> Thanks for the input!
<wgrant> Thanks for fixing that.
<adeuring> noodles775: I think it is a safe guess to use UTF8 encoding -- but shouldn't we add a test that filtering for a string conainting non-ascii character finds the expected data?
<noodles775> adeuring: as per my comment there, as far as I can see, this is only ever used for getting keys by email address or fingerprint (neither of which use non-ascii code), so I'm not sure how to test it (other than what I've done, ensuring it doesn't error). Any ideas?
<adeuring> noodles775: we could add a test key having a "bad" name. But since you say that we seem to search only for email addresses and FPs, I think it is not that important. (sorry, missed that)
<adeuring> noodles775: so, r=me
<noodles775> adeuring: great, 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> wgrant: resending your branch now to ec2 now (let me know if you've already gotten someone else to do it.)
* sinzui changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> sinzui: r=me; two nitpicks
<sinzui> pick away
* 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
* adeuring changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> I'm not really on call today, but I can do reviews if needed
<abentley> rockstar, could you review my branch, please? lp:~abentley/launchpad/fix-encoded-attachments into	lp:launchpad/devel
<abentley> https://code.launchpad.net/~abentley/launchpad/fix-encoded-attachments/+merge/24917
<rockstar> abentley, looking
<rockstar> abentley, r=me
<abentley> rockstar, thanks.
<mars> gary_poster, do you have a moment to review this config change?  It is very simple, just reorders the keys: https://code.edge.launchpad.net/~mars/launchpad/reorder_config_keys/+merge/24865
<gary_poster> looking, mars
<gary_poster> mars, mostly out of curiosity, do we have standards for this formatting, such as two CRs before each section start?
<mars> gary_poster, nope, I think convention works well enough here.
<mars> plenty of prior art.  violations stand out.
<gary_poster> mars, couldn't the same be said of alphabetization?
<mars> gary_poster, ?
<mars> you mean, "It looks better without alphabetizing it, why should I"?
<gary_poster> mars, IOW, you define an alphabetization standard in writing at the top of the doc, even though most things were alphabetized.  Convention did not seem to be sufficient.  Therefore, do we also need to document two CRs before each heading?  All that said, I think I'm being picky because I feel tired. ;-) I'll approve.
<mars> \o/
<mars> thanks Gary :)
<gary_poster> :-)
<mars> gary_poster, fwiw, that's why I personalized the message at the top.  Personalization can be a more powerful motivator than standards sometimes.
<mars> It proves I care
<mars> gary_poster, have a moment for one more, a trivial addition of an XXX comment?  https://code.edge.launchpad.net/~mars/launchpad/trivial-add-ec2-xxx/+merge/24927
<gary_poster> alright mars
<gary_poster> mars, approved with request to add date per the usual policy
<mars> gary_poster, good catch, thanks
<gary_poster> np
<rockstar> Can I get a quick review from someone?  https://code.edge.launchpad.net/~rockstar/launchpad/recipe-polish/+merge/24928
<rockstar> mars, ^^ ?
<mars> rockstar, looking
<mars> rockstar, why did you nuke all of that test output around line 49 of the diff?  I don't see anything in your code that would suggest its removal.
<rockstar> mars, they were distroseries that have distributions that have no archives.
<rockstar> mars, basically, this branch prevents someone trying to request a recipe build for lenny (Debian)
<rockstar> ...because we don't support any distro but Ubuntu.
<mars> oh!  So your fix works, because it proved that the tests themselves had invalid distros in there
<rockstar> mars, yup.
<mars> rockstar, ok.  Do you need a test for the explicit exclusion of a distro?
<mars> rockstar, a unit test or something?
<rockstar> mars, no, I don't think so.
<rockstar> The test coverage is being accomplished properly with what we have now.
<mars> ok
<mars> rockstar, r=mars
<rockstar> mars, thanks
