[10:48] <allenap> henninge: I have a couple of branches to review. Do you fancy taking either of them on?
[10:48] <henninge> allenap: sure, which one do you want done first?
[10:51] <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.
[10:51] <henninge> allenap: ok, I'll have a look and take my pick ;-)
[11:10] <allenap> henninge: Thanks :)
[11:11] <henninge> allenap: lifeless was working on cachedproperty, too. Is this related somehow?
[11:12] <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 :)
[11:15] <henninge> allenap: yes, I understand what you mean, I felt similar about 'ugly'. ;-)  Did you have a pre-imp discussion with somebody, though?
[11:15] <henninge> and I notice that the branch is called an "experiement" ...
[11:15] <allenap> henninge: No. It was just an experiment at first, but it went better than I expected so I cleaned it up for review.
[11:17] <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.
[11:18] <allenap> henninge: I'm happy to move them.
[11:18] <allenap> henninge: Would you like me to move them before you continue?
[11:19] <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).
[11:19] <henninge> allenap: so, yes, I'd prefer them being moved.
[11:19] <allenap> henninge: Okay, I'll do that now.
[11:30] <henninge> allenap: I like the new interface, btw... ;-)
[11:30] <allenap> henninge: Brilliant :)
[11:34] <allenap> henninge: Done; the documentation is now in doc/propertycache.txt.
[11:34] <henninge> cool ;)
[11:39] <henninge> allenap: aren't these two calls identical? Do you really still need both?
[11:39] <henninge> 120	         clear_cachedproperties(self)
[11:39] <henninge> 121	+        IPropertyCacheManager(self).clear()
[11:39] <allenap> henninge: They're not identical yet; they clear different caches. I will remove clear_cachedproperties in a subsequent branch.
[11:41] <henninge> allenap: so, both implementation of cachedproperty exist in parallel atm and it depends on which one you import?
[11:42] <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.
[11:43] <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.
[11:43] <allenap> henninge: Okay.
[11:45] <henninge> allenap: the change in lib/canonical/configure.zcml is an unrelated drive-by fix, right?
[11:46] <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.
[11:47] <henninge> allenap: cool, I see other clean-ups, too. Thanks! ;-)
[11:48] <henninge> allenap: btw, are you sure this is supposed to go into lp.services? It is not really LP specific, is it?
[11:49] <henninge> I mean, is the whole "canonical" tree expected to go away eventually, or just "canoincal.launchpad"?
[11:50] <allenap> henninge: I don't know actually.
[11:52] <wgrant> I think canonical.* is meant to go into lp.* and lazr.*
[11:54] <allenap> henninge: Can we assume this is okay for now?
[11:56] <allenap> henninge: XXX and comment added.
[11:58] <henninge> allenap: oh yeah, I would not have asked you to move it, I was just wondering ... ;-)
[11:58] <allenap> henninge: Cool :)
[11:58] <allenap> henninge: I have to go and help with the kid's lunch now. Do you mind if I step out for ~1h?
[11:59] <henninge> allenap: Fits perfectly, I'll be out to lunch soon, too. ;-)
[11:59] <allenap> henninge: Cool, thanks.
[12:07] <bigjools> henninge: hi, I've got a small branch just pushed up, can I add it to your queue please?
[12:07] <StevenK> bigjools: Get in line!
[12:07] <StevenK> :-P
[12:07] <bigjools> StevenK: yes that's what adding to the queue generally means ...
[12:10] <henninge> bigjools: go ahead ;-)
[12:11] <bigjools> thanks henninge
[12:11] <bigjools> it was quite tempting to jump StevenK for a laugh then :)
[12:12] <StevenK> Hmph :-P
[12:15] <henninge> StevenK: I assume that there is no obvious way to stormify the INSERT query in your branch?
[12:15]  * henninge has no idea, just checking ;)
[12:18] <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
[12:20] <henninge> StevenK: I know it's being copied. ;-) I can imagine that it is not possible to Storm-ify that ...
[12:20] <henninge> so nm.
[12:40] <henninge> StevenK: review sent. r=me after some fixes ;)
[12:40] <StevenK> henninge: Thanks!
[12:40]  * henninge goes to lunch
[12:41] <StevenK> henninge: topic =~ s/allenap/lunch/ ?
[13:51] <henninge> StevenK: no, I am not done with allenap's yet
[13:56] <StevenK> henninge: And no, it isn't the same call -- one is *sets*Included, the other is *sources*Included
[13:57] <henninge> StevenK: ah, now that you mention it ... ;-)
[13:59] <StevenK> henninge: And yes, relatedSets returns an ordered list
[14:00] <henninge> cool
[14:06] <StevenK> Doh! Alphabetical fail
[14:31] <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 ...
[14:48] <StevenK> henninge: Would you like to mark the Status of my MP as Approved, or shall I do it?
[14:48] <henninge> allenap: Can you please extend DefaultPropertyCache's docstring to point out that getattr and setattr are already implemented by object?
[14:48] <allenap> henninge: Sure.
[14:49] <henninge> StevenK: You can do that yourself if you feel that you considered my comments enough ... ;-P
[14:49] <henninge> StevenK: I never set that when I review. It's for you to decide when your branch has all the reviews it needs.
[14:50] <henninge> allenap: or a comment
[14:50] <allenap> henninge: Oh yeah, a comment would make more sense actually.
[14:55] <henninge> allenap: I am done reviewing and I see what I like.
[14:55] <henninge> ;-)
[14:55] <allenap> henninge: \o/ Thanks
[14:56] <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.
[14:57] <allenap> henninge: Yeah, good idea.
[14:57] <henninge> Kind of a after-imp discussion...
[14:57] <henninge> Cool, r=me ;-)
[15:04] <henninge> bigjools: your turn ;)
[15:04] <bigjools> cool
[15:15] <henninge> bigjools: Why did you create Test_rescueBuilderIfLost? Could this not have been part of TestBuilder?
[15:15] <bigjools> henninge: arse - it's because I forgot to remove the layer - it doesn't need any layers
[15:16] <bigjools> and it's also testing a standalone function
[15:21] <henninge> bigjools: builder.rescueIfLost? That is a method, not a function. Am I missing something?
[15:21] <bigjools> henninge: there's builder.rescueIfLost and rescueBuilderIfLost
[15:21] <bigjools> the former calls the latter
[15:22] <bigjools> it's a separate function for ease of testing, since it needs mock builders in some tests
[15:23] <henninge> bigjools: ok, I see.
[15:24] <henninge> bigjools: r=me ;-)
[15:24] <bigjools> henninge: actually I think I lied about the layer
[15:24] <bigjools> but
[15:24] <bigjools> I remember now I created the extra Test case so that we could add more tests for that function
[15:24] <bigjools> it's woefully under-tested
[15:24] <bigjools> henninge: and thanks :)
[15:25] <henninge> bigjools: welcome ;)
[15:55] <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
[15:55] <noodles775> Thanks!
[15:57] <henninge> noodles775: I am sorry, I will stop reviewing now because I have to take care of something else.
[15:57] <henninge> I hope abentley will become available soon ...
[15:58] <noodles775> henninge: no probs.
[15:58] <abentley> henninge, I am just back from vacation and catching up on emails, but I can sign on, I guess.
[15:59] <abentley> henninge, what's the status of allenap?
[16:00] <henninge> abentley: that is a second branch that I have not looked at yet.
[16:00] <abentley> henninge, okay.
[16:01] <henninge> abentley: I meant this one: https://code.edge.launchpad.net/~allenap/launchpad/refactor-mailnotification/+merge/32923
[16:01] <henninge> aka "the long one"
[16:01] <henninge> abentley: the cache-experiment is done.
[16:01] <abentley> henninge, cool.
[16:02] <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.
[16:02] <henninge> allenap: ;-) It's not much longer than the cache-experiment branch is.
[16:03] <allenap> henninge: Mmm, yes, that did get a bit big :-/
[16:08] <abentley> allenap, it looks okay, except that the copyright on date on newbug.py is wrong.
[16:08] <allenap> abentley: Oops! I'll fix that. Thanks.
[16:09] <allenap> How did I manage to get 2012 in there?
[16:11] <abentley> topic On call:  abentley || reviewing: noodles775  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:17] <abentley> noodles775, r=me
[16:17] <noodles775> Thanks Aaron.
[16:52] <leonardr> EdwinGrubbs: a while back you reviewed https://code.launchpad.net/~leonardr/lazr.restful/integrate-optimized-len/+merge/33023 for me
[16:52] <leonardr> you were about to approve it when i asked you to hold off because i'd identified a problem
[16:52] <leonardr> it turned out the problem had nothing to do with my branch (it was a bug in storm)
[16:52] <EdwinGrubbs> leonardr: I remember
[16:52] <leonardr> can you take another look at that branch and approve it?
[16:52] <EdwinGrubbs> leonardr: sure
[16:53] <EdwinGrubbs> leonardr: r=me
[16:54] <leonardr> thanks
[17:13] <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
[17:13] <abentley> gary_poster, sure.
[17:13] <gary_poster> Thank you
[17:15] <abentley> gary_poster, r=me
[17:15] <gary_poster> thanks again
[17:25] <jcsackett> abentley, ping.
[17:25] <jcsackett> just letting you know, mp is at https://code.edge.launchpad.net/~jcsackett/launchpad/620494-downloads-sort/+merge/33413.
[17:44] <leonardr> abentley, i have a very small mp at https://code.edge.launchpad.net/~leonardr/launchpad/optimized-length-2/+merge/33417
[18:35] <sinzui> abentley, I added to the queue. If you do not have time to review my branch, i'll hunt for someone.
[18:47] <abentley> jcsackett, when you say "Implementation details: As above", it sounds like "Implementation details: talked with Curtis".  Is that what you mean?
[18:47] <jcsackett> abentley: no, sorry, as above references Proposed fix.
[18:48] <jcsackett> bad write up on my part there, sorry.
[18:48] <jcsackett> i've updated the Description.
[18:48] <abentley> jcsackett, Cool.
[18:49] <abentley> jcsackett, ProductReleased.datereleased is a date, not a datetime?
[18:50] <jcsackett> abentley: the form that is presented when you create one expects datetime, but it appeared only date is used.
[18:50] <jcsackett> so time always == 0:00:00.
[18:51] <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.
[18:51] <jcsackett> er, EdwinGrubbs, rather.
[18:51] <abentley> jcsackett, why not change it use the actual time?
[18:51] <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.
[18:52] <jcsackett> I *believe* that EdwinGrubbs has something dealing with the db issues.
[18:52] <jcsackett> but it won't be available for a bit.
[18:53] <abentley> jcsackett, r=me.
[18:53] <jcsackett> thanks, abentley. :-)
[18:56] <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
[18:57] <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
[18:57] <jcsackett> abentley: that's an excellent call. i will do that.
[18:58] <abentley> leonardr, r=me.
[19:02] <abentley> EdwinGrubbs, in what context is it illegal for an email to have non-ascii characters?
[20:33] <EdwinGrubbs> abentley: whenever a different encoding is used, it should be escaped as quoted-printable hence the "=3E" characters seen often, or as binhex.
[20:34] <abentley> EdwinGrubbs, "should", perhaps, but definitely not "must".  Content-transfer-encoding may be "8-bit" or "binary" for MIME messages in 8-bit encodings.
[20:38] <EdwinGrubbs> abentley: ok, then I think it is just the headers that are expected to be ascii, which is where the problem is occuring.
[20:38] <abentley> EdwinGrubbs, yes, those are illegal.
[20:40] <EdwinGrubbs> abentley: I can change it to just escape the headers then.
[20:40] <abentley> EdwinGrubbs,  but it appears you're applying the escaping to the entire message, which would actually break valid messages.
[20:41] <EdwinGrubbs> abentley: yes, I'll have to split the headers and the body.
[20:47] <abentley> EdwinGrubbs, I recommend tweaking message_from_string if possible.
[20:48] <EdwinGrubbs> ok
[21:00] <leonardr> abentley, i need a pretty simple review
[21:00] <leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/find-length/+merge/33440
[21:00] <abentley> leonardr, I am good at giving simple reviews :-)
[21:01] <abentley> leonardr, is this based on your previous branch?
[21:03] <leonardr> abentley: well, it's related work, but it doesn't depend on the branch you reviewed earlier
[21:04] <abentley> leonardr, r=me
[22:18] <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?
[22:18] <abentley> james_w, sorry, off for the day.
[22:19] <james_w> someone else then?
[22:23] <jelmer> james_w: Yes
[22:23] <james_w> thanks
[22:23] <james_w> I think it's a merge of two approved branches, but I need a review in order to ec2 land it
[22:23] <james_w> and it's backwards anyway, as the code is already in lp-buildd 70, so it's hard to disagree with it :-)
[22:24] <jelmer> hehe
[22:29] <jelmer> james_w: r68 mentions reverting all of r68, but I don't see those changes in the diff
[22:29] <jelmer> (and I don't understand why)
[22:30] <james_w> jelmer: yeah, that is odd
[22:34] <jelmer> james_w: see #soyuz
[22:48] <jelmer> james_w: as lamont has just updated the branch, r=me
[22:48] <james_w> thanks
[23:18] <EdwinGrubbs> abentley: you're probably already gone for the day, but I made the changes you suggested if you're still around.