#launchpad-reviews 2010-08-23
* henninge changed the topic of #launchpad-reviews to:  call:  henninge || reviewing: -  || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/milestones/+merge/32855] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to:  call:  henninge || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> henninge: I have a couple of branches to review. Do you fancy taking either of them on?
<henninge> allenap: sure, which one do you want done first?
<allenap> henninge: I don't have a preference. refactor-mailnotification is a bit long, but will be a bit boring and simple. cache-experiment might be more interesting for you.
<henninge> allenap: ok, I'll have a look and take my pick ;-)
* StevenK changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: -  || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> henninge: Thanks :)
<henninge> allenap: lifeless was working on cachedproperty, too. Is this related somehow?
<allenap> henninge: Kind of. I saw the cachedproperty code getting a bit ugly, and I'd long had a vague idea for how to go about it differently, so I tried it out. It seems good, to my eyes at least :)
<henninge> allenap: yes, I understand what you mean, I felt similar about 'ugly'. ;-)  Did you have a pre-imp discussion with somebody, though?
<henninge> and I notice that the branch is called an "experiement" ...
<allenap> henninge: No. It was just an experiment at first, but it went better than I expected so I cleaned it up for review.
<henninge> allenap: I see you using tests in docstrings. I seem to remember that we have been moving these into doctest files and not introducing new ones.
<allenap> henninge: I'm happy to move them.
<allenap> henninge: Would you like me to move them before you continue?
<henninge> allenap: I am not sure if that is an actual rule that we follow but I have not seen anybody using them before (except for lifeless).
<henninge> allenap: so, yes, I'd prefer them being moved.
<allenap> henninge: Okay, I'll do that now.
<henninge> allenap: I like the new interface, btw... ;-)
<allenap> henninge: Brilliant :)
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: allenap  || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> henninge: Done; the documentation is now in doc/propertycache.txt.
<henninge> cool ;)
<henninge> allenap: aren't these two calls identical? Do you really still need both?
<henninge> 120	         clear_cachedproperties(self)
<henninge> 121	+        IPropertyCacheManager(self).clear()
<allenap> henninge: They're not identical yet; they clear different caches. I will remove clear_cachedproperties in a subsequent branch.
<henninge> allenap: so, both implementation of cachedproperty exist in parallel atm and it depends on which one you import?
<allenap> henninge: Yes. This branch was getting too long, and I wanted to see if it would pass review before doing more work. They won't interfere with one another; it's safe.
<henninge> allenap: could you please add a comment to those lines up there explaining why both are needed? Also, it would be nice if lifeless' comment was a proper XXX.
<allenap> henninge: Okay.
<henninge> allenap: the change in lib/canonical/configure.zcml is an unrelated drive-by fix, right?
<allenap> henninge: Yes. I realised it made sense to put all the lp.services zcml in the same place while I was there. It's related to the branch, but it is a clean-up.
<henninge> allenap: cool, I see other clean-ups, too. Thanks! ;-)
<henninge> allenap: btw, are you sure this is supposed to go into lp.services? It is not really LP specific, is it?
<henninge> I mean, is the whole "canonical" tree expected to go away eventually, or just "canoincal.launchpad"?
<allenap> henninge: I don't know actually.
<wgrant> I think canonical.* is meant to go into lp.* and lazr.*
<allenap> henninge: Can we assume this is okay for now?
<allenap> henninge: XXX and comment added.
<henninge> allenap: oh yeah, I would not have asked you to move it, I was just wondering ... ;-)
<allenap> henninge: Cool :)
<allenap> henninge: I have to go and help with the kid's lunch now. Do you mind if I step out for ~1h?
<henninge> allenap: Fits perfectly, I'll be out to lunch soon, too. ;-)
<allenap> henninge: Cool, thanks.
<bigjools> henninge: hi, I've got a small branch just pushed up, can I add it to your queue please?
<StevenK> bigjools: Get in line!
<StevenK> :-P
<bigjools> StevenK: yes that's what adding to the queue generally means ...
<henninge> bigjools: go ahead ;-)
* bigjools changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: allenap  || queue: [StevenK, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> thanks henninge
<bigjools> it was quite tempting to jump StevenK for a laugh then :)
<StevenK> Hmph :-P
<henninge> StevenK: I assume that there is no obvious way to stormify the INSERT query in your branch?
 * henninge has no idea, just checking ;)
<StevenK> henninge: I don't know how to Storm-ify stuff, and my last try ended in failure -- what's been added there for archivepermission is copied from other sections of the code
<henninge> StevenK: I know it's being copied. ;-) I can imagine that it is not possible to Storm-ify that ...
<henninge> so nm.
<henninge> StevenK: review sent. r=me after some fixes ;)
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: allenap  || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> henninge: Thanks!
 * henninge goes to lunch
<StevenK> henninge: topic =~ s/allenap/lunch/ ?
<henninge> StevenK: no, I am not done with allenap's yet
<StevenK> henninge: And no, it isn't the same call -- one is *sets*Included, the other is *sources*Included
<henninge> StevenK: ah, now that you mention it ... ;-)
<StevenK> henninge: And yes, relatedSets returns an ordered list
<henninge> cool
<StevenK> Doh! Alphabetical fail
<henninge> Talking to me? ;) I saw that they were alphabetical but did not know if that was just coincidence because you added the entries in that order ...
<StevenK> henninge: Would you like to mark the Status of my MP as Approved, or shall I do it?
<henninge> allenap: Can you please extend DefaultPropertyCache's docstring to point out that getattr and setattr are already implemented by object?
<allenap> henninge: Sure.
<henninge> StevenK: You can do that yourself if you feel that you considered my comments enough ... ;-P
<henninge> StevenK: I never set that when I review. It's for you to decide when your branch has all the reviews it needs.
<henninge> allenap: or a comment
<allenap> henninge: Oh yeah, a comment would make more sense actually.
<henninge> allenap: I am done reviewing and I see what I like.
<henninge> ;-)
<allenap> henninge: \o/ Thanks
<henninge> allenap: since this is a parallel implementation, I don't see a problem with landing it but I ask you to present this on the dev mailing list to see if people see problems with it.
<allenap> henninge: Yeah, good idea.
<henninge> Kind of a after-imp discussion...
<henninge> Cool, r=me ;-)
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: bigjools  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bigjools: your turn ;)
<bigjools> cool
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: bigjools  || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bigjools: Why did you create Test_rescueBuilderIfLost? Could this not have been part of TestBuilder?
<bigjools> henninge: arse - it's because I forgot to remove the layer - it doesn't need any layers
<bigjools> and it's also testing a standalone function
<henninge> bigjools: builder.rescueIfLost? That is a method, not a function. Am I missing something?
<bigjools> henninge: there's builder.rescueIfLost and rescueBuilderIfLost
<bigjools> the former calls the latter
<bigjools> it's a separate function for ease of testing, since it needs mock builders in some tests
<henninge> bigjools: ok, I see.
<henninge> bigjools: r=me ;-)
<bigjools> henninge: actually I think I lied about the layer
<bigjools> but
<bigjools> I remember now I created the extra Test case so that we could add more tests for that function
<bigjools> it's woefully under-tested
<bigjools> henninge: and thanks :)
<henninge> bigjools: welcome ;)
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: -  || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: -  || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi henninge, if you've time before you leave, the MP is: https://code.edge.launchpad.net/~michael.nelson/launchpad/remove-bug-217644-workarounds/+merge/33397
<noodles775> Thanks!
<henninge> noodles775: I am sorry, I will stop reviewing now because I have to take care of something else.
<henninge> I hope abentley will become available soon ...
* henninge changed the topic of #launchpad-reviews to: On call:  - || reviewing: -  || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> henninge: no probs.
<abentley> henninge, I am just back from vacation and catching up on emails, but I can sign on, I guess.
* abentley changed the topic of #launchpad-reviews to: On call:  abentley || reviewing: -  || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> henninge, what's the status of allenap?
<henninge> abentley: that is a second branch that I have not looked at yet.
<abentley> henninge, okay.
<henninge> abentley: I meant this one: https://code.edge.launchpad.net/~allenap/launchpad/refactor-mailnotification/+merge/32923
<henninge> aka "the long one"
<henninge> abentley: the cache-experiment is done.
<abentley> henninge, cool.
<allenap> henninge: If it's too long, please let others jump the queue; it can wait. Also, it's entirely code moves so might not take too long to review.
<henninge> allenap: ;-) It's not much longer than the cache-experiment branch is.
<allenap> henninge: Mmm, yes, that did get a bit big :-/
<abentley> allenap, it looks okay, except that the copyright on date on newbug.py is wrong.
<allenap> abentley: Oops! I'll fix that. Thanks.
<allenap> How did I manage to get 2012 in there?
<abentley> topic On call:  abentley || reviewing: noodles775  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> noodles775, r=me
<noodles775> Thanks Aaron.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> EdwinGrubbs: a while back you reviewed https://code.launchpad.net/~leonardr/lazr.restful/integrate-optimized-len/+merge/33023 for me
<leonardr> you were about to approve it when i asked you to hold off because i'd identified a problem
<leonardr> it turned out the problem had nothing to do with my branch (it was a bug in storm)
<EdwinGrubbs> leonardr: I remember
<leonardr> can you take another look at that branch and approve it?
<EdwinGrubbs> leonardr: sure
<EdwinGrubbs> leonardr: r=me
<leonardr> thanks
<gary_poster> abentley: could you review a super small branch, please?  It just upgrades our zc.buildout and friends to the new releases.  https://code.edge.launchpad.net/~gary/launchpad/buildout1.5.0/+merge/33410
<abentley> gary_poster, sure.
<gary_poster> Thank you
<abentley> gary_poster, r=me
<gary_poster> thanks again
* jcsackett changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> abentley, ping.
<jcsackett> just letting you know, mp is at https://code.edge.launchpad.net/~jcsackett/launchpad/620494-downloads-sort/+merge/33413.
<leonardr> abentley, i have a very small mp at https://code.edge.launchpad.net/~leonardr/launchpad/optimized-length-2/+merge/33417
* leonardr changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [jcsackett,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [jcsackett,leonardr,sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> abentley, I added to the queue. If you do not have time to review my branch, i'll hunt for someone.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: jcsackett || queue: [leonardr,sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jcsackett, when you say "Implementation details: As above", it sounds like "Implementation details: talked with Curtis".  Is that what you mean?
<jcsackett> abentley: no, sorry, as above references Proposed fix.
<jcsackett> bad write up on my part there, sorry.
<jcsackett> i've updated the Description.
<abentley> jcsackett, Cool.
<abentley> jcsackett, ProductReleased.datereleased is a date, not a datetime?
<jcsackett> abentley: the form that is presented when you create one expects datetime, but it appeared only date is used.
<jcsackett> so time always == 0:00:00.
<jcsackett> it is my understanding that Edwin-Grubbs has a better fix rolling out to address the more core issues. this is a bandaid on the edge-case.
<jcsackett> er, EdwinGrubbs, rather.
<abentley> jcsackett, why not change it use the actual time?
<jcsackett> it's my understanding time is not used in the db, and we wanted the bandaid out before the next round of changes from db are rolled out.
<jcsackett> I *believe* that EdwinGrubbs has something dealing with the db issues.
<jcsackett> but it won't be available for a bit.
<abentley> jcsackett, r=me.
<jcsackett> thanks, abentley. :-)
* abentley changed the topic of #launchpad-reviews to:  abentley || reviewing: leonardr || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jcsackett, I think you should add an XXX on the sort explaining that it's a band-aid.  Note our XXX policy: https://dev.launchpad.net/PolicyandProcess/XXXPolicy
* EdwinGrubbs changed the topic of #launchpad-reviews to:  abentley || reviewing: leonardr || queue: [sinzui, Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: I added this branch to the queue. It's easy. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615655-unicode-oops/+merge/33428
<jcsackett> abentley: that's an excellent call. i will do that.
<abentley> leonardr, r=me.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: leonardr || queue: [sinzui, Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: Edwin || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs, in what context is it illegal for an email to have non-ascii characters?
* abentley changed the topic of #launchpad-reviews to: abentley || reviewing: sinzui || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: whenever a different encoding is used, it should be escaped as quoted-printable hence the "=3E" characters seen often, or as binhex.
<abentley> EdwinGrubbs, "should", perhaps, but definitely not "must".  Content-transfer-encoding may be "8-bit" or "binary" for MIME messages in 8-bit encodings.
<EdwinGrubbs> abentley: ok, then I think it is just the headers that are expected to be ascii, which is where the problem is occuring.
<abentley> EdwinGrubbs, yes, those are illegal.
<EdwinGrubbs> abentley: I can change it to just escape the headers then.
<abentley> EdwinGrubbs,  but it appears you're applying the escaping to the entire message, which would actually break valid messages.
<EdwinGrubbs> abentley: yes, I'll have to split the headers and the body.
<abentley> EdwinGrubbs, I recommend tweaking message_from_string if possible.
<EdwinGrubbs> ok
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> abentley, i need a pretty simple review
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/find-length/+merge/33440
<abentley> leonardr, I am good at giving simple reviews :-)
<abentley> leonardr, is this based on your previous branch?
<leonardr> abentley: well, it's related work, but it doesn't depend on the branch you reviewed earlier
<abentley> leonardr, r=me
<james_w> abentley: hi, could you review https://code.edge.launchpad.net/~lamont/launchpad/lp-buildd-70/+merge/33455 which is a merge of lamont's changes back in to launchpad to keep them in sync?
<abentley> james_w, sorry, off for the day.
* abentley changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<james_w> someone else then?
<jelmer> james_w: Yes
<james_w> thanks
<james_w> I think it's a merge of two approved branches, but I need a review in order to ec2 land it
<james_w> and it's backwards anyway, as the code is already in lp-buildd 70, so it's hard to disagree with it :-)
<jelmer> hehe
<jelmer> james_w: r68 mentions reverting all of r68, but I don't see those changes in the diff
<jelmer> (and I don't understand why)
<james_w> jelmer: yeah, that is odd
<jelmer> james_w: see #soyuz
<jelmer> james_w: as lamont has just updated the branch, r=me
<james_w> thanks
<EdwinGrubbs> abentley: you're probably already gone for the day, but I made the changes you suggested if you're still around.
#launchpad-reviews 2010-08-24
<rockstar> thumper, care to do a review for me?
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/lazr-js/overlay-width/+merge/33478
<thumper> rockstar: my head is down right now, how big is it?
<rockstar> thumper, not big, but it adds value...
<rockstar> They're merely CSS changes that fix two bugs.
<rockstar> Also, it'll make it easier to animate the opening and closing of the overlay.
 * thumper looks
<thumper> done
<thumper> rockstar: you around?
<rockstar> thumper, indeed.
<thumper> rockstar: I have a very large but boring review
<rockstar> thumper, :(
<rockstar> thumper, send it on over.  I'll take a look at it in a sec.
<thumper> rockstar: it adds layer to our browser zcml
<thumper> all of the changes are either adding the layer in the zcml
<thumper> or changing tests to show code.lp.dev instead of lp.dev
<rockstar> (I'm currently providing marriage counseling for bzr and savannah)
<thumper> or adding rootsite= to canonical url sites
<thumper> heh
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/fix-browser-zcml/+merge/33482
<mwhudson> 7	 >>> canonical_url(getUtility(IBazaarApplication))
<mwhudson> 8	- u'http://launchpad.dev/+code'
<mwhudson> 9	+ u'http://code.launchpad.dev/+code'
<mwhudson> the +code there is freaking odd really
<mwhudson> i wonder if it would actually be better to have the root objects be different for each publication
<thumper> mwhudson: probably
<thumper> mwhudson: but somewhat out of scope for this change
<rockstar> thumper, holy balls that's a big diff...
<mwhudson> thumper: yes
<thumper> rockstar: but boring
<rockstar> thumper, might I make a suggestion for the next time?
<thumper> yes
<thumper> rockstar: don't do them all at once?
<rockstar> thumper, yes, for the love of Odin, yes.
<thumper> :)
<rockstar> thumper, here's an example: line 1640-1641 of the diff doesn't look to be along the same lines as the rest of the changes in the branch.
<rockstar> Actually, 1635-1665...
<thumper> rockstar: :)
<thumper> yes, I removed a chunk of code that was showing bad data
<thumper> the person that wrote it thought it was showing the number of new revisions this month for that branch
<thumper> but what it was showing was the number of revisions on all branches this month
<rockstar> thumper, the change makes sense.  I just think it would be better to not get lumped in with something that's supposed to be just technical debt.
<rockstar> Is there a bug filed for that change?
<thumper> no
<rockstar> thumper, I think there should be.
 * thumper nods
<rockstar> thumper, other than that, r=me.
<thumper> rockstar: thanks
<rockstar> thumper, no prob.
<rockstar> thumper, do you think it's a sensible suggestion to break up branches like that into smaller branches?
<thumper> rockstar: yeah, probably
<thumper> in hindsight I shouldn't have tried to do all at once
<rockstar> thumper, do you think a 400 line diff limit would have been reasonable in breaking them up?
<rockstar> (I ask because I'm planning on proposing a smaller diff limit in the next reviewers meeting)
<thumper> perhaps 500...
<lifeless> I don't think smaller diffs are really a great need
<lifeless> I think more focused work is great
<lifeless> the -big- thing we had in the path was huge entire projects landing
<lifeless> s/path/past/
<lifeless> ymmv
<jtv> stub: care to review a branch for me?  It's a blocking risk, but not large.  https://code.edge.launchpad.net/~jtv/launchpad/recife-pre-resetCurrentTranslation/+merge/33490
<stub> jtv: Not a blocking risk yet - nothing is using it apart from tests
<jtv> stub: nothing I can _land_ now, no.  Hence: blocking.
<stub> You have an XXX that doesn't cite a bug.
<stub> There are two ways of fixing that.
<jtv> stub: our policy is currently not to file bugs too far in advance.  It's listed in various places though.
<stub> Our policy is also no XXX's without cited bugs IIRC. So remove three letters or create a bug I think are the options.
<jtv> Okay, okay, I'm removing the XXX.
<jtv> Pushing.
<stub> jtv: IPOFile doesn't describe the parameters, which is minor but doesn't hurt to mention translator remains unchanged and timestamp is now by default.
<jtv> Pushed.
<stub> Otherwise all fine.
<jtv> stub: thanks.  I'll update the docstring as you say.
<jtv> Done.
<henninge> jtv: sure, I can ;-)
<jtv> ok, mp'ing now...
<jtv> henninge: https://code.edge.launchpad.net/~jtv/launchpad/recife-retire-traits-helpers/+merge/33491
<henninge> jtv: r=me but please hold back landing until we have that ec2 failure figured out.
<jtv> henninge: ok, thanks
<jtv> henninge: I did just land another small branch on recife
<henninge> oh well ...
<jtv> adeuring, maybe you would like to review this very small and hopefully fun branch?  https://code.edge.launchpad.net/~jtv/launchpad/fakelibrarian-commit/+merge/33322
<adeuring> jtv: sure, I'll look
<lifeless> (I suggested the approach, I think, if that helps)
<jtv> adeuring: it allows (some) tests that need the librarian to get by without database commits
<jtv> lifeless: I ended up going with the "just slap a method on the fakelibrarian class" approach.  Not exactly future-proof but effective for the time being.
<lifeless> jtv: I hear we can change code in the future ;)
<jtv> The future's just crazy.
<adeuring> jtv: r=me.
<jtv> adeuring: great, thanks
<StevenK> adeuring: Would you like a review a small branch of mine too?
<adeuring> StevenK: If it's a _small_ one, sure ;)
<StevenK> adeuring: Is 144 lines too bug?
<StevenK> s/bug/big/
<adeuring> OK, I'll look at it
<StevenK> adeuring: No news is good news? :-)
<adeuring> StevenK: yes, r=me
<StevenK> adeuring: Thanks!
<jtv1> gmb: congratulations Recipient!  Fate has drawn your name.  Today could be the day, dear Recipient, that you review jtv's branch.  Collect your prize here: https://code.edge.launchpad.net/~jtv/launchpad/recife-check-conflicts/+merge/33494
<StevenK> Bwahaha
<jtv> Don't laughâif it works for the direct marketers, why shouldn't it work for me?
<lifeless> because they try 10K for every bite
<lifeless> and if you do that in this channel... it will end badly
<gmb> jtv, Sorry, wasn't ignoring your ping; have had to deal with home things. I'll be starting my shift in ~45 minutes, so I'll look then. Don't worry if your day ends before then, though.
<jtv> gmb: no worriesâactually I wasn't even aware that today was your shift.  :-)
<jtv> Just trying someone at random.
<jtv> Fate picked another winner, Recipient!
<gmb> :)
 * gmb goes to finish pre-shift stuff
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: if you had any questions pent up, I'm back
<gmb> jtv, No questions; it's r=me :)
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: Thank you, Recipient, for your interest in our product!
<gmb> :)
<jtv> Hmmâ¦ this tack seems to work.  I should do it more often.
<gmb> jtv, You need to be careful on whom you try it. See, me, I'm polite and friendly to all comers. But if you ask the wrong person they'll try to /kick you before you get to finish your pitch.
<jtv> gmb: I'm not worriedâ¦ people know me here
<jtv> â¦hey, what happened?
<gmb> Heh.
<jtv> Okay, maybe I won't try that again then.  :-)
<jtv> gmb: if you're not busy, I've got another small one in the backlog here.  Converts a test to use the FakeLibrarian (speeding it up by about 20%).  https://code.edge.launchpad.net/~jtv/launchpad/use-fakelibrarian/+merge/33324
<gmb> jtv, Sure
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Thanks Rec^H^H^Hgmb!
<gmb> jtv, r=me
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: thanks once more
<gmb> np
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: vittles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb, can I chuck a modest one on the queue?
<gmb> jtv, Sure. I'll take a look in about 20 minutes
<jtv> gmb: splendid
* jtv changed the topic of #launchpad-reviews to: On call: gmb || reviewing: vittles || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jtv, Is there any reason that the new pagetest, which isn't really a user story, just a set of tests in doctest form, can't be written as a unittest?
<gmb> That would fit better with our policy of not adding stories and doctests where they aren't needed.
<jtv> gmb: mostly just laziness on my part tbhâ¦ I think you're right.
 * jtv twiddles
<jtv> gmb: this will take a bit of course
<gmb> jtv, No problem. I'll happily re-review when it's done.
<jtv> thanks!
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jtv, Other than that it's absolutely fine.
<jtv> \o/
<jtv> Of course "other than that" is the remaining 2 lines or so of change.  But still, nice to hear.  :)
<jtv> gmb: I have the test converted and, in the process, found intimately related "stories" tests that I could move over into the same unit tests.  Pushing.
<gmb> jtv, Cool.
 * gmb grabs a drink before looking
<gmb> jtv, I wouldn't object if you removed the VWS from those tests, but that's a preference thing. r=me either way.
<gmb> Nice work :)
<jtv> thanks!  <beam>
<jtv> I like blank lines to separate setup, execute, verify stages.  But yes, in this case there's not much point.  I'll try it on for size.
<jtv> Yup, shorter works better in this case.
<leonardr> gmb, i need a follow-up review of https://code.edge.launchpad.net/~leonardr/launchpad/optimized-length-2/+merge/33417 (the latest revision only)
<leonardr> it's a little complicated so i'm happy to ask questions
<gmb> leonardr, Okay, just OTP at the moment; will look shortly.
<leonardr> np
<gmb> leonardr, Looking nwo.
<gmb> leonardr, So, is it just r11392 that I need to look at?
<leonardr> gmb, right
<gmb> Righto
<gmb> leonardr, Nice XXX explaining the problem.
<gmb> leonardr, No further explanations needed; r=me.
<leonardr> cool
<leonardr> we'll see if the tests pass
<EdwinGrubbs> abentley: when you have time, can you review the changes I made to my branch?  https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615655-unicode-oops/+merge/33428
<abentley> EdwinGrubbs, I am tempted to think that your binary header handling should still be performed in message_from_string, so that it is easier for others to reuse.
<abentley> EdwinGrubbs, btw, 5.5% of the messages in my inbox use "8bit" or "binary" content-transfer-encoding.
<abentley> EdwinGrubbs, also, the standard mandates CRLF, not LF or CR as the header separators.  Do you know whether the existing parser treats CR and LF as equivalent to CRLF?
<EdwinGrubbs> abentley: is there a different message_from_string() function? I only see the one on the email module.
<abentley> EdwinGrubbs, I may be confused.  There's one for creating a SignedMessage from a string, I believe.
<EdwinGrubbs> abentley: yes, the email module treats all three different line endings as possibly splitting the header and body.
<abentley> EdwinGrubbs, okay, I think it would be nice to provide this as a utility function in lp.services.mail, but I won't require it.  r=me.
<EdwinGrubbs> abentley: thanks
<jml> simple little patch: https://code.edge.launchpad.net/~jml/launchpad/subject-and-attachment/+merge/33544
<jtv> gmb: one more tiiiiny branch?  https://code.edge.launchpad.net/~jtv/launchpad/bug-615673/+merge/33547
<gmb> jml, Looking now
<gmb> jtv, You're up next.
<gmb> First and last customer for the day :)
<jtv> cool!
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jml || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> gmb: thanks.
<gmb> jml, Approved.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jtv, r=me.
 * gmb is done
* gmb changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: thanks again.  You have earned your rest.
<jml> Another small one: https://code.edge.launchpad.net/~jml/launchpad/testtools-0.9.6/+merge/33556
<jml> Yet another small patch: https://code.edge.launchpad.net/~jml/launchpad/launchpad-tester/+merge/33564
#launchpad-reviews 2010-08-25
* StevenK changed the topic of #launchpad-reviews to: On call: StevenK || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * mwhudson waves https://code.edge.launchpad.net/~mwhudson/launchpad/move-SpecificationDepCandidatesVocabulary/+merge/33611 around if anyone is free
<mwhudson> it's only waffer thin
<StevenK> mwhudson: 613 lines is waffer thin? :-)
<mwhudson> StevenK: it's mostly just moving code around
<StevenK> mwhudson: It looks fine to me, but I'd also need thumper to look at it, and I think he's afk, so you can beg jtv_ if you'd like to land it soonish
<jtv_> Do I hear a grovel approaching?
<mwhudson> jtv_: if you could take a look
<mwhudson> i'm not in a massive hurry
<jtv> mwhudson: btw if you want a review, do try naming the OCRs so they know there's customers!
<jtv> mwhudson: I'll take it.
<jtv> StevenK: I worked through the overnight backlog.
<jtv> Forgot to update the topic though.
<jtv> On call: StevenK, jtv || reviewing: -,mwhudson || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> I meanâ¦
* jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || reviewing: -,mwhudson || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mwhudson> jtv: i never know who's on call, even if it says so in the /topic :-)
<mwhudson> jtv: thanks a lot
<jtv> mwhudson: why is the copyright on lib/lp/blueprints/vocabularies/configure.zcml dated 2009?
<mwhudson> jtv: you get one guess
<jtv> "I copied it from a file that said 2009"?
<mwhudson> jtv: bingo
<mwhudson> fixed
<jtv> What do I win?
<mwhudson> jtv: are you going to UDS-N?
<jtv> No
<mwhudson> mm
<mwhudson> will 'a beverage next time we meet' do?
<jtv> absolutely.
<mwhudson> deal
<jtv> *grin*
<mwhudson> jtv: are you going to be done soon?
<jtv> mwhudson: _filter_specs could do with a brief docstring.
<jtv> mwhudson: I think so, yes
<mwhudson> don't want to rush you, just want to know if to hang on here for another few minutes
<jtv> mwhudson: won't be long.
<mwhudson> ok
<mwhudson> jtv: yes, i wonder what that function does...
<jtv> In _doSearch, you may want to use "%(query)s, %(query)s, %(query)s" % {'query': quoted_query}
<mwhudson> jtv: http://pastebin.ubuntu.com/483286/
<jtv> mwhudson: Just a formality.  Its name also doesn't follow the guidelines, and the code is mis-formattedâ¦ I'm not planning to be too difficult about code you only moved though.
<mwhudson> jtv: i don't want to touch the code
<mwhudson> it will probably all get thrown away in the next branch anyway
<jtv> mwhudson: that's actually helpful, thanks.
<mwhudson> jtv: sorry for not saying so in the merge proposal then
<jtv> mwhudson: what I don't get isâ¦  I _think_ this is code you're moving, but I only see it added.  Aren't you also removing it somewhere else?
<mwhudson> jtv: haha, it's _supposed_ to be removed from dbobjects
<mwhudson> jtv: but clearly that was a different branch :(
<mwhudson> jtv: need to run for a bus, can you comment on the mp and i'll sort it tomorrow
<jtv> OK
<mwhudson> actually...
<mwhudson> pushing a fix
<jtv> get that bus!
<mwhudson> but now running
* StevenK changed the topic of #launchpad-reviews to: On call: jtv || reviewing: mwhudson || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> StevenK: bowing out?  Good night then!
<StevenK> jtv: From OCR at least :-)
<jtv> ah ok
<StevenK> jtv: Going to be in and out until the call, so no point people pinging me if I'm afk
* jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> lifeless: or maybe this is something you can approve for CP?  https://code.launchpad.net/~jtv/launchpad/bug-623865/+merge/33617
<jtv> (it can wait for danilo to come back, but I'm eager to shave off a few more seconds :)
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> bigjools, reviewing your big branch...
<bac> hi jelmer, may i get a review?
<jelmer> bac: Hi!
<jelmer> bac: Yes, of course.
<bac> jelmer: thanks.  https://code.edge.launchpad.net/~bac/launchpad/bug-618148/+merge/33600
* bac changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> jml: it's WIP
<bigjools> so don't
<jml> bigjools, too late.
<jml> bigjools, it sucks a little that I couldn't have found that out over email.
<bigjools> jml: the status is WIP!
<jml> bigjools, not mentioned in the email
<bigjools> assuming you're looking at my soyuz-enums branch?
<jml> bigjools, yes, I was.
<bigjools> I created it with the "ready for review" unchecked
<bigjools> so if it's sending email, that's a bug
<jml> bigjools, surely.
<bigjools> in any case I was going to merge it rs=whoevcer
<bigjools> since it's a purely mechanical change
<bigjools> I'm sorry that you started reviewing it :(
<jml> fair enough.
<jml> that's ok
<jml> I finished reviewing it too.
<bigjools> !!!!!!
<bigjools> it's 5k lines!
<jml> 22 minutes ago.
<bigjools> it's not finished yet either :)
<jml> bigjools, yes, but it doesn't take very long to look at a changed import and decide that it's fundamentally sound :)
<bigjools> :)
<bigjools> jml: you can help me though if you have 5 mins
<jml> bigjools, I do.
<jml> I also have half a beef sandwich
<bigjools> jml: I've got one failure left
<bigjools> lp.services.scripts.tests.test_all_scripts.ScriptsTestCase.test_scripts
 * bigjools pastes
<bigjools> jml: http://pastebin.ubuntu.com/483416/
<jml> I hate Python. :(
<bigjools> there's 2 failures, but surprisingly to me the import at the bottom is one I added to get around circular imports elsewhere :/
<bigjools> In Soviet Russia, Python hates you.
<jml> bigjools, perhaps that workaround is hurting now?
<jml> bigjools, Python hates me here in big society Britain.
<bigjools> jml: I dunno what else to do.  It doesn't have any hearing on the other error though.
<jml> bigjools, is ALLOW_RELEASE_BUILDS in the wrong __all__, perhaps.
<bigjools> I'd love it if we actually had a big society :)
<jml> bigjools, Python would still hate me.
<bigjools> jml: ALLOW_RELEASE_BUILDS is in the right place
 * jml fetches the branch.
<jml> I always forget if you separate your names with - or .
<allenap> Does anyone want to give me a trivial for http://paste.ubuntu.com/483422/? I'll land it as part of lp:~allenap/launchpad/cache-experiment-roll-out.
<jml> oh, I see.
<jml> bigjools, test_all_scripts assembles one big error. That's confusing.
<deryck> allenap, I can.  What does the extra +f line do?
<jml> although arguably justifiable.
<allenap> deryck: It includes the base-name of each file in the tags table. So if you search for a tag like "shipit.py", you'll get the file. Works nicely with Emacs, but I'm not sure about Vim.
<bigjools> jml: doesn't help me much :(
<allenap> deryck: The --extra line might be better in my personal .ctags.
<bigjools> jml: I wonder how many of our scripts really need to be tested like this
<deryck> allenap, doesn't sound contraversial, though.  I've never used that, but we can drop it later if it doesn't play well with vim.
<jml> bigjools, it's a sanity check, that's all.
<deryck> allenap, r=me
<jml> hmm. given that ./bin/py ./scripts/gina.py -h works, the failure seems bogus. let me try running the actual test locally.
<allenap> deryck: Okay, thanks :)
<bigjools> jml: seems unnecessarily slow
<bigjools> we should do them all in test_scripts, or all in their own test cases
<jml> bigjools, gina -h, or the test?
<bigjools> jml: test_scripts I mean
<bigjools> it's duplicating tests, and Popen on zopeless scripts is not exactly quick :/
<jml> bigjools, it's running every script, and each script is quite slow (just try running gina.py -h!)
<bigjools> my point.
<jml> bigjools, at the very least, the test could be rewritten to make one test case per script
<jml> that would make it interact more nicely with the test runner.
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> this test ensaddens me.
<jml> bigjools, there's precedent in lp.services.scripts.tests for marking a script as "known broken" because of circular imports.
<bigjools> jml: where?
<jml> bigjools, luckily, software is built on a common law system of jurisprudence, so I can recommend that we happily ignore it.
<jml> bigjools, lib/lp/services/scripts/tests/__init__.py
<bigjools> jml: the point is that this test is shit, it's failing unnecessarily.
<jml> yes.
<bigjools> the script works IRL
<jml> I wonder what's causing it to fail
<jml> I *think* the answer is that it's not using ./bin/py
<bigjools> interesting
<jml> "./bin/py cronscripts/ppa-generate-keys.py -h" *does* fail, however.
<bigjools> sigh
<jml> which reduces my confidence in the bin/py guess.
<bigjools> it fails w/o bin/py too
<bigjools> FPOS circular import shit
<jml> yes.
<jelmer> bac: Did you see StevenK's comment on your branch?
<bac> jelmer: i did not.  /me assumed no one had looked at it.
 * bac looks
<bac> jelmer: so, never mind.
<jml> bigjools, I'll keep trying to figure out why gina is failing.
<bigjools> jml: ok thank you.  I'll do the same import trick on the other script too
<bigjools> after I fix bug 623859 anyway
<_mup_> Bug #623859: edge rollouts broke CSS on revno 11435 <canonical-losa-lp> <Soyuz:In Progress by julian-edwards> <https://launchpad.net/bugs/623859>
<jml> that's the second edge rollout broken by CSS in two weeks.
<jml> what's causing them?
<StevenK> jml: Er, why is gina failing?
<jml> StevenK, that's the question.
<StevenK> jml: I have a branch ready to land that changes gina
<jml> StevenK, it's to do with bigjools's soyuz-enums branch
<StevenK> Ahh
<bigjools> huh?
<bigjools> jml: that bug is a missing future import for "with"
<bigjools> but the "with" has been around since revno 7xxx
<jml> bigjools, what?
<jml> bigjools, oh sorry, too many conversations :)
<bigjools> the bug above
<bigjools> :)
<jml> I quite like the new ec2 mail subjects, if I do say so myself.
 * bigjools wouldn't know
<jml> bigjools, http://pastebin.ubuntu.com/483446/ makes test_all_scripts have one test per script
<bigjools> jml: I'll try it out in a sec
<bigjools> jml: jeebus, that test_scripts takes nearly 4 minutes to finish
<bigjools> trying out your patch now
<bac> hi henninge
<bac> henninge: where does your super import tool live?  i'd like to reference it in the wiki.
<bigjools> jml: nice chance, it's good to see progress through the scripts.
<bigjools> still doesn't help me :)
<jml> bigjools, it helps me help you.
<bigjools> heh
<bigjools> jml: if I run bin/py gina/py it works, running it standalone doesn't
<bigjools> gina.py even
<bigjools> jml: argh I can see the problem
 * bigjools fixes it
<bigjools> can someone approve this one-liner to restore edge rollouts please: https://code.edge.launchpad.net/~julian-edwards/launchpad/bug-623859/+merge/33640
<rockstar> bigjools, r=me
<bigjools> thanks rockstar
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: bac || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi Guest48768, if you've time, the MP is: https://code.edge.launchpad.net/~michael.nelson/launchpad/remove-shortlist-getPublishedReleases/+merge/33645
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: bac || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<Guest48768> noodles775: Yep, sure - will have a look.
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> noodles775: r=me, thanks for the cleanups!
<noodles775> Thanks jelmer
<sinzui> deryck, do you have time to look at the changes I made to script permissions in https://code.edge.launchpad.net/~sinzui/launchpad/dsp-bug-counts-1/+merge/33257 ? I think it needs your (or gmb's) judicious opinion to land
<deryck> sinzui, sure.  Looking now....
<deryck> sinzui, so I'm combing the permissions now, since that's your main concern.... but one thought, do you need to keep the call to recalculateBugHeatCache in setHeat?  Or is doing it in updateHeat enough now?
<sinzui> SetHeat is still being called on new task, dupe and new bug
<sinzui> Dup certainly needs to remain
<deryck> ok, I think it's fine then.
<bac> hi EdwinGrubbs.  i've got a MP that was reviewed by stevenk who is mentored by thumper.  i'd really not like to wait until thumper re-appears to land it.  would you mind reviewing/mentoring it?
<deryck> sinzui, the permission changes seem pretty safe to me.  Why do you hesitate with them?
<sinzui> I think it is odd to update distros
<deryck> yeah, but it's only for heat.  You're just worried that allowing update leads to other stuff later without us catching it?
<deryck> sinzui, is that last statement of mine correct about your concern? ^^
<sinzui> deryck, indeed I am paranoid
<sinzui> deryck, I am confident of our scripts
<deryck> sinzui, ok.  I think your changes are fine.  I see your concern, but I think the cost of the paranoia is quite high, and there are other checks to ensure we don't do something we shouldn't here.
<sinzui> indeed
<deryck> sinzui, so r=me.  Updating MP now...
<sinzui> thanks
<deryck> np
<EdwinGrubbs> bac: sure, what's the url for the mp?
<bac> EdwinGrubbs: https://code.edge.launchpad.net/~bac/launchpad/bug-618148/+merge/33600
<bac> EdwinGrubbs: i'm going to lunch now, though
<EdwinGrubbs> that's fine
<benji> jelmer/Edwin: is that an open review slot I see?  how about we fill it with https://code.edge.launchpad.net/~benji/launchpad/check-in-wadl/+merge/33661
<jelmer> benji: Yes, though I should note I have some concerns about your music suggestions...
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: benji, - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> heh
<gmb> allenap, I've finally made changes based on your review in https://code.edge.launchpad.net/~gmb/launchpad/dont-piss-the-losas-off-bug-596877/+merge/30186; if you could check that I'm doing TRT that would be great.
 * allenap looks
<jelmer> benji: buildout.cfg has a reference to your homedir :-)
<benji> jelmer: oh yeah, I was using a not-publicly-available version of lazr.restful; that should affect anything; I'll remove it and double-check
<benji> s/should/should not/
<jelmer> benji: Thanks
<jelmer> benji: Your MP description mentions tests that compare the stored data to freshly generated data; I can't find them in the branch though, have they already landed?
<benji> jelmer: lib/canonical/launchpad/ftests/test_checked_in_wadl.py
<gmb> allenap, http://bazaar.launchpad.net/~gmb/launchpad/dont-piss-the-losas-off-bug-596877/revision/11154 Specifically; I'll paste a diff that makes it a bit more obvious.
<jelmer> benji: Sorry, my bad. I just noticed the diff on lp has been truncated.
<benji> yeah, it was a monster
<benji> (because of the new, generated files)
<benji> jelmer: the now-without-a-homedir-reference version has been pushed
<jelmer> benji: is there a particular reason this needs a newer lazr.restful?
<allenap> gmb: I've commented. Looks good :)
<gmb> allenap, Ta.
<benji> jelmer: yep; the WADL generated by the older lazr.restful wasn't deterministic, so the tests would fail intermittently
<gmb> allenap, I'll make those changes. Can you give it an r= so that I can land it when I'm done?
<allenap> gmb: Sure :)
<gmb> Ta.
<gmb> allenap, Although, I don't think the last change you suggest is necessary TRT to do; are you okay if I leave that as-is? I don't see us changing that permission (and even if we did, our tests would pick it up).
<allenap> gmb: Yeah I'm happy :)
<gmb> Kewl
 * benji grabs lunch
<jelmer> do we allow using the "assert" statement in unit tests as part of the assertions of an individual test? It looks wrong to me, but the style guide doesn't forbid it.
<gmb> jelmer, I can't answer that without more context (I'm inclined to agree it's not right, but I'd like to see an example)
<jelmer> gmb: http://bazaar.launchpad.net/~benji/launchpad/check-in-wadl/annotate/head:/lib/canonical/launchpad/ftests/test_checked_in_wadl.py
<jelmer> gmb: line 87
 * gmb looks
<benji-lunch> jelmer and gmb: in that case it was an assert that the *code* of the test is correct, much like an assert in "normal" code
<benji-lunch> I don't really mind changing it to a test assertion but my intent was to communicate that the assert wasn't about the test but about the workings of the test itself
<gmb> benji-lunch, jelmer: Yeah, that's one that's purely reviewer discretion. Basically, whatever jelmer prefers to see ;)
<jelmer> benji-lunch: I hadn't looked at it that way; makes sense.
<jelmer> gmb: Thanks for having a look.
<gmb> np
<jelmer> abentley: did you notice your mp has conflicts?
<abentley> jelmer, yes.
* jelmer changed the topic of #launchpad-reviews to: On call: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> abentley: ok, just checking
* sinzui changed the topic of #launchpad-reviews to: On call: Edwin || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs, I have a branch ready for review if you have time today.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: bac || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: I can do it after I finish bac's branch.
<sinzui> thanks
<EdwinGrubbs> bac: it looks like your field.description.replace('pillar', 'project') code is actually changing the IServiceUsage interface, which would be bad if that is ever used for projectgroups and distros.
<bac> EdwinGrubbs: really?
<bac> yes, would be bad
<EdwinGrubbs> bac: you can use pdb to check that, but I believe you would want to use the copy_field() function to get a field you can modify.
<bac> EdwinGrubbs: i will look in pdb now
<EdwinGrubbs> bac: it doesn't look like you pushed up the latest changes to your branch. After you're done looking at the field, can you do that?
<EdwinGrubbs> bac: BTW, I also think that you should change field_names into a @property, so that it is easier to override that if necessary. Right now, setUpFields would overwrite anything that a subclass tried to add to field_names.
<bac> EdwinGrubbs: ok
<abentley> rockstar, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/no-chroot-builder/+merge/33674
<bac> EdwinGrubbs: you were correct about the interface description being modified in place.  good catch.
<james_w> abentley: with that change it looks like sudo doesn't need to be installed in the chroot anymore?
<bac> EdwinGrubbs: i've made those changes and updated the MP
<abentley> james_w, good catch.  I'll confirm that.
<james_w> abentley: though maybe the pbuilder installation of build-deps needs it
<james_w> I can't see that it does here, but I don't know about all series
<james_w> I think it expects to be run as root, so we should be ok
<abentley> james_w, and because the satisfydepends script's requirements aren't considered, the pbuilder package will not have a dependency on sudo...
<james_w> right, but I don't think that it uses it
<abentley> james_w, not on lucid, at least.
<abentley> james_w, I'd like to add a "--safe" option to "dailydeb" that will make it choke on "run" and anything else that could produce arbitrary code execution.  What do you think?
<james_w> abentley: at execution time?
<abentley> james_w, yes.
<james_w> abentley: belt and braces?
<james_w> as I added a way for LP to reject it at parse time as well
<abentley> james_w, exactly.
<james_w> I'm fine with that
<abentley> james_w, ? we already reject it at parse time.
<james_w> abentley: yes, but this was at rockstar's request, to do it in a cleaner fashion
<abentley> james_w, oh, cool.
<james_w> not meaning to say that you didn't, just making sure it was in addition, not instead of
<james_w> --safe will be easy to add though
<james_w> you can file a bug and I will get to it eventually, or you could add it easily if you like
<EdwinGrubbs> bac: r=me
<bac> thanks EdwinGrubbs
<abentley> james_w, I would prefer a list of permitted instructions instead of a list of forbidden instructions.  You fail safe that way.
<james_w> abentley: true, but requires more work for LP :-)
<james_w> easy enough to change
<bac> EdwinGrubbs: could i get you to mark the MP?
<james_w> abentley: bzr says that sudo has never been used by a satisfydepends script
<EdwinGrubbs> bac: sure, I was just writing up some comments for Steve also.
<bac> EdwinGrubbs: great
<abentley> james_w, you mean, except for satisfydepends-experimental?
<james_w> abentley: in a comment?
<abentley> james_w, oh, didn't see that.  Cool.
<abentley> james_w, builder trunk has a test failure: http://pastebin.ubuntu.com/483595/
<james_w> abentley: hmm, that depends on the version of python-debian apparently
<james_w> abentley: maybe make it .startswith, or generate that exception ourselves and put it in to the string we are asserting against?
<abentley> james_w, if that message is externally-generated, I think it makes sense to leave it alone.
<EdwinGrubbs> sinzui: do you want me to review official-services or mailing-list-subscribers-cache first?
<james_w> well, the test was to check that it included the str(e) of the exception from python-debian, but it's brittle as written. I don't think it's too important, so either way of avoiding hardcoding it is fine by me.
<sinzui> official-services plaase
<bac> EdwinGrubbs: time for a super-easy one?
<bac> sinzui: or, perhaps you could look at it?  https://code.edge.launchpad.net/~bac/launchpad/bug-429248/+merge/33684
<EdwinGrubbs> that would be better, since I should review sinzui's first, although he doesn't seem to be around.
<sinzui> I am
<sinzui> EdwinGrubbs, oh, I forgot to include your name in my answer. official-services is the one I really want reviewed first
<EdwinGrubbs> oh
<bac> sinzui: can you glance at my branch?
<sinzui> yep
<bac> thanks
<sinzui> we could trade, I have a trival fix too
<sinzui> trivial cache
<bac> sinzui: gladly
<sinzui> bac: https://code.launchpad.net/~sinzui/launchpad/mailing-list-subscribers-cache-0/+merge/33681
<sinzui> bac: r=me
<bac> sinzui: that's not fair.  i'm still reading your description.
<sinzui> the diff is shorter too
<bac> only b/c i'm so conscientious and fixed the lint stuff
<benji> jelmer: I just noted on https://code.edge.launchpad.net/~benji/launchpad/check-in-wadl/+merge/33661 that your two review requests have been done.
<benji> mmm, I think jelmer's not home any more
<EdwinGrubbs> sinzui: I don't see why you create the selected_template property that returns self.template instead of just turning self.template into a property. That should eliminate the need to define render().
<sinzui> EdwinGrubbs, yes. I considered that. I decided that most engineers are not aware of that. That take render for granted. I decided to create selected_template to be clear that something special is going on
<sinzui> s/that take/they take/
<sinzui> EdwinGrubbs, by example, when XRDS breaks the person page, I am the person who has to explain how...it is not obvious that render() is redefined
<EdwinGrubbs> sinzui: ok, then I think you should rename self.template, so that programmers who are aware of that aren't confused that it is not being used implicitly by render().
<EdwinGrubbs> sinzui: I can't remember what XRDS stands for.
<sinzui> me neither
<sinzui> self.template cannot be a property (I tried it). The call modifies the base class in an od way
<benji> "eXtensible Resource Descriptor Sequence"?
<sinzui> EdwinGrubbs,  the choice was to modify render() to do the work quietly or try a more explicit approach
<sinzui> EdwinGrubbs, If you really want, I can make render do the work and add comments warning future developers to that they need to understand the base class
<EdwinGrubbs> sinzui: no, I don't want you to move the logic into render(). It is just suprising to see selected_template used like self.template, and self.template not used like template normallly is.
 * sinzui nods
<jcsackett> EdwinGrubbs: are you going to have time for another review today (it's pretty big) or should i just hunt someone down tomorrow?
<EdwinGrubbs> jcsackett: I might be able to start on it today.
<sinzui> template cannot be a property so I chose selected template. I can add a comment about the decision in selected_template
<jcsackett> EdwinGrubbs: okay, i'll add myself to the queue.
* jcsackett changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: bac || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: really? Why can't template be a property?
<jcsackett> EdwinGrubbs: https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-official_malone/+merge/33694
<sinzui> Calling ViewPageTemplateFile does magic
<jcsackett> if you can get to it, thanks in advance. :-)
<sinzui> EdwinGrubbs, ^
<sinzui> Lots of tests broken when made a property of it. It seemed like the obvious course at the time. And a nasty surprise afterwards
* benji changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: bac || queue: [sinzui, jcsackett, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: I was confused by what ViewPageTemplateFile would be doing. I tried looking at the source code, which is a mess, so I just made the changes to see what error it would give. All the tests passed with  SearchQuestionsView.template as a property.
<sinzui> I am confused
<sinzui> I will certainly do that if all answers passes
<EdwinGrubbs> bac: ping
<bac> EdwinGrubbs: yes?
<EdwinGrubbs> bac: am I remembering correctly that all lists should be formatted vertically? Not just the recent import statement change.
<bac> EdwinGrubbs: yes
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: bac || queue: [sinzui, jcsackett, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: review sent
<sinzui> Thanks EdwinGrubbs
* benji changed the topic of #launchpad-reviews to: On call: - || reviewing: bac || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-08-26
<jtv> thumper: on what server are ITipChanged events fired and handled?
<thumper> jtv: only within the branch scanner process
<jtv> So that's where the buildfarmjobs all get created then, I guess.
<jtv> What server does that run on?
<jtv> Is that codehosting, or the script server?
<jtv> thumper: (I need to know for a CP request)
<jtv> thumper: I can guess it's codehosting, but this isn't a moment for guessing.  :)
<thumper> loganberry
<jtv> So the script server!  Thanks.
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary/+merge/33728
<mwhudson> thumper, jtv: can you take a look at https://code.edge.launchpad.net/~mwhudson/launchpad/initialize-bzrlib-in-ec2-bug-624434/+merge/33730 ?
 * thumper looks
 * jtv goes off to look
<thumper> why does it still say updating diff?
<jtv> thumper: it's been doing that latelyâ¦ for perhaps two hours at a time.
<thumper> seems screwy
 * thumper wonders if there is a bug filed
<jtv> mwhudson: is with still future?
<thumper> jtv: it is for 2.5
<mwhudson> jtv: don't we still deploy on 2.5 ?
<mwhudson> for this specific file i guess it doesn't matter
<mwhudson> thumper: it doesn't say updating diff for me
<jtv> We still have stuff on dapper afaik, so that makes sense
<thumper> replication lag?
<jtv> mwhudson: it's gone now
<mwhudson> thumper: ta
<jtv> thumper already approved it?
<jtv> ah yes
<jtv> mwhudson: under what circumstances does one need to initialize bzrlib?  I'm wondering if we need to do the same in our buildfarm jobs.
<mwhudson> jtv: we probably should, yeah
<mwhudson> i don't think it's a specific requirement yet
<jtv> yet?
<mwhudson> well
<mwhudson> i suspect as bzr changes, it will become more necessary
<mwhudson> or less necessary, if the people with pitchforks win out i guess
<jtv> heh
 * jtv imagines a tough Hudson going "what kind of idiot brings a pitchfork to a flamewar"
<lifeless> well right now if you don't it won't log properly
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> noodles775, got one for ya: https://code.edge.launchpad.net/~jtv/launchpad/recife-new-resetcurrenttranslation/+merge/33747
<jtv> I warn you, not for the faint of heart.
<jtv> Not for the code per se; more for the murder mystery.
<jtv> â¦and having thus ensured rapt attention from my potential reviewer, I trundle off to other work.
<noodles775> :)
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> oh, hi benji!
<jtv> did I mention you really did free us of our one daily-recurring oops?
<benji> jtv: awesome!
<jtv> very
<benji> jelmer: for some reason ec2 land doesn't think you approved https://code.edge.launchpad.net/~benji/launchpad/check-in-wadl/+merge/33661: "ec2: ERROR: Merge proposal is not approved. Get it approved, or use --force to land it without approval."
<benji> I'm about to go AFK, I suppose I'll use --force when I get back if you don't have a better idea.  Thanks.
<jelmer> benji: Sorry, I had voted approved but not marked the whole MP as approved. Done now.
<noodles775> jtv: on line 288 of the MP diff, why do you remove the assertion?
<jtv> noodles775: not removing itâthat's just an awkward slicing of the diff.  Just replacing it with a clearer one: is_diverged.
<jtv> That's a property that returns "potemplate is not None"
<noodles775> Right, thanks.
<salgado> jtv, around?  did you see my reply to those two reviews you did for me yesterday?
<jtv> salgado: ah, I knew there was something I wanted to check up onâ¦
<jtv> salgado: ah, and of course I never saw the most important part in my email.
<salgado> :)
<jtv> That's why your later follow-up didn't make any sense to me.  I never saw the part it was a follow-up to.
<jtv> salgado: I _would_ be looking at your additional revisions at this point if codebrowse were willing to show them to me.
<salgado> jtv, I can generate the incr. diffs here if you'd like
<jtv> salgado: nah, at some point it'll start working again, I know it
<jtv> See?  I've already gone from "Please try again" to "This website is not available."
<jtv> Erâ¦ hang on
<jtv> salgado: got 'em.  Some pretty big changes there; I won't be able to review them tonight.  Will tomorrow do?
<jelmer> noodles775: Can I add a trivial soyuz branch to the queue ?
<salgado> jtv, big changes?  which ones?  I thought they were all small-ish
<jelmer> StevenK: did you see my followup to the merge proposals you reviewed the other day?
<jtv> salgado: isn't this one of your post-review changes?  http://bazaar.launchpad.net/~salgado/launchpad/remove-security-upload-policy/revision/11427
<salgado> jtv, no, that's the initial diff you've reviewed
<jtv> ah good
<jtv> (before you say it: no, I did _not_ bother to read it :)
<salgado> http://bazaar.launchpad.net/~salgado/launchpad/remove-security-upload-policy/revision/11428
<salgado> that's the one you haven't seen yet
<salgado> jtv, 11427 is the mostly red diff, where I removed tons of stuff
<jtv> salgado: 11428 is still giving me about 7 pages.  Dinner just arrived; I'll look at it tomorrow.
<jtv> The code, not the dinner.
<salgado> jtv, do you mind if I get someone to review these other bits, then?
<jtv> salgado: not at all.
<salgado> cool, I'll do that, then
<jtv> ok
<salgado> hi noodles775.  care to do a couple follow up reviews for me?  they're both quite simple
<noodles775> salgado: sure!
<salgado> cool, thanks
* salgado changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jtv || queue: [salgado*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jelmer || queue: [salgado*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> noodles775: the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/remove-archiveuploader-utils/+merge/33764
<noodles775> jelmer: yep, just merged it (for some reason the diff not yet generated on the MP)
<noodles775> jelmer: r=me :) Nice and easy.
<jelmer> noodles775: thanks :-)
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: salgado || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> noodles775, so, I think it's a good idea to take https://code.edge.launchpad.net/~salgado/launchpad/remove-security-upload-policy/+merge/33581 first
<noodles775> k, thanks.
<leonardr> noodles775, i need a one-line review, can i sneak in before salgado's next review?
<salgado> jtv's reviewed it, but there were test failures on the revision he reviewed
<noodles775> leonardr: sure, if it really is a one-liner ;)
<leonardr> it is
<leonardr> noodles, take a look at http://bazaar.launchpad.net/~leonardr/launchpad/optimized-length-2/revision/11392 which is my last reviewed code for this branch
<leonardr> there's some code that sets resultset._select.*
<leonardr> the tests failed because not all ResultSet objects have ._select, only the Storm ones
<leonardr> so i added this line:
<leonardr> if hasattr(resultset, '_select'):
<noodles775> salgado: So, looking at r11428 (the unreviewed revision), it adds TestStagedBinaryUploadBase back in, but not TestStagedSecurityUploads?
<salgado> noodles775, right, I've only added it back in because test_buildduploads.py needs TestStagedBinaryUploadBase
<gmb> allenap, Can I get an r[s]= from you to merge that branch from yesterday into devel. I merged it into db-devel by mistake.
<salgado> noodles775, I've removed TestStagedSecurityUploads because that's a test for the 'security' upload policy, which the branch removed
<noodles775> OK.
 * noodles775 reads the MP
<gmb> allenap, Actually, I suspect I don't need to ask you for *another* r=, since the code's the same :)
<salgado> noodles775, and I've added a new unit test (in the other MP) to exercise the same code that was exercised by the doctest chunk I've removed
<noodles775> salgado: The doctest snippet that you removed in r11428 (reject instead of fail for non-existent pocket), you mention that you'll cover it with a unit test in the following branch...
<noodles775> lol
<noodles775> but
<salgado> :)
<salgado> http://bazaar.launchpad.net/~salgado/launchpad/vostok-upload-policy/revision/11432
<noodles775> as far as I can see, the following branch tests that an assertion is raised, but not that a rejection is sent
 * noodles775 double checks, guessing that he missed it.
<salgado> that's correct
<noodles775> Is there a test elsewhere that ensures when the assertion is raised an email is sent?
<salgado> I thought just checking for the exception was good enough, as we have plenty of tests showing a rejection is sent when there's an exception
<salgado> I could be wrong
<noodles775> OK.
<jml> noodles775, would you please review https://code.edge.launchpad.net/~jml/launchpad/one-email-on-crash/+merge/33768 ?
<jml> there appears to be some glitch on production stopping diffs from being generated.
<jml> I guess I should escalate to a losa
<noodles775> jml: sure.
<jml> noodles775, thanks.
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: leonardr || queue: [salgado, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> leonardr: I don't see a one-liner? It's r11442 that needs to be reviewed right? This: http://bazaar.launchpad.net/~leonardr/launchpad/optimized-length-2/revision/11442
<jml> yay, emails.
<leonardr> noodles775, 11442 corrects a typo made to the original one-liner. i was hoping to just give you the code so it wouldn't be so complicated
<leonardr> i'll just do another merge proposal and get in the queue
<noodles775> leonardr: I'm happy to review just the one-line that's changed, but currently I can only see it in 11442 along with a few other changes. If the rest is already reviewed, just paste a diff and I can review that, otherwise yes, a new MP is probably easier.
<leonardr> noodles775: i see the problem. i had to create a new branch with the old changes, so all the old changes show up as one revision
<leonardr> this code is the only code that hasn't been reviewed yet:
<leonardr> +        if hasattr(resultset, '_select'):
<leonardr> +            resultset._select.limit = Undef
<leonardr> +            resultset._select.offset = Undef
<noodles775> leonardr: r=me (I assume the second two lines were there previously, just not indented). Thanks!
<leonardr> noodles, yeah
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> jml: why do you set test_directory when it's passed as None?
<noodles775> (that is, why do you set it to a non-existent directory)
<jml> noodles775, umm, not sure. maybe because stuff explodes.
 * jml tries.
<noodles775> According to the docs, I'd assume it wouldn't explode when its None, but will in its current state?
<noodles775> But yes, science is the best answer :)
<salgado> thanks for the reviews, noodles775
<noodles775> jml: ah, you just moved that code... I'd not realised. But still worth checking.
<noodles775> salgado: np.
<jml> noodles775, yeah, deleting that bit works.
<noodles775> jml: OK, the other option would be to make it a required arg (which I guess was the original intent?)
<jml> noodles775, well, it's a required arg of the constructor, but most tests just don't care about it.
<noodles775> k
<jml> noodles775, so I'd rather leave it optional in the make_foo() method
<noodles775> Great.
* noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, i have a branch at https://code.launchpad.net/~leonardr/lazr.restful/its-really-read-only/+merge/33820 that you might enjoy reviewing
<leonardr> (or anyone, really)
<salgado> leonardr, sure, I'll take it
<leonardr> salgado, thanks
<deryck> sinzui, could I get your review (code/ui) of my CSS changes?
<sinzui> yep
<deryck> sinzui, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/good-god-comment-fonts-yall/+merge/33822
<sinzui> deryck, do you know what uses p.search-results?
<deryck> sinzui, looked to be things like suggestions of projects when registering a new project.  I saw no difference when testing locally, so saw no harm in making all these 116% changes together.
<sinzui> I'll look into that. that implies something else redefined the style. maybe we do not need it
<salgado> tagged_as_readonly = ('readonly' in tags and tags.get('readonly'))
<salgado> if (readonly and not tagged_as_readonly):
<salgado>     raise TypeError(...)
<salgado> if not readonly and tagged_as_readonly:
<salgado>     readonly = True
<salgado> leonardr, how about that?
<salgado> that won't work, I think
<salgado> yeah, it will raise TypeError if the field is readonly and there's no readonly tag
<sinzui> deryck, I think I see YUI font css on the body tag. I see our font rules are work, but YUI is also defining font-size and line-height
<sinzui> I hate YUI
<deryck> yeah, it made this work much harder than it should have been, IMHO
<deryck> and I generally love YUI
<sinzui> yep, the lines spacing on bug descriptions is easy to read when I disable the YUI rule
<deryck> I noticed in this work the great mis-match in descriptions and the rest of the page
<leonardr> salgado, fine
<sinzui> but why is it's rule *after* ours?
<salgado> leonardr, that won't work, but I have another idea I'm suggesting in my review
<leonardr> salgado: i can just change the names around so i don't redefine 'readonly'
<deryck> sinzui, you mean the description's rule?  I assumed it was set via JS on the fly.  But I didn't look into it.
<sinzui> no, webkit provides an inspector that shows the order of rules, and it lets you toggle them
<sinzui> I thought it was the widget too, but it only applies margin rules
<deryck> well, there are the rules in the stylesheets in lazr-js.  That's what I meant.
<deryck> well, part of what I meant/thought.  Not saying it correctly. :-)
<deryck> I'm hesitant to change the CSS on the description widget for this branch, just because it's quite fiddling to get all the spacing and alignment correct.
<jcsackett> EdwinGrubbs: did you/will you have a chance to look at the update to my MP?
<EdwinGrubbs> jcsackett: I actually just started to look at it.
<jcsackett> EdwinGrubbs: fantastic. :-)
<EdwinGrubbs> jcsackett: I commented on one more thing that I want changed. I marked it Approved, so you don't have to reply on the MP after you make the changes.
<jcsackett> EdwinGrubbs: sounds good. thanks, man.
#launchpad-reviews 2010-08-27
<gary_poster> lifeless: hey.  bad news: I have a 1336 line MP.  possibly good news: it is for ++profile++ .  https://code.edge.launchpad.net/~gary/launchpad/lsprof-on-demand/+merge/33849 if you want to look at it, or I'll try to get someone to look at it tomorrow.
<lifeless> gary_poster: thanks
<lifeless> gary_poster: it still drops the profile output in the previous place ?
<gary_poster> lifeless, yes.  configurable.
<lifeless> I have a suggestion
<gary_poster> cool;
<lifeless> if we used a feature flag
<lifeless> with a scope that matches users in canonical-launchpad
<lifeless> it would be less risky to have on on staging
<lifeless> and also permit turning it on and off without rebooting the appserver
<lifeless> you could do this without any schema changes
<lifeless> its just a thought
<gary_poster> lifeless, that sounds great.  want that in a separate branch?
<lifeless> something else that would be really lovely
<lifeless> is a kcachegrind file on disk (and oops on disk); and a way to download them (again, enabled for specific users/groups by flags)
<lifeless> gary_poster: in end_request, you've added a bunch of vertival white space
<lifeless> this is a matter of taste, but I find that very jarring when reading a function
<lifeless> I prefer either no VWS or separate functions.
<gary_poster> lifeless, download: I maybe could make ++profile++download download the kcachegrind.
<lifeless> (PEP8 says to use it for 'significant regiosn's (misquoting)
<gary_poster> VWS: OK.  I liked it here obviously, but it was very...organic.  Happy to remove it.
<lifeless> we don't need docstrings on tests btw
<lifeless> or if we do its nuts :)
<lifeless> as long as the intent is clear
<lifeless> if its vague, a docstring or comment to make it clear is good in its own right
<lifeless> 1334	+
<lifeless> 1335	+def test_suite():
<lifeless> 1336	+    return unittest.TestLoader().loadTestsFromName(__name__)
<lifeless> thats not needed now
<lifeless> gary_poster: approved
<gary_poster> lifeless: awesome thanks
<lifeless> gary_poster: there was one other thing
<lifeless> the 'tests are not setting stuff up so ignore it' squicks me; I realise why, and the pragmatic need, but I'd like to be able to put a ratchet on those tests and eliminate the badness.
<gary_poster> lifeless: not clear what you mean yet?
<gary_poster> oh!
<gary_poster> yeah that was annoying
<gary_poster> what do you have in mind for ratcheting?
<lifeless> well
<lifeless> depends on how widespread it is I guess
<lifeless> if its only a few root causes, like some test helpers, we could just do warning.warn() in a branch, and iterate on the branch till its done.
<lifeless> if its more substantial, perhaps starting the test suite could empty a file
<lifeless> and each occurence be logged (one line per)
<lifeless> and the total number of lines reported at the end
<gary_poster> between a few and several.  I have to run though.  Please copy that in to the MP and I'll act on it tomorrow.
<lifeless> lint could report if that file is longer than N
<mwhudson> thumper: care to look at https://code.edge.launchpad.net/~mwhudson/launchpad/cross-product-spec-links-bug-3552/+merge/33866 ?
<thumper> mwhudson: on a friday afternoon?
<thumper> mwhudson: just before beer?
<mwhudson> thumper: it'll make the beer taste better
<mwhudson> mm, proportionally fonted diffs
<thumper> mwhudson: line 23 of the diff
<mwhudson> thumper: lolz
<mwhudson> thumper: fix being pushed
<lifeless> hmm, exported_as doesn't work if there is an attribute with the same name that isn't exported.
<lifeless> echannel
<mwhudson> thumper: thanks
<henninge> jtv: fancy a quick review?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/recife-pofile-creation/+merge/33800
<jtv> henninge: coming
<henninge> jtv: thanks
<jtv> henninge: in this test you say "shared" POTemplates.  IIRC Danilo has been calling them "sharing" POTemplates.
<jtv> Also, "a <thing> has the same name" is a bit nonsensicalâsame name as what?
<henninge> jtv: right, I copied that comment without thinking.
<jtv> That explains the old-style setup as well.
<henninge> And I have always ued 'shared'.
<henninge> jtv: what's 'old-style' ?
<jtv> I think "shared" is misleading here, because the POTemplate isn't shared.  It shares things with other POTemplates.
<henninge> I see
<jtv> Old-style is relying on sample data (hoary and warty) instead of creating fresh data.
<jtv> (*) in this context.  :)
<jtv> After that it suddenly goes into the new style.
<henninge> jtv: Good point. I will change that.
<jtv> Also, you can shortcut some of the setup by letting the first self.factory.makeSourcePackage cal create your SourcePackageName for you.
<jtv> *call
<jtv> Probably your distroseries as well.
<jtv> Otherwise it's just too much setup to read through!
<jtv> henninge: in the comment for test_pofile_creation_shared_in_ubuntu, I assume "POTemplate of project" means "POTemplate of a project," not some other typo such as "POTemplate or project"?
<henninge> jtv: you are right
<henninge> jtv: Don't think I'll get around an explicit distroseries creation because it has to be for Ubuntu, does it not?
<jtv> henninge: ouchâI thought that's what we got.  The code you're testing here is probably one of the very few places that care about the difference.
<henninge> jtv: Also, in the second test I use sourcepackagename so often that I'd put it in a variable anyway and creating it explicitely keeps the source packages creation symetric.
<jtv> henninge: whatever works for youâjust try not to throw too much text at the reader!
<jtv> it's not all about the number of lines though.
<jtv> A short assignment line is generally easier to read than a very long one with a method call in it and lots of mixed-case letters.
<StevenK> adeuring: Hi! Got time to review a trivial MP for me?
<adeuring> StevenK: sure (except for a not yet sufficient caffeine level ;)
<StevenK> adeuring: https://code.edge.launchpad.net/~stevenk/launchpad/lucille-archivepermission/+merge/33873 when you're sufficently caffeinated
<adeuring> StevenK: where is it?
<adeuring> StevenK: so, the method newPackageUploader needs to be called by the db user 'lucille'. What does lucille do? And do we know if lucille indeed adds permissions only for people who should get them?
<StevenK> adeuring: No, that isn't it, that's just set up.
<StevenK> adeuring: The code that this actually affects is in part of the code that didn't change -- it adds new archivepermission rows directly into the database during initialise-from-parent
<adeuring> StevenK: so, some sort of copy operationÃ
<adeuring> sÃ/?/
<StevenK> adeuring: Yes, it is initialising a child distroseries from its parent
<adeuring> StevenK: ah, ok. r=me then
<StevenK> adeuring: Thank you!
<noodles775> adeuring: one-liner to fix the latest db-devel test-fix (when the MP diff updates): https://code.edge.launchpad.net/~michael.nelson/launchpad/db-devel-import-fix-20100827/+merge/33876
<StevenK> noodles775: Oh, if you're going to fix db-devel, I noticed a .THIS file crept in, can you bin it?
<noodles775> StevenK: sure
<adeuring> noodles775: approved
<noodles775> adeuring: thanks. I've just pushed 9708 which deletes the .THIS file StevenK mentioned.
<adeuring> noodles775: well, that's fine, I think ;)
<noodles775> StevenK: my landing will remove it from db-devel, but it's actually committed in devel.
<noodles775> Thanks adeuring
<StevenK> Rah
<StevenK> noodles775, adeuring: I have a branch ready for devel, shall I kill that .THIS file in it?
<noodles775> StevenK: Sounds good.
<adeuring> StevenK: makes sense
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring, Could you review https://code.edge.launchpad.net/~gmb/launchpad/lp-devs-can-reset-watches/+merge/33796 for me please?
<adeuring> gmb: sure
<gmb> Thanks
<noodles775> stub or lifeless: do either of you have time to review a 70line db patch? https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-schema/+merge/33515
* noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi again adeuring, when you've time, can you please take a look at https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-basic-model/+merge/33885
<adeuring> noodles775: sure
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: gmb || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gmb: r=me; 1 nitpick
<gmb> adeuring, Thank you.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring, foo*Condition is the usual naming idiom for methods used to decide whether or not we can display an action. Maybe showResetActionCondition() would be clearer.
<adeuring> gmb: right, that's better!
<gmb> Right, I'll do that, then.
<gmb> Thanks adeuring.
<stub> noodles775: Reviewed. The derived distro series calling its parent the derived distro series seems a problem.
<noodles775> stub: thanks... looking.
<stub> Or I think I'm just confused..
<stub> oic. This is the difference, it points to the derived distro series, there is some other link that points to the parent series.
<noodles775> Yes, there's a reference from IDistroSeries.parent_series... which is why we only need to reference the one (derived) series here.
<stub> DistroSeries.parent_series already exists.
<stub> Ok... rereviewing :)
<noodles775> Correct.
<noodles775> Thanks.... just reading your other comments :)
<noodles775> stub: Why would we want delete permissions? At the moment I'm planning that these differences will end in a resolved state, but we wouldn't want to delete them until the derived distroseries is itself deleted (if it ever is).
<stub> The derived distribution has a patched version of firefox. Tomorrow it decides that it should just use the parents version of firefox.
<noodles775> Great, so assuming the parents version has a higher revno, they upload it, this record is marked as resolved and the activity log is updated, but the record is not deleted, as tomorrow the parent might upload a new version, and the previous comments/activity is still useful.
<stub> ok
<stub> How big do you think the comments and activity will grow?
<stub> It will be a pain to split out into a structured format later.
<stub> If it is append only, storing it as a single text blob isn't ideal. If it is more a whiteboard or a copy of a text file stored in a branch, that would be fine I guess.
<noodles775> On average 4 or 5 lines. If a particular difference goes through resolved->needs attention-> resolved it would be a few more.
<stub> But over time
<noodles775> Yes, it is just like a whiteboard.
<stub> ok
<noodles775> Yep, so if overtime it went through that cycle (resolved->needs attention->resolved) *lots*, there would be 2 lines for each change.
<noodles775> Do you think its worth adding a separate comment model?
<noodles775> (from memory the code/bugs guys made the comment model re-usable... I'll check).
<noodles775> stub: oh, and I think I misunderstood your comment above. I am making the activity_log append only.
<stub> So we would probably be better off with a separate table containing the activity, one row per addition. Or even a link table between distroseriesdifference and message same as our other comment spools.
<noodles775> Yep, just looking at the message table... I'll do that then.
<noodles775> Thanks stub.
<stub> That gives you the infrastructure for attachments, email interfaces etc. if we want them in the future.
<noodles775> Yeah, given that the table already exists, it sounds crazy not to use it.
<jtv> noodles775: this definitely needs code review and it may also need UI review: https://code.edge.launchpad.net/~jtv/launchpad/bug-517700/+merge/33888
<jtv> (Sorry for the diff sizeâ¦ I didn't exactly aim to use up the entire budget)
<jtv> Drat.  Doesn't linkify the bugs in = Bugs 484375, 517700 =
<jtv> bugs 484375, 517700
<jtv>  That used to work.
<jtv> noodles775: oh, hang onâthere's conflict now (also neatly pulling it across the 800-line limit)
<noodles775> jtv: did you mean adeuring ?
<jtv> noodles775: arrrgh, yes, sorryâ¦ an experiment in reading the topic line from the tiny low-contrast fineprint my client shows at the top of the window
<adeuring> jtv: I'll look at it
<jtv> adeuring: hang on, I've got some conflicts to resolve first!
<adeuring> jtv: ok, ping me when the branch is ready
<jtv> adeuring: MP has updated â https://code.edge.launchpad.net/~jtv/launchpad/bug-517700/+merge/33888
<adeuring> jtv: OK, let me first finish the review for noodles775, then I'll need a lunch break, then i'll look at your branch
<jtv> adeuring: great, thanks.  I'll probably be out by then, unfortunately
<noodles775> stub: updated with an incremental, but feel free to leave it for Monday :D Thanks.
 * noodles775 lunches
<jtv> bigjools: https://code.edge.launchpad.net/~jtv/launchpad/fakelibrarian-fixture/+merge/33892
<jtv> bigjools: I thought you might like to review that one.  :)
<adeuring> noodles775: in test_new_non_derived_series(), you are using  the factory method in assertRaises, which is IMHO quite indirect. What about testing the new() method directly?
<jelmer> adeuring: Hi, can I add another mp to your queue?
<adeuring> jelmer: sure
<jelmer> adeuring: Thanks; the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa-qa/+merge/33804
* jelmer changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: Yep, I'll update.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jtv || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * jtv chears adeuring on
<jtv> *cheers
<bac> hi adeuring
<adeuring> hi bac1
<adeuring> bac!
<bac> hi adeuring, i'm ocr today but won't be able to help for a couple of hours.
<adeuring> bac: ok, no problem
<bac> thanks abel.
<adeuring> (except perhaps for people needing a review after 1730UTC ;) I have an appointment this evening...
<adeuring> erm, 1700UTC..
<adeuring> jtv: r=me
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> adeuring: splendid, thank you
<adeuring> welcome :)
* benji changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jelmer || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> my MP is at https://code.launchpad.net/~benji/launchpad/bug-580035/+merge/33911; it's for a bug in a bug fix (found while doing QA)
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jelmer: R=Me
<adeuring> (no longer r=me)
<adeuring> ...for this branch ;)
<jelmer> adeuring: :-)) and thanks
<adeuring> benji: GPGHanlder.getVerifiedSignature() can raise GPGVerificationError. I think this exception should be caught in extract_signature_timestamp()
<adeuring> erm, or in ensure:sane:signaturetimestamp()
<adeuring> ...or are we sure that we have a valid signature?
<benji> adeuring: at the point that I call GPGHanlder.getVerifiedSignature() it has already been called successfully
<benji> right
<adeuring> benji: ok, thanks!
<adeuring> benji: r=me
<benji> danke
<benji> adeuring: it says "Approve (code)" but at the top still says "Status:
<benji> Needs review"
<adeuring> benji: fixed
<benji> thanks
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: benji,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi jelmer
<sinzui> adeuring, bac: I have a branch for review
<bac> jelmer: i'm looking at https://code.edge.launchpad.net/~jelmer/launchpad/buildmaster-enums/+merge/33893
<jelmer> hi Brad
<adeuring> sinzui: I'll look
<bac> at line 379 is it really necessary to do the import locally?
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -,jelmer || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> adeuring, bac: I am going to start work on a branch to disable google maps. We are treating this as a critical issue.
<bac> sinzui: ok
<jelmer> bac: Thanks. 379 is a removed line here in the diff
<adeuring> sinzui: the mp i should at is this one: https://code.edge.launchpad.net/~sinzui/launchpad/licenses-0/+merge/33916 ? Or the google maps branch?
<jelmer> bac: 511 has a local import that I couldn't really work around.
<bac> jelmer: you pushed a new branch since i started looking at it, that's why the line numbers don't match
<jelmer> bac: Ah, my bad - sorry. I intended to push that to a separate branch.
<bac> but in that MP line 511 is the one i was asking about
<jelmer> bac: Would you like me to remove that revision or is the MP fine as is (it's a bit large)
<bac> jelmer: the branch is so mechanical it doesn't matter
<jelmer> bac: Ok
<jelmer> bac: I can probably work around the local import by putting BUILDD_MANAGER_LOG_NAME in a separate module. manager.py seems like the best place for it though.
<bac> jelmer: the local import isn't critical...just wanted to make sure there was a good reason for it.  if you can remove the need without a lot of hassle that would be great.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: sinzui,jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jelmer: r=bac, with some import sorting fixes.  perhaps you can find henning's tool and run it? (though that would take longer than doing it by hand i suspect.)
<jelmer> bac: I should get used to doing it right in the first place so I'll do it by hand. :-) Thanks for the review
<bac> np jelmer.   thanks for the branch.
<henninge> bac: I am working on putting it into the tree ATVM. ;-)
<bac> henninge: i'll happily review it.
<henninge> bac: should I add tests?
<henninge> ;)
<bac> henninge: that would be nice, of course.  but we do have lots of thing in utilities that are stand-alone tools that don't have tests.
<henninge> bac: since the tool changes other files in the tree, the real test is if those changes break the test suite.
<henninge> or not
<bac> henninge: you've sold me
 * henninge prepares MP ;)
<henninge> Actually, let me add something else first.
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -,jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bac: Here it is:
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/format-imports/+merge/33926
<bac> thanks henninge
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -,henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: -, bac || reviewing: -,henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> henninge:  i don't understand the comment describing FIRST
<bac> do we actually use that anywhere?
<henninge> once
<henninge> bac: It's about imports that need other imports.
<henninge> I feared we'd have more problems like that but luckily only two.
<henninge> one of which is lib/canonical/launchpad/interfaces/__init__.py which is the only use of the "SKIP" comment.
<henninge> the other one is in lp.code
<henninge> bac: sorry, "imports that need other imports" is badly put. It's about import ordering.
<bac> henninge: so there are imports that must be done in a specific order or they fail?
<henninge> bac: yes, or so says the comment.
<henninge> I don't like that very much, either.
<henninge> let me find it
<bac> cool
<henninge> lib/lp/code/bzr.py
<henninge> bac: I will end my week now. Do you have any more questions?
<bac> henninge: no.
<henninge> cool
<bac> henninge: sadly, i just ran your tool against lp/registry and it created a 231 line diff
<henninge> oops
<bac> so in just a week we've diverged that much
<bac> henninge: one more question
<bac> henninge: you say embedded comments will break the formatter.  is that reported at all or is it so broken you don't get a chance?
<henninge> bac: it's not reported at all
<bac> henninge: ok.  i guess it'll create such a mess that it'll be easily discovered
<henninge> bac: we only had one case like that in the tree that I simply fixed before running the tool
<bac> sorry to keep you henninge.  i'll finish the review after lunch.  have a good weekend.
<henninge> bac: thank you
<henninge> bac: enjoy your lunch ;-)
<bac> henninge: i shall...but it's getting cold.
<henninge> sorry to keep you ... ;)
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> indeed; I had almost exactly the same reaction
<benji> the thing is, the few things I'd like (probably the same ones you're referring to) would be pretty easy to implement in Python 2
<benji> I really wish I had the desire to be the new BDFL.  Now's the time for a Coup d'Ã©tat.
<benji> yeah, I expect most of those won't, but I don't see why most of those couldn't be backported
<benji> oops, I'm in the wrong chan
<benji> bac: watch out!  https://code.launchpad.net/~benji/launchpad/bug-622765/+merge/33951
 * bac ducks
<benji> heh
* benji changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> bac: for some reason my MP just shows "Approve (code)" and still shows "Needs review" at the top.  That's the third time that's happened to me.  Has the review UI changed for reviewers?
 * benji really needs to start on getting his reviewer merit badge.
<bac> benji: that reason would be it is a two step process and i forgot to do the second step
<benji> it's the processes fault ;)
<bac> benji: reviewers should set it.  when they forget, you can still proceed using 'ec2 land --force'  ... but it is better to just bug them.
<bac> benji: done
<benji> I like to bug people... everyone wins!
* benji changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> hello, bac.
<jcsackett> https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-official_codehosting/+merge/33953
<jcsackett> have time for another review today?
<bac> jcsackett: sure
<jcsackett> awesome.
<jcsackett> bac, this one shouldn't take too long.
<jcsackett> apologies if it does.
* bac changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: re your comments, i actually thought it was clearer with the full if structure than "var = (usage_enum == ServiceUsage.something). you think the compressed version would be better?
#launchpad-reviews 2010-08-28
<sinzui> Sinzui want https://code.edge.launchpad.net/~sinzui/launchpad/disable-gmaps-0/+merge/33971 reviewed so that it can be CPed
#launchpad-reviews 2010-08-29
<lifeless> anyone around ?
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011
<StevenK> lifeless: Are you that busy that you need to work on a Sunday?
<lifeless> StevenK: I had a very fragmented week last week
<lifeless> the first few days of caffeine detox were a bitch
<lifeless> + moved house
<lifeless> + this has been annoying me since the epic
<lifeless> StevenK: are you a reviewer yet ?
<lifeless> jelmer: hai
<StevenK> lifeless: Mentored by thumper, and I'm about to go out
<lifeless> have fun
* lifeless changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> lifeless, hi
<lifeless> jelmer: hi
<jelmer> lifeless, 'morning
<lifeless> jelmer: are you a full reviewer now ?
<jelmer> lifeless: yep, have been for some time
<lifeless> care to do a small review ? https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011
<jelmer> lifeless: Sure
<jelmer> I need to get some groceries before the shops close, but I'll have a look when I get back.
<lifeless> thanks!
<lifeless> thumper: can I get a review ?
<thumper> lifeless: I'm sprinting with wallyworld_ this week, so my time to response may be a little lower than normal
<thumper> so almost glacial
<lifeless> ah
<lifeless> see, I have this patch
<lifeless> that removes an O(N^2) behaviour in the bug attachments API
<lifeless> its been our biggest oopser since the epic
<lifeless> I'm excited.
<lifeless> I'll see if mwhudson has a cycle or three
<lifeless> mwhudson: ^
<mwhudson> well
<mwhudson> noone is around from linaro on mondays to see me slacking on that front...
<lifeless> its small
<mwhudson> link me up
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011
<lifeless> the typo on line 173 i'm fixing now
<mwhudson> hmm
<mwhudson> the bug xyz text on the merge proposal links to https://code.edge.launchpad.net/bugs/625102 :(
<_mup_> Bug #625102: exported_as does not handle overriding an unexported attribute <lazr.restful:New> <https://launchpad.net/bugs/625102>
<lifeless> yes
<lifeless> it made this more awkward than I would have liked.
<lifeless> If there is another alternative workaround I'm happy to do-over - ec2 test told me some stuff failed, and I've fixed and checked all that locally, so I'm pretty sure there are no dragons left
<mwhudson> no, the rootsite of the url is wrong
<mwhudson> that's what i was complaining about
<lifeless> oh, ah!
<lifeless> it works :P
<mwhudson> yeah
<mwhudson> i don't know what i think about that aspect :)
<lifeless> what do you think about the other aspects
<mwhudson> ah sorry got distracted by email
 * lifeless undistracts mwhudson
<mwhudson> lifeless: i think you need to have a big XXX block explaining what attachments_unpopulated is and how it differs from plain attachements
<lifeless> mwhudson: like the one in the docstrings on the two properties?
<mwhudson> yeah, that'd be good i think
<mwhudson> er
<mwhudson> no
<mwhudson> in the interface
<lifeless> this poses a quandry
<lifeless> I don't want to repeat myself.
<lifeless> The implementation is the right place to write this up - which I've done.
<lifeless> the shenanigans involved are certainly totally irrelevant to the API
<mwhudson> sure
<lifeless> and at the interface level these two things are identical - to the extent that the interface models behaviour
<mwhudson> so maybe i should name the problem rather than a solution:
<mwhudson> if someone looks at the interface as it is in your branch, they are going to say "wtf"
<lifeless> I'm happy to add a 'see lp.bugs.model.bug.Bug.attachments'
<lifeless> but having the implementation and explanation separated pretty much guarantees skew in the future.
<lifeless> +    # See lp.bugs.model.bug.Bug.attachments for why there are two similar
<lifeless> +    # properties here.
<mwhudson> you should also link to the lazr.restful bug
<lifeless>      attachments_unpopulated = CollectionField(
<mwhudson> lifeless: that'll do
<lifeless> well
<lifeless> the lazr.restful bug wouldn't eliminate having two properties
<lifeless> it would however let the obvious one be the less-eager-loading one as we did in Person.allmembers and Person.all_members_prepopulated
<mwhudson> .sure
<mwhudson> it should still be mentioned somewhere
<lifeless> Unless we're going to actually come back and change this when the bug is fixed in lazr.restful, I don't see much point.
<mwhudson> i assumed we were
<lifeless> its cosmetic
<mwhudson> having most of the code in launchpad reference this peculiarly named property is tech debt, surely?
<lifeless> it would be a little nicer, but not faster or more correct, if lazr.restful had allowed exporting it
<mwhudson> well sure it's cosmetic
<mwhudson> cosmetics are important
<lifeless> mwhudson: it being a property is tech debt
<lifeless> uhm
<lifeless> I can mention it, I just don't see the value in doing so.
<mwhudson> lifeless: the way things are, i think it's very likely that the next time someone writes code that iterates over attachments, they will say
<mwhudson> for attachment in self.bug.attachements: ...
<mwhudson> because that's the natural thing to write
<mwhudson> wouldn't it be better if that was actually the correct thing to say?
<lifeless> it would be, I agree. there are - maybe - 5 bugs needed to fix to make that correct.
<lifeless> we need eager loading
<lifeless> we need a QL of some sort to control it
<lifeless> we need separated domain and storage code
<lifeless> we need the API to use object introspection on interfaces to specify a QL that will eager load appropriately
<lifeless> look, I'll reference the bug, I just think its minutiae; we could fix all the XXX linked bugs and not be hugely better off.
<lifeless> or we can fix the structural things, and most of the XXX comments will just get deleted as we swing past the code and overhaul it.
<mwhudson> maybe i'm too pessimistic, but launchpad is littered with 'temporary fixes' from 2005
<mwhudson> i know you're good at fixing that sort of problem
<lifeless> absolutely.
<lifeless> (to both lines :P)
<lifeless> +    # attachments_unpopulated would more naturally be attachments, and
<lifeless> +    # attachments be attachments_prepopulated, but lazr.resful cannot
<lifeless> +    # export over a non-exported attribute in an interface.
<lifeless> +    # https://bugs.launchpad.net/lazr.restful/+bug/625102
<_mup_> Bug #625102: exported_as does not handle overriding an unexported attribute <lazr.restful:New> <https://launchpad.net/bugs/625102>
<lifeless>      attachments_unpopulated = CollectionField(
<mwhudson> lifeless: thank you
<lifeless> All I'm saying is that the tech debt lets-use XXX comments approach seems to be not working.
<mwhudson> lifeless: it has worked out ok a few times for me
<lifeless> because, the deep problems aren't actually being tackled, or something.
<lifeless> or aren't getting enough official airtime. I don't have enough data to isolate causes yet.
<mwhudson> when i've worked around a bug in bzr, i've been able to grep for the bug number when the fix in bzr has been released
<mwhudson> (and twisted)
<lifeless> generally though thats not something thats really a workaround to a structural issue in LP is it ?
<lifeless> like, having to do X to bzr, which it doesn't support, because Y is brain-damaged in LP itself
<mwhudson> i guess not
<lifeless> I think things that can't be done the right way are worth noting with bugs; but the right bug number should be the root cause.
<lifeless> anyhow, thats committed and pushed.
<mwhudson> lifeless: how did you find the places to s/attachments/attachments_unpopulated/, just grep?
<lifeless> mwhudson: yes
<mwhudson> must have been fun :-)
<lifeless> not at all
<lifeless> it will work if the wrong one is used, just be more expensive.
<lifeless> I don't *think* I missed any
<mwhudson> lifeless: i don't find the docstring on BugAttachment.message very enlightening
<lifeless> so, you understand what its doing ?
<mwhudson> i do now
<lifeless> any suggestions for what to add into it ?
<mwhudson> trying to come up with some :-)
<mwhudson> maybe something like "see lp.bugs.model.bug.Bug.attachments for where the IIndexedMessage is injected"
<mwhudson> and maybe change the first line to "this is a cached property to allow message to be prepopulated"
<lifeless> -        """This is a property to allow message to be an IIndexedMessage.
<lifeless> +        """This is a cachedproperty to allow message to be an IIndexedMessage.
<lifeless>          
<lifeless> -        This is needed for the bug/attachments API calls, which seem to be the
<lifeless> -        only users of .message.
<lifeless> +        This is needed for the bug/attachments API call which needs to index
<lifeless> +        an IIndexedMessage rather than a simple DB model IMessage. See
<lifeless> +        Bug.attachments where the injection occurs.
<mwhudson> lifeless: +1, thanks
<mwhudson> lifeless: no other comments, apart from "why is there SAMPLE_PERSON_EMAIL and USER_EMAIL??"
<mwhudson> but that can rest for another day
<lifeless> if I knew
<lifeless> having found they were the same, I thought I'd make it clear they were the same :)
<mwhudson> yeah fair enough
 * mwhudson goes to do things on the website
<lifeless> thanks
#launchpad-reviews 2011-08-24
<Ursinha> hello launchpadders :) quick python line wrapping question
<Ursinha> in an if statement with an and that is more than 78 characters what is the right way to wrap it?
<jelmer> Ursinha: usually to use parentheses around the expression
<jelmer> Ursinha: most people are in #launchpad-dev these days, not sure why we're still here :)
<Ursinha> jelmer: and break wherever it's possible or after the and or something?
<Ursinha> haha, indeed :)
<jelmer> Ursinha: yeah, whereever possible but preferably after the end
<Ursinha> cool
<Ursinha> thanks jelmer :)
<jelmer> np
