[04:02] Want codebrowse to generate OOPS reports? Then please review https://code.edge.launchpad.net/~spiv/launchpad/codebrowse-oops-84838/+merge/31007 :) [04:17] * mwhudson looks [04:19] spiv: er [04:19] + Strictly speaking this isn't 100% correct WSGI middleware, because it [04:19] 99 + doesn't respect the return value from start_response (the 'write' [04:19] 100 + callable), but using that return value is deprecated and nothing in our [04:19] 101 + codebrowse stack uses that. [04:19] loggerhead in fact uses that callable [04:19] mwhudson: heh [04:19] mwhudson: so I just noticed! [04:19] mwhudson: see latest push [04:20] mwhudson: for some reason that didn't show up until my last-minute-before-I-push cleanups :( [04:20] Sorry for the noise. [04:20] heh [04:21] There's also something funny I'm seeing with the very first request I make to the server after starting it giving a weird HTTP error, but I'm not sure that's related to this branch. [04:25] I'll confess to thinking this isn't the most beautiful code I've ever written :) [04:27] mwhudson: I have a couple of small branches if you have a few minutes [04:27] https://code.edge.launchpad.net/~thumper/launchpad/fix-mirror-failure-story/+merge/31010 [04:27] spiv: heh heh [04:27] https://code.edge.launchpad.net/~thumper/launchpad/stop-factory-mirroring/+merge/31011 [04:28] I feel so wonderfully old-school with all these mutable variables destined to be mutated by nested functions, though! [04:29] you've been wsgi-ed! [04:29] Possibly there's enough going on there that I can profitably refactor out an actual object. Initially it was just the one little variable, but well, wsgi... [04:30] I plan one day soon to take more of a look at loggerhead [04:30] spiv: blurgh [04:31] spiv: we shouldn't return a 200 when we're reporting an error [04:34] mwhudson: ah ok, I wondered about that, I meant to ask [04:34] I'm happy to ape whatever lpnet does here, if you happen to know :) [04:35] I was hoping to avoid spelunking through the layers of zope publication internals more than necessary [04:36] * mwhudson attempts to trigger a timeout [04:37] spiv: this timeout has a http 503 [04:38] I'm happy to change it to 500 Internal Server Error. [04:39] spiv: please [04:39] I guess if some IE users get a boilerplate "IE noticed that the server broke, but I won't show you any helpful details the server may have sent about that" page then that's their problem ;) [04:40] yeah, i think that's a potential issue [04:40] but i agree with your conclusion [04:40] spiv: i reviewed the branch btw [04:42] Thanks! [05:17] mwhudson: another relatively brainless review https://code.edge.launchpad.net/~thumper/launchpad/bzr-format-matching-refactoring/+merge/31014 [05:17] mwhudson: although more thought needed than the other two :) [05:18] thumper: looking [05:19] thumper: "# The default branch format is unrecognized." [05:19] thumper: this caused mental jarring; the default branch format is 2a, surely? [05:19] mwhudson: ah [05:19] mwhudson: hmm... [05:20] the idea is that if we don't recognise it, it is unrecognized [05:20] a rename perhaps [05:20] so for the test methods s/default/unrecognized/ [05:21] thumper: +1 [06:19] mwhudson: last for the day... https://code.edge.launchpad.net/~thumper/launchpad/merge-directive-handling-no-request-mirror/+merge/31019 [06:21] * mwhudson waits for the diff [06:23] thumper: done [06:23] mwhudson: thanks [06:27] * thumper kicks off 4th ec2 land run === bigjools is now known as bigjools-afk === bigjools-afk is now known as bigjools [12:20] wgrant, hi [12:20] jelmer: Evening. [12:22] wgrant: Any particular reason for returning None rather than raising an exception in your refactor-nuf-creation branch? [12:23] jelmer: Hm, not really. [12:24] Not sure which exception is appropriate, though. [12:27] Probably a new one. ValueError would work too, but is perhaps a bit too generic. [12:27] Right. Is it worth creating a new exception for that? [12:28] Personally I think so, it'd be clearer if people come across that error. I can see the argument either way though. [12:28] s/it'd/it'll/ [12:42] bac`: hi === jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [jtv²] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [jtv²] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:41] mars: I just send an MP mail, can I add myself to your queue please === bigjools changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [jtv², bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === bac` is now known as bac [13:45] hi jelmer [13:46] bac: 'morning! [13:46] bac: I was wondering if you'd seen my follow-up to https://code.edge.launchpad.net/~jelmer/launchpad/600153-qafix/+merge/30790 [13:47] jelmer: done [13:47] thanks for the reminder [13:48] bac: thanks for the review :-) [14:17] * flacoste is back. === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: jtv || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:31] hi mars, thanks for reviewing me [15:31] np [15:52] benji: I'll see how far I get. To start: [15:53] - I'd suggest making a local release of this release of batchnavigator and integrate it with Launchpad. Then you can see if the change to the endNumber behavior is problematic [15:54] - in lines 55 and 59 of the diff, why do you call len(batch) outside of the assertions? [15:55] re. local release: good idea [15:56] - I don't see the value of the behavior described in lines 68-74 of the diff [15:56] shouldn't prevBatch try again, or at least puke? [15:56] as it is, it seems like an odd edge case designed to bite people unexpectedly :-) [15:57] - I like the new endNumber behavior. I also like the new comments. [15:57] re. len: that's cruft that shouldn't be there; removing [15:58] cool [15:58] also cruft: line 115 (#print "_get_length called (EXPENSIVE)") [15:59] re. 68-74: me neither, I just added comments that describe what is being testsed; I don't think the behavior is useful, but I don't believe that strongly enough to remove the tests [15:59] - line 120: the Lazy decorator is supposed to mean that you just "return self._get_length(self.list)" [15:59] you shouldn't have to also set the _listlength value [16:00] 68-74: but...that's new test for this branch [16:00] maybe this is something that leonard added for this branch? [16:00] if so, my question goes to him [16:00] the reason we set _listlength is so we can use it if available; avoiding calling listlength if not [16:01] let me look at 68-74 again [16:02] _listlength: ah ok. Maybe remove the @Lazy then, since I suspect it adds processing cost for the reader for little to no advantage for speed? [16:02] gary_poster: right, Leonard added the test, I added the comment [16:03] ok. do you want me to ping him, or do you want to pursue on your own so you feel you understand it first? [16:03] (where "pursue on your own" == "you ping Leonard" in my book, but as you wish :-) ) [16:03] re. _listlength: +1 [16:03] cool [16:03] I'll pursue [16:04] cool thanks [16:05] I don't understand "return len(self._sliced_list) - 1" yet: why -1? Maybe I'll see soon... [16:05] (line 139) [16:08] - another cruft but: #print "sliced_list called (EXPENSIVE)" (line 206) [16:08] s/but/bit/ === henninge changed the topic of #launchpad-reviews to: On call: mars || reviewing: jtv || queue: [bigjools, henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:42] mars: Hi, I added myself to the queue and it would be great if you could get to my branch but I will also have to leave in 30 min. So I don't mind it waiting around till tomorrow, either. [16:43] ;-) [16:44] oops, confilcts ... === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: bigjools || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: henninge || queue: [lunch] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: henninge || queue: [lunch] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jml` is now known as jml === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars 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