#launchpad-reviews 2010-06-21
<jtv> Do we still require the backquotes for """See IThisClass.""" docstrings?
<noodles775> It's still listed here: https://dev.launchpad.net/PythonStyleGuide#Docstrings
<jtv> noodles775: Thanks!  Then that settles it for me, and I'll continue to be a prick about it.  :-)
<noodles775> heh... I meant that if there's no reason to use it, update the wiki (and bring it up for the review meeting?). I assume it's used for pydoctor, but have never run our code through pydoctor myself.
* abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> abentley, can i trouble you to review https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/server-takes-precedence/+merge/28059 ?
<abentley> leonardr, sure.
<abentley> leonardr, why are 'team' and 'person' different types?
<leonardr> abentley: 'team' has some features like membership_policy that 'person' doesn't have
<leonardr> same reason ITeam and IPerson are different interfaces
<abentley> leonardr, it seems like this behaviour should require that the returned type is derived from the expected type, don't you think?
<leonardr> abentley: i don't think so--i think if the client and the server disagree for any reason, the client is just wrong. in real life, the only reason they'll disagree is because of an inheritance situation
<leonardr> but there's no way to represent inheritance in wadl, so we can't be much more sophisticated
<abentley> leonardr, I don't think it matters who's wrong.  If the returned object's interface isn't a superset of the expected interface, chaos will result.
<abentley> leonardr, I am having trouble finding out where the 'people' collection is defined, but PersonSet always returns IPerson, not ITeam, which seems to contradict your documentation.
<abentley> leonardr, even findTeam returns IPerson.
<leonardr> abentley: let's say we had a magic way of knowing. if we detected a non-inheritance scenario, we'd throw an exception. that's exactly what'll happen now: an exception saying 'a bar-type object doesn't have a property "onlypresentinfoo"'
<leonardr> the 'people' collection i refer to is the client-side collection in launchpadlib
<leonardr> on the server side, we declare the most general interface (though findTeam really should return ITeam). on the client side, we expect the most specific interface
<leonardr> that is, it's guaranteed that everything coming out of PersonSet implements at least IPerson
<leonardr> but if you grab an object from PersonSet, and you know *nothing* about it, the most convenient thing to do for your end-user is to let them treat it as ITeam if they think it's an ITeam, and raise an exception if that assumption turns out to be invalid
<leonardr> that decision could go the other way, and this branch would still be needed, but launchpadlib wouldn't be as useful
<leonardr> you couldn't do launchpad.people['team'].membership_policy
<abentley> leonardr, don't you need to load the object to evaluate launchpad.people['team'].membership_policy ?
<leonardr> only if 'membership_policy' is something that could conceivably be a property of launchpad.people['team']
<leonardr> if you do launchpad.people['team'].nosuchproperty it won't load the object, you'll just get an error
<leonardr> the question is whether or not membership_policy (or anything held by teams but not persons) should look like nosuchproperty
<abentley> leonardr, it seems like it would be a programming error to try to access launchpad.people['team'].nosuchproperty and therefore making avoid retrieving the object is not a great advantage.
<leonardr> i agree it's not a great advantage, but that's how it works now. a request will only go over the wire if it conforms with what the client knows about the application based on the wadl
<leonardr> by contrast, launchpad.people['person'].membership_policy will go over the wire and _then_ raise an exception
<leonardr> the important thing is that both cases raise exceptions
<abentley> Can you give me an example of a case where there wouldn't be an exception?
<leonardr> launchpad.people['person'].name or launchpad.people['team'].membership_policy
<leonardr> in the first case, the property is present in both team and person
<leonardr> in the second case, the property is only present for team, but fortunately the object happens to be a team (according to the server)
<abentley> leonardr, can you give me an example where the object is a person, but appears to be a team?  The first one I can think of is dir(person)
<leonardr> abentley, i'm about to push a launchpadlib branch that will hopefully make it clearer
<leonardr> but, to answer your question, launchpad.people['person'] will appear to be a team until the request is made. so, yes, dir(person) and .lp_attributes will show team attributes
<leonardr> once the request is made, dir(person) will change to be more accurate
<leonardr> abentley: https://code.edge.launchpad.net/~leonardr/launchpadlib/shim-users/+merge/28067
<krkhan> gmb: thanks for the review. i'll post on the list tonight for suggestions and see how it goes
<gmb> krkhan, Cool.
<abentley> leonardr, why doesn't setting "httplib2.debuglevel = 0" make the test always pass?
<leonardr> abentley: going to go out on a limb here and suppose that you have the wrong idea about what httplib2.debuglevel does?
<abentley> leonardr, could be.
<leonardr> all it does is show the requests as they're being made, so that i can demonstrate that requests are made later rather than sooner
<leonardr> it doesn't affect the test environment itself
<abentley> leonardr, I think that's what I expected.
<abentley> leonardr, so why does line 56 show a GET?
<leonardr> ah
<leonardr> when you create a client object, it inherits the current value of httplib2.debuglevel
<leonardr> it would be better to set httplib2.debuglevel later, for readability, but technically it doesn't matter--you can unset it anytime after you create the client
<leonardr> i'll make the change
<abentley> leonardr, okay, cool.
<abentley> leonardr, it would be nice if you could create it with "debug_service = CookbookWebServiceClient(_http_debuglevel=1)" to reflect this.
<leonardr> that's a good idea but i'd rather do that in a separate branch, since that would affect a bunch of tests
<abentley> leonardr, that's fine.  I don't require it, just an idea.
<leonardr> ok
<abentley> Could you update the documentation to mention that WADL doesn't provide a way to find out inheritance relationships?
<abentley> leonardr, also, did you consider allowing collections claim that they provide multiple types?
<abentley> leonardr, that would allow a shim to be an IPersonOrITeam shim, and presumably it could find out which it is when it needs to.
<leonardr> abentley: yes, that was my original (much more complicated plan), until flacoste said that team is and can only be a superset of person
<leonardr> and the way this works in practice is very similar to that
<leonardr> since team is a superset of person, IPersonOrTeam looks just like ITeam
<leonardr> and any attempt to explicitly find out what kind of object a specific object is will collapse the wavefunction by making an http request
<abentley> leonardr, I thought IPersonOrTeam would make http requests when dir(IPersonOrTeam) was invoked, etc.
<leonardr> i see
<james_w> salgado-lunch: https://code.edge.launchpad.net/~james-w/launchpad/copy-archive-test-refactor/+merge/28073 is the branch I was talking about if you could review please.
<leonardr> i don't think that's worth the work, and if it is, we can do that work just as easily with this branch as a base
<abentley> leonardr, approved, but I'd like to see an explanation of why you didn't consider inheritance (WADL doesn't provide that) in the docs.
<leonardr> ok
<leonardr> abentley, want to look at the launchpadlib branch too? (if not, gary has expressed an interest)
<gary_poster> (a willingness :-) )
<abentley> gary_poster, if you don't mind, there are some issues I'd like to chase up.
<gary_poster> abentley, leonardr, will do
<salgado> james_w, I'll get to it in a minute
<james_w> thanks
<gary_poster> leonardr: launchpadlib branch approved, no comments
<leonardr> gary: cool. lazr.restfulclient release is out, launchpadlib release will follow, then i'm going to go to bed
<gary_poster> ok, leonardr
<leonardr> gary: done, see you tomorrow
* sinzui changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> is anyone available for a 47 line fix: https://code.edge.launchpad.net/~sinzui/launchpad/unique-openid-identifiers-0/+merge/28081
<sinzui> abentley, are you available for a 47 line fix: https://code.edge.launchpad.net/~sinzui/launchpad/unique-openid-identifiers-0/+merge/28081
<abentley> sinzui, sure.
* abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> sinzui, openid identifiers are derived from account names?
<sinzui> no random stringd
<sinzui> sample data sucks and uses
<abentley> sinzui, so the random strings aren't random enough?
<sinzui> the randoms get reused apparently
<sinzui> I saw something like merge-Z3dgy45  and Z3dgy45 in staging. the latter cannot be merged
<abentley> sinzui, that doesn't sound like a good idea...
<sinzui> We do not control openid identifiers
<abentley> sinzui, Ah, that's canonical-foundations?
<EdwinGrubbs> abentley: will you have time to review this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-35728-bugtracker-overlay/+merge/28100
<abentley> EdwinGrubbs, I expect so.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: sinzui || queue: [sinzui, Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> sinzui, "dash which does not occur in SSO identifier." should probably be "dash, which does not occur in SSO identifiers."
<sinzui> abentley,  ISD, canonical-identity-provider reuses identifier tokens. I was surprised. In my original fix I wrote that since we know the identifier is unique and cannot contain a dash, we could prepend 'merge-' and there was no chance of future conflict...
<sinzui> I was proven wrong in 10 days
<abentley> sinzui, if you are replying to my last statement, I was just nitpicking grammar.
<sinzui> abentley, no, I was trying to provide backgroind to  "that's canonical-foundations?"
 * sinzui is being pedantic, sorry
<abentley> sinzui, ah, I missed that you mentioned ISD in there.
<abentley> sinzui, I think it would be good to add a unit test to ensure that merging accounts works when you have duplicate identifiers.
<sinzui> yes, I should do that
<abentley> sinzui, the change itself looks fine.
<abentley> sinzui, ping me when you have that test.
<sinzui> okay
<abentley> sinzui, also, this sinzui in the queue: is that the same thing?
<sinzui> yep
* abentley changed the topic of #launchpad-reviews to: abentley || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs, could you explain 395-397, please?
* abentley changed the topic of #launchpad-reviews to: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: it's not really important now that we normally run windmill tests through the test runner so we always have a clean database. I copied it from test_add_milestone. I can remove it if you want.
<abentley> EdwinGrubbs, please do, then.
* abentley changed the topic of #launchpad-reviews to: is On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> abentley, check your email for my reply and incremental diff
<abentley> sinzui, I am EOD.
#launchpad-reviews 2010-06-22
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> gmb: howdy, would you have a few minutes for https://code.edge.launchpad.net/~james-w/launchpad/copy-archive-package-sets/+merge/28165 ?
<gmb> james_w, Sure
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: james_w || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> gmb: https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/28170
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: james_w || queue: [stub] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> stub, Will look at it after I've finished james_w's
<stub> Ta.
<bdmurray> gmb: if you could review my mp that'd be great https://code.edge.launchpad.net/~brian-murray/launchpad/bug-97633/+merge/28109
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: james_w || queue: [stub, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bdmurray, Will do
<gmb> Merge proposals are like buses today...
 * gmb hates at inconsistent naming styles for test* methods.
<gmb> james_w, r=me with one minor nitpick
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: stub || queue: [bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> thanks gmb
<gmb> stub, Your first test doesn't appear to test anything. Or, rather, if it failed, what would the output be?
<stub> eh?
 * stub has a look
<stub> Its testing that it doesn't explode when I omit the unit
<gmb> stub, Ah, right, I see.
<gmb> yes, that's now actually quite obvious. Sorry
<stub> There are no hooks to test that the specified expiry is actually what is being used, but that is the case for the existing tests (I'd need to hack things to add the calculated cache time as an HTML comment I think).
<stub> Which I could add easily enough but might make some tests explode.
<gmb> stub, No need to change it (maybe you could update the narrative to say that it won't explode, I guess).
<gmb> stub, r=me anyway
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> Ta.
<stub> gmb: https://code.edge.launchpad.net/~stub/launchpad/cache-bugcomments/+merge/28177 is the follow on to that branch. Not sure if you want to review it today or punt until after an ec2 run has discovered any test fallout.
<gmb> stub, I might as well review it today. If EC2 results necessitate large changes then you can always request a review on just those revisions.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: bdmurray || queue: [stub] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bdmurray, r=me. Nice work!
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: gmb (having connection issues; responsiveness my be affected) || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> stub, r=me on your second branch too.
<stub> Ta.
<stub> gmb: Don't see a tick on https://code.edge.launchpad.net/~stub/launchpad/cache-bugcomments/+merge/28177
<gmb> stub, Oh, it's still spinning. Hang on, I'll re-do it.
<gmb> stub, Done
* gmb changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> could somebody mark https://code.edge.launchpad.net/~james-w/launchpad/copy-archive-test-refactor/+merge/28073 approved please?
<jelmer_> james_w: sure
<jelmer_> james_w: can you land the changes yourself?
<james_w> yeah, just can't approve them
<james_w> thanks
<bac> mars: ping
<mars> bac, pong
<bac> hey mars.  are you reviewing today or leonard (or no one?)  :)
<mars> bac, ah, Leonard is sick today.  My bad, I should have swapped with him
<mars> since I'll be RM next week
<mars> bac, do you have a branch you would like reviewed?
<bac> mars: actually, yes, if you can
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-162754/+merge/28227
* mars changed the topic of #launchpad-reviews to:  On call: mars || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> mars: and could you ask leonard to be sure to be OCR next week?
<mars> bac, I'll set a calendar item for it
<mars> and yes, I will
<bac> sweet
<bac> and have fun being RM!
<mars> CHR, OCR and RM, oh my.
<mars> bac, btw, +1 on the branch concept - I had that exact problem when I registered Geany, which uses SF and Github
<bac> mars: yeah, it's been a wart for a while
<bac> hope this approach makes people happy
<mars> bac, has the text been run by mrevell?
<bac> mars: he looked at some early screenshots.  i'll do a 'text' review with him
<mars> ok, thanks.  These pages are critical for conveying new concepts - the text is important
<mars> Like "what's a driver, and why do you offer me the choice of not being one?" :)
<bac> yep.  i've requested the review
<bac> brb
<mars> I take it Curtis counts as the UI reviewer?
<bac> mars: i'll get a UI review, whether curtis or another
<mars> bac, ok. I am doing a cursory review now, not official.  I'll include my recommendations with the code review.
<bac> thanks mars
<EdwinGrubbs> mars: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part3/+merge/28240
<mars> EdwinGrubbs, yes, please add yourself to the que
<mars> queue
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: mars || reviewing: bac || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call: mars || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> EdwinGrubbs, looking at your patch, I realize that I do not have the knowledge necessary to review it.  I know the upper layers of Launchpad, but little of the lower.
<EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part3/+merge/28240
<bac> EdwinGrubbs: sure
<EdwinGrubbs> thanks
* mars changed the topic of #launchpad-reviews to:  On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs: the SQL comment you wrote is: Whether an upstream link should be added if it does not already exist.
<bac> EdwinGrubbs: should that "should" be a "may"?
<EdwinGrubbs> sure, that sounds fine.
<bac> EdwinGrubbs: done
<EdwinGrubbs> cool
* mars 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-06-23
<EdwinGrubbs> stub: I'm available for a little bit if you have questions regarding my merge proposal that fixes the branch that got reverted. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part3/+merge/28240
<stub> k
* jelmer_ changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi mrevell
<mrevell> hi bac
<bac> mrevell: hey did you see my request for review?
<bac> part of it you've already seen.
<mrevell> Sorry, yes, I've been catching up from a day at Millbank. I'll get to it now.
<bac> mrevell: thanks.
<Edwin-zzzz> BjornT: can you do a db review of this branch, please. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part3/+merge/28240
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: - || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bdmurray> EdwinGrubbs: could I get https://code.edge.launchpad.net/~brian-murray/launchpad/bug-130902/+merge/28246 reviewed?
<EdwinGrubbs> bdmurray: sure
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: bdmurray || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: - || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> BjornT: ping
<jelmer_> EdwinGrubbs: hi
<EdwinGrubbs> jelmer_: hi
<jelmer_> EdwinGrubbs: Could you review a relatively simple soyuz branch?
<jelmer_> EdwinGrubbs: Never mind, you already did. Sorry!
* jelmer_ changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> awesome, I love not having more work
<bac> hi mar
<bac> hi mars
<mars> hi bac, I've got a few minutes - what's up?
<bac> mars: not much.  just wanted to chat about your review.  the question is moot, though, as i've incorporated the changes
<mars> ok.  Did you get a proper UI review yet?
<bac> yeah, curtis did it
<mars> Alright, just read the mail thread
<mars> bac, ok, so all is well then?
<bac> yeah
#launchpad-reviews 2010-06-24
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/fix-publishing-getbyid/+merge/28359 if someone has a minute please
* james_w changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [james_w] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> (assuming Edwin has gone to bed)
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: james_w || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> 'morning James
* StevenK changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: james_w || queue: [StevenK*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> morning jelmer_
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [StevenK*2] || 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: jelmer || reviewing: StevenK || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> looks like it's going to be a soyuz-heavy review day again :-)
 * bigjools will also have 2 more later :)
<StevenK> jelmer_: The two MPs are https://code.edge.launchpad.net/~stevenk/launchpad/expose-iarchive-newauth/+merge/28090 and https://code.edge.launchpad.net/~stevenk/launchpad/subscribers-can-view-p3as/+merge/27806
<james_w> replied
<jelmer_> james_w: r=me
<james_w> thanks
<james_w> writing tests for all the factory would be incredibly tedious
<james_w> and generating them isn't much better than not having them.
<james_w> but I agree that relying on indirect testing for it is not great
<jelmer_> james_w: just checking that e.g. makeBinaryPackagePublishingHistory() returns something sensible ?
<james_w> yeah, that would be ok
<jelmer_> and takes into account the parameters its passed
<james_w> but going through each parameter and specifying it, and then not specifying it and checking that you get something sensible created?
<jelmer_> james_w: I guess there's a tradeoff decision to make there somewhere.
<james_w> I guess you can leave some of that to Storm, because it won't let you set the wrong type to an attribute, or leave of a required one
<james_w> I'm happy to start a testsuite and add the things that I just added if you like?
<james_w> well, I would be if I wasn't working on a db patch, I've had enough of "make schema" for a little while
<jelmer_> :-)
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> StevenK: r=me for the first branch, reviewing the second now
<wgrant> jelmer_, StevenK: Shouldn't getPrivateSourcesList have its person argument forced to REQUEST_USER?
<wgrant> letting anyone get anyone else's URL does not sound desirable.
<wgrant> As for the other one, doesn't that let any subscriber use the API to poke around, negating the security benefits conveyed by the +packages restriction?
<wgrant> +copy-packages, etc., too.
<jelmer_> wgrant: good point (regarding REQUEST_USER)
<jelmer_> wgrant: looking at the other issue now
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> jelmer_: What's your assessment of the permissions one?
<salgado> hi jelmer_.  care to review https://code.launchpad.net/~salgado/launchpad/remove-policy_options/+merge/28392 for me?
<jelmer_> wgrant: following up to steve now
<wgrant> jelmer_: Thanks.
<jelmer_> StevenK: I was about to head out for lunch, but I'll have a look after.
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> actually, I'll have a look now as it's a small branch
<StevenK> wgrant: How would you recommend I change it to use REQUEST_USER always?
<jelmer_> StevenK: use @call_with(user=REQUEST_USER)
<wgrant> StevenK: @call_with(person=REQUEST_USER)
<wgrant> And drop the @operation_parameters
<StevenK> jelmer_, wgrant: http://paste.ubuntu.com/454419/
<jelmer_> StevenK: yep
<jelmer_> StevenK: You probably also have to update your web service API test, as it should refuse that argument now.
<jelmer_> salgado: r=me, thanks for cleaning this up
<StevenK> Right
<salgado> jelmer_, cool, thanks!
 * jelmer_ really goes to lunch this time
<rockstar> jelmer_, can I jump on your queue?
<james_w> jelmer_: another soyuz one if you can fit it in :-) https://code.edge.launchpad.net/~james-w/launchpad/test-package-cloner/+merge/28405
* bigjools changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [salgado, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> 2 liner from me jelmer_
<rockstar> jelmer_, my proposal is here: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-security/+merge/28410
<jelmer_> rockstar: sorry, was lunching - go ahead
<rockstar> jelmer_, see my proposal above
 * rockstar is one of those folks who likes to ASK to have a review, not just assume one...
<rockstar> (like bigjools)
* rockstar changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [salgado, bigjools, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bigjools changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [salgado, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> pff
<jelmer_> :-)
<jml> rockstar, submitting a merge proposal _is_ asking for a review
<rockstar> jml, I entirely disagree.
<jml> rockstar, do go on.
<rockstar> jml, it's the reviewees responsibility to get it reviewed, not the reviewers.
<jelmer_> jml: my impression is that the stuff in the queue is what the current reviewer is going to process in their shift
<rockstar> If the OCR decides to go out and look for reviews, good on him, but I don't think it's his job.
<jelmer_> jml: Not sure if that's right but that's what I've observed so far :-)
<rockstar> jml, basically, I believe in the "On Call Reviewer" philosophy, not "Review Lackey" philosophy.
<jml> rockstar, nothing that you've said contradicts what I said.
<jml> jelmer_, that does seem to be how it works.
<rockstar> jml, indeed I think it does.  Throwing a ball for my dog doesn't necessarily imply "go fetch"
<jml> rockstar, How is sending an email with text requesting a review and an attached patch to a list of developers with commit access *not* asking for a review?
<jml> rockstar, it might not be asking the OCR for a review, it might be ineffective because contributors can't expect developers to have the time to incorporate their efforts, it might be counter to the process, but it is most certainly asking for a review
<rockstar> jml, okay, so let's just settle on it being a terrible way to ask for a review, like standing in the middle of Paddington station and yelling "DOES ANYONE HAVE A PEN?"
<rockstar> jml, if no one does the review, and it sits for a week, who's responsibility is it?
<jml> rockstar, yours
<rockstar> jml, yours == reviewee?
<jml> rockstar, no, yours as a Launchpad developer with commit access. mine too, I guess, although I'm inactive.
<rockstar> jml, yes, I could get to it, but I also have my own responsibilities and branches to land.
<rockstar> jml, if you come to me and say "can I have a review?" I can say "Can you find someone else? I'm rather swamped" or "Sure"  Otherwise, I'm focusing on the work I need to do, and arranging my own reviews with others.
<rockstar> jml, so yes, it's my responsibility (and yours) but it's one in many different responsibilities, and often not the highest priority to me.
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: james_w || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> So I guess by going to a OCR or another specific reviewer, you're asking for a higher priority on their list of things to do.
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: rockstar || queue: [james_w] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jml, so you could say "Creating a merge proposal is asking for a review, but if you want it to get on someone's TODO list, you better ask them"
<jelmer_> is *everybody* doing soyuz branches today?
<rockstar> jelmer_, the line between code and soyuz is blurring at a rapid rate...  :)
<rockstar> jelmer_, although truthfully, I'm finding the Soyuz problems becoming more and more interesting.
<bigjools> ch..ch..ch...changes
<rockstar> jelmer_, howgoesit?
<jelmer_> rockstar: almost done reviewing
<rockstar> jelmer_, ack, thanks for doing this.
<rockstar> jelmer_, I can take james_w's review (since I'm OCR next) if you want to EOD after me.
<jelmer_> rockstar: that's alright, I'll be here for another 2 or 3 hours
<rockstar> jelmer_, okay.
<jelmer_> rockstar: although I won't stop you, it would mean a quicker review for James
<rockstar> jelmer_, yeah, I need to start my shift now.
* rockstar changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: rockstar, james_w || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> rockstar: i just read your conversation with jml.  my unsolicited thoughts:
<rockstar> bac, they are solicited now
<rockstar> bac, I was planning on copying the conversation and posting it to the list.
<bac> 1) as an OCR with no requested reviews, I look to +activereviews and try to beat down any backlog.  i assume other reviewers do the same.
<bac> 2) submitting a MP *is* asking for a review but in the most ineffective way possible.  it is still the submitters responsibility to *hunt down* a reviewer and make sure it gets done.
<bac> 3) i can't think of a third.
<rockstar> bac, I go through +activereviews as well, to an extent.  If the person isn't around to ask questions to, I usually just leave it alone.
<rockstar> bac, in a way though, I do agree with jml that it's also the responsibility of the people who have commit rights.
<bac> rockstar: i can see your point but that really disadvantages our colleagues in underrepresented time zones
<jml> there's an analogy with a user asking a question on #launchpad
<bac> i prefer doing on-call reviews that can be done interactively but have no problem just posing my questions in the review if the person is not available
<rockstar> bac, this is true, but every time I get involved with jtv as reviewer, I always drag it out because we're in different time zones.  I don't think I'm the most effective reviewer for jtv.
<rockstar> jml made a good argument that it's the committers responsibility, because this is an open source project, and I'd expect something similar on any project I contribute to.
<rockstar> Granted I feel the need to be a squeaky wheel as well though.
<bac> rockstar: we've had different experiences then that have led to different conclusions.  i think jtv and i work well on both sides of the review fence and have considered a little delay for him just to be part of living in BKK
<bac> the review team should be pro-active but i still maintain it is the branch owner who must ultimately be responsible for getting a review.  if that process is difficult then that is egg on our faces and something we should fix.
<rockstar> bac, ack, I completely agree with you.
<bac> jelmer_, rockstar: great, now that we're in agreement, who'd like to do a review for me?  https://code.edge.launchpad.net/~bac/launchpad/bug-595907/+merge/28423
<rockstar> bac, pop it on the queue.
* bac changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: rockstar, james_w || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> thanks
<bac> rockstar, jelmer_: i am truly a despicable person.  after requesting an on-call review, i must step out for a bit.  feel free to skip over me if i'm not here when my number is called.
<rockstar> bac, don't sweat it.  I need to chat with sinzui on post-imp call that should have been a pre-imp call at some point this morning.
<jml> who would like to review https://code.edge.launchpad.net/~jml/launchpad/apidoc-dir/+merge/28432
<jml> it's wafer thin.
<jml> As someone once said, "All that is necessary for the triumph of evil is for good men to do nothing."
<jelmer_> jml: me me me
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: jml, james_w || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> jelmer_, thanks.
<jelmer_> jml: I would expect that branch to also add an entry to .bzrignore
<jml> jelmer_, I would likewise except the same!
 * jml fixes
<jelmer_> :-)
<bac> jelmer_, rockstar: i've returned from a very refreshing lunch
<jml> jelmer_, other than that, am I ok to land?
<jelmer_> jml: Yeah, r=me
<james_w> could someone check that I have submitted https://code.edge.launchpad.net/~james-w/launchpad/archive-job-db/+merge/28437 correctly?
<jelmer_> james_w: Yeah, looks ok to me - against db-devel and db review from stub
<jelmer_> I don't know what else there is to consider.
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: bac, james_w || queue: [bac] || 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: jelmer, rockstar || reviewing: bac, james_w || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> Somebody should write a bot to update the topic..
<james_w> thanks
<rockstar> jelmer_, my review is still pending...  :(
<rockstar> jelmer_, I've also thought about having a bot update the topic.
<bac> jelmer_: are you still working on my review?  any questions?
<jelmer_> bac: sorry, yeah - kept getting interrupted but almost finished now
<bac> ok, thanks jelmer_
<jelmer_> bac: I'm getting strange test failures in distribution-mirror.txt that seem unrelated to your changes
 * bac looks
<jelmer_> bac: I can confirm that the pop-up icons are displayed and your changes are fine.
<jelmer_> bac: r=me, it seems specific to my desktop machine
<jelmer_> bac: my apologies for taking so long.
<bac> jelmer_: i'm re-running those tests now and will only land the branch via ec2
<bac> jelmer_: no worries.  you've had a very long day by now, i'm sure
* sinzui changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: bac, james_w || queue: [sinzui] || 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: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-06-25
* wgrant changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: sinzui || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> jtv: Can you sanity check https://code.edge.launchpad.net/~james-w/launchpad/archive-job-db/+merge/28437 with a quick db review?
<jtv> hi stub!  Sure, just taking care of something hang on
<jtv> stub: looking now
<jtv> stub: why disable and immediately re-enable triggers?
<stub> That is just fluff from the sample data build script. There is no point to doing it, but pg_dump seems to do it anyway.
<stub> Actually, it might ensure that the triggers are enabled after the restore, which might be a desirable feature.
<stub> But it is still noise to us
<jtv> Oh, you're saying enabling them twice is an error but disabling them twice isn't, so you disable first?
<jtv> It's unfortunate that we end up with tables for "jobs of various sorts that happen to have a lot to do with a branch" and now one for "jobs that do stuff with an archive"
<jtv> Oh, call coming up
<stub> I have no idea. Its an artifact from pg_dump anyway and the sample data generation script isn't smart enough to strip the noise.
<stub> What do you find unfortunate?
<stub> I was sort of hoping the separate queues would make things easier but maybe it needs rethinking
<stub> I worry that having FooJob and Job mean we get neither the benefits of separate queues nor the benefits of a shared queue.
<jtv> stub: sorry for being unresponsiveâthat call is currently in progress
<stub> There are things we could be doing with the central Job table, like centralised scheduling, but we are not doing that.
<stub> No worries.
<jtv> stub: off call.  noodles775: g'day
<noodles775> Hi jtv
<jtv> stub: what I find unfortunate is what looks like a growing taxonomy of classes based on what they happen to contain.  I know it's more or less inevitable sometimes, so I'm not saying it's wrongâjust a bit concerned over the trend.
<stub> Well... its a database, not an object hierarchy. Or are you thinking of the code that uses these tables for storage?
<jtv> stub: Bit of both.  IIRC we get to have a scheduler per table, and then use the class model to figure out what to do for each type of job.
<jtv> So a very mixed model, with class tagging etc.
<jtv> Might be best to have a single scheduler that just outer-joins BranchJob and ArchiveJob and then delegates the "what does processing this job entail, anyway" part to the Python type system in some reasonably uniform way.  Maybe that's what's being done here; I don't know that yet.
<wgrant> We're also about to stuff buildfarm jobs into Job.
<jtv> wgrant: your timing is impeccable
<wgrant> jtv: Oh?
<jtv> wgrant: I just mean it's great that you're bringing in this fact just now.  Could help avoid some future stumbling-in-the-dark.  :-)
<wgrant> Ah.
<jtv> wgrant: though come to think of it, the same scheduler should probably ignore those, right?
<wgrant> Well, 'about to' may be an exaggeration. It's designed, though I'm not sure if it's scheduled, and I'm hoping that someone gets sufficiently irritated by the insane queue structure that we have now.
<wgrant> Right, this sort of thing would have to be ignored by any standard scheduler.
<wgrant> Since it's handled by the buildmaster.
<jtv> Which has its own very extensive scheduling logic.
<wgrant> Um.
<wgrant> Not... exactly.
<jtv> No?
<noodles775> Which could happily die a quick death IMO.
<noodles775> (It's overcomplicated for what it does).
<jtv> I was thinking of the stuff with architectures, availability, private archives etc.
<wgrant> Mm, I guess.
<jtv> Compared to BranchJob scheduling, which is simple FCFS
<jtv> AFAIK
<jtv> So I guess that the BuildFarmJob would simply attach to Job in the same way but live outside the BranchJob/ArchiveJob scheduling world.
<noodles775> jtv: for example, couldn't private archives just be a *separate* queue if we used a messaging system/other scheduler?
<wgrant> I think we need to move SPRBs and TTBJs over to the New World, rip out all the old stuff, and think about how we can kill even more given the new simplified design.
<wgrant> Because at the moment it doesn't really work.
<noodles775> +100
<wgrant> And is very overcomplicated for the whole not-working thing.
<wgrant> Nice simple buildfarm-related branch at https://code.edge.launchpad.net/~wgrant/launchpad/faster-and-more-general-getBuildQueueSizes/+merge/28476, if anyone's interested...
<noodles775> Nice... more red than green :)
<noodles775> And much simpler query. Is it worth adding tests that ensure the other build types are considered (or possible yet? or worth waiting to land this when it is possible?)
<wgrant> It's not possible yet.
<wgrant> Hm.
<jtv> stub: but anyway, I was looking at that MP for you...  I'm tempted to ask whether json_data should be in Job, but that's got enough generic dead wood in it already.
<wgrant> Actually, it might be.
<wgrant> Since this is just BuildQueue, not BuildFarmJob.
<noodles775> Right - which means it's implied anyway (and doesn't need to be tested... as the BFJ table isn't even referenced)?
<wgrant> If it works for one it will work for the others. It would still be nice to test, though.
<wgrant> Let's see how easy the tests are to change.
<stub> jtv: A case could certainly be made for putting json_data on Job. I can also make a case for putting the job type on job too. Probably not on this branch though.
<noodles775> And would ensure the implementation doesn't change in the future to exclude them... great, thanks wgrant.
 * wgrant throws in a new recipe build job.
<jtv> stub: in that case, just for future reference, BranchJob has the json_data but AFAIK in most cases doesn't use it.
<wgrant> How I hate to make lp.buildmaster depend on lp.code, though :(
<stub> jtv: Hopefully this design will hold together until we can revisit the messaging infrastructure.
<wgrant> stub: Is that ever going to happen?
<jtv> stub: No such thing as a temporary system.  I'm sure it'll hold together and we'll probably never get around to moving it over.  :-)
<jtv> stub: I don't get line 98 of the diff in the MP...  Is it meant to say "job_type" (with an underscore instead of space) or is this a piece of syntax I'm not aware of?
<stub> wgrant: Eventually. foundations got performance this round though, and other teams too busy chasing their features lists. I suspect bugs would have saved time if they had bit the bullet and did it with the bug heat stuff...
<stub> jtv: Has an underscore here -- job_type integer NOT NULL,
<stub> jtv: You installed some new fonts perchance?
<jtv> stub: yes but this isn't using it...  Hang on, I'll reload.
<jtv> ...and now it renders correctly.
<jtv> That's a bit worrisome.
<jtv> stub: trying hard to come up with stuff to bitch about...  are these jobs inserted and deleted by the same db role?
<stub> I have no idea. They probably should add some security.cfg entries, yes.
<stub> oh - there are some there.
<stub> Ahh... bad dev. [write] is deprecated. I've added a new comment.
<jtv> stub: it's just that often you get one role inserting them and another deleting them.
<jtv> stub: I've found about all I can bitch about... are you content with that?
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> noodles775: It's actually not easy to test, since the SPRB and TTBJ factory methods are broken in various ways.
<wgrant> (and fixing them causes widespread test failures)
<wgrant> makeSourcePackageRecipeBuild creates an arch-agnostic build, when SPRBs aren't actually arch-agnostic any more.
<wgrant> And makeTranslationTemplatesBuildJob creates a virt-agnostic build, which doesn't make sense (bug #598390)
<_mup_> Bug #598390: BuildQueue.virtualized and BuildFarmJob.virtualized should be NOT NULL <Soyuz:New> <https://launchpad.net/bugs/598390>
<noodles775> ok. Is it possible (not as nice) to use queued BinaryPackageBuild's and then in one test, just manually change the job_type of one of them (as long as you're not trying to use it for anything, and it's only the duration of that one test, it may test what you're after)?
<wgrant> I guess I could create an SPRB and then manually mangle its processor.
<wgrant> That's probably better.
<wgrant> noodles775: Test there now.
<noodles775> wgrant: thanks... if it's ok, do you mind if adeuring does the review (hopefully the comments we've added will be helpful). I've just got my head in some other stuff atm.
<wgrant> noodles775: Of course.
 * adeuring is looking
<wgrant> adeuring: Can you have a look at https://code.edge.launchpad.net/~wgrant/launchpad/faster-and-more-general-getBuildQueueSizes/+merge/28476 when you have a moment?
<adeuring> wgrant: sure
<wgrant> 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
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> wgrant: r=me; thanks for the additional test.
<adeuring> wgrant: please remind me to land the branch once pQM opens again next week
<wgrant> adeuring: Oh, PQM is closed already?
<wgrant> Thanks.
<adeuring> wgrant: yes, it was closed yetserday
 * wgrant didn't see any emails.
<adeuring> wgrant: gahh, I mixed that up, PQM closes _today-- 22UTC...
 * adeuring is starting the EC2 job
<wgrant> Thanks.
<wgrant> Though I haven't seen any emails to that effect either -- I guess it must have been internal.
<jelmer_> adeuring: it's been extended to today 2200 UTC
<adeuring> wgrant: right, it was announced on an internal list only...
* 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 abgeÃ¤ndert
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews abgeÃ¤ndert
<bac> hi adeuring
<adeuring> hi bac
<bac> sorry i failed to show up to review last week until you'd already gone.  i'm still not used to the switch to friday
<adeuring> bac: no problem --it was a quiet day :)
<bac> adeuring: like today, it seems
<adeuring> right
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews abgeÃ¤ndert
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews abgeÃ¤ndert
<bac> adeuring: your MP has a huge diff.  is that a mistake?
<adeuring> bac: yes, I assume due to the wrong target branch... I re-submitted it. (and aked already deryck to review it)
<bac> oh, ok
<adeuring> wgrant: still around?
<leonardr> adeuring, bac, request a review of this very small branch: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/load-relative-uri/+merge/28511
<adeuring> leonardr: I can look
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: leonardr,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> leonardr: purely a matter of taste: you could use "url[:1] == '/'" instead of "len(url) > 0 and url[0] == '/'" in line 91 of the diff.
<adeuring> leonardr: r=me
* adeuring changed the topic of #launchpad-reviews to: On call:  bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call:  bac || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> Hi, can a non-lunching reviewer take a look at my what's new branch please? https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-1006/+merge/28519
<leonardr> mrevell: couple questions
<mrevell> sure
<leonardr> what's the None by the surveymonkey link? shouldn't it be "take our survey"? or is it none because of the <strong> tag?
<mrevell> leonardr, It seems to be due to the <strong> tag
<leonardr> ok, i bet that's beautifulsoup related, so i can't really complain
<leonardr> it looks like you added two news items, and removed two older news items, but kept the 'launchpad is now open source' news item because that's really important
<leonardr> is that right?
<mrevell> leonardr, That's right. I considered removing the open sourcing item but don't want to do that until we have some other mention of Launchpad being open source on the front page, and I don't have time to do that right now.
<leonardr> mrevell: ok, r=me
<mrevell> thanks leonardr
* bac changed the topic of #launchpad-reviews to: On call:  bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> I have a soyuz-related patch up for review.
<jml> https://code.edge.launchpad.net/~jml/launchpad/poppy-cleanup/+merge/28525
 * jml has been procrastinating
<jml> easy patch here:https://code.edge.launchpad.net/~jml/launchpad/git-and-hg/+merge/28527
<bac> jml: both done
<jml> bac, thanks.
<bac> leonardr: it looks like you reviewed mrevell's branch here but didn't update the MP.  i didn't see this conversation so i did the review again.
<leonardr> bac: argh, sorry
<leonardr> i'm still somewhat flaky from my illness
<leonardr> (not literally)
<bac> leonardr: np.  hope you're feeling better
