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