[02:50] thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-code-windmill-tests/+merge/17268 === stub1 is now known as stub [07:00] https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails. [09:16] henninge: I assume the topic is out of date...? [09:16] allenap: well, bac should be in bed by now ... ;-) === allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [09:17] allenap: My branch is not reviewed yet and bac has not claimed it. [09:17] henninge: And he's a very naughty boy if he's not. [09:17] henninge: I'll go and have a look. [09:17] ;-) definitely [09:17] allenap: thanks [09:17] allenap: it's over-sized. [09:22] henninge: I've just read my email; my duty first and foremost today is to get checkwatches working again, so thank you for flying AllenapAir, but I need to jump out of the emergency exit now. === allenap changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [09:23] allenap: right, I read that, too. Good luck! [09:24] Don't hit the deck... [09:28] https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails. === stub changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge,stub] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === danilo_ is now known as danilos [10:50] hey henninge, do you have a few moments for me? [10:50] mrevell: shure [10:50] ;) [10:51] henninge, Axctually, don't worry. unping [10:52] mrevell: np, you can unping me any time ... ;) [10:52] heh === matsubara-afk is now known as matsubara === danilo_ is now known as danilos [11:46] stub: review done ;) === henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [11:46] w00t === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [14:35] beuno: could you do a ui review of this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-172501 ? [14:41] adeuring, sure, looking [14:42] beuno: thanks! [14:43] adeuring, screenshots? :) [14:43] beuno: sorry, forgot them. let me make them... [14:44] or do I need to really interact with this to review it? [14:45] beuno: nah, i don't think that's necessary. The main idea is: if you upload an attachment called something.diff but don't mark that as a patch, you're redirected to the attachment edit page where you see a note that LP thinks there is ann incoistency. [14:45] beuno: same thing, if you upload a file without the suffix .diff, but mark it as a patch [14:46] great, screenshots will do fine then [14:51] beuno: http://people.canonical.com/~adeuring/patch-check-1.png http://people.canonical.com/~adeuring/patch-check-2.png === matsubara is now known as matsubara-lunch [14:52] adeuring, thanks, that's enough for me to review [14:53] a few layout tweaks come to mind, I'll add them to the MP [14:53] beuno: OK [14:55] adeuring, so you upload the file, and if it detects either of the cases mentioned above, you get redirected here, correct? [14:56] beuno: right. And, BTW, the "patch" checkbox still has the value LP thinks is correct [14:56] the user must override that value again. [14:56] gotcha, so it will check/unchecs based on our guess === salgado is now known as salgado-lunch [14:57] beuno: yes [14:58] adeuring, perfect, thanks for the info. I think we'll need to re-jig the UI a bit so people understand what happened without reading all that text (which they won't) [14:58] beuno: yeah... [14:59] I'll chew on it for a bit and propose something, hopefully very easy to change :) [14:59] ok === deryck_ is now known as deryck === deryck is now known as deryck[lunch] === salgado-lunch is now known as salgado === beuno is now known as beuno-lunch [16:08] adeuring, starting to write the review, I'll finish it after lunch [16:08] beuno-lunch: ok [16:09] allenap, jtv: are you not on-call today? === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [16:09] EdwinGrubbs: I am, but I have a semi-critical bug to fix (checkwatches). [16:09] henninge: I will look at your mp now [16:09] ah [16:10] EdwinGrubbs: I saw that you assigned a review to me. I can look at it tomorrow... is that okay? [16:10] that's fine [16:10] EdwinGrubbs: great, thank you! Note: it's over-sized. [16:10] but it would be great to get it reviewed because it is likely to rot quickly. === matsubara-lunch is now known as matsubara === henninge_ is now known as henninge === deryck[lunch] is now known as deryck === beuno-lunch is now known as beuno [17:10] henninge: what is the purpose of IPersonRoles? [17:11] EdwinGrubbs: ;-) [17:12] EdwinGrubbs: to reduce noise in code from repeatedly having to do the ILaunchpadCelebrities dance. [17:13] EdwinGrubbs: it has a little history behind it, where I first created a helper file that provided functions to encapsulate this. [17:13] But an adapter on IPerson is much nicer to handle. [17:15] EdwinGrubbs: bug 503454 was the previous one that introuced it (well, the branch linked to it) [17:15] Bug #503454: Make checks for celebrity persons and roles easier [17:16] * henninge just realized that he forgot to link the branch on that bug. [17:17] done [17:32] danilos: fancy to review my fix for our critical bug? https://code.edge.launchpad.net/~henninge/launchpad/bug-506925-oops-export/+merge/17304 [17:33] danilos: I got Edwin busy on my big branch ... [17:33] or anybody else? ;-) [17:33] henninge, sure, if it's not longer than 275 lines ;) [17:34] henninge, whoops, sorry, it's 277 :) [17:34] danilos: hey, you've been peeking. [17:34] ;) [17:34] henninge, ok, I've got to start with a question: who's Kubla Kahn at pleasure dome? (you scare me sometimes, did I ever mention it? ;) [17:35] danilos: heh [17:35] danilos: ;) [17:35] danilos: I believe he is older than me. [17:35] ... [17:37] line 29, you should be able to re-raise the exception simply by calling "raise" I think [17:37] henninge, ^ [17:37] danilos: I copied that from the previous code but I believe this adds the file name to the message. [17:37] henninge, also, the check for UTF-8 is likely not bulletproof (i.e. does "UTF8" also work? I am not sure, but it might) [17:38] henninge, right, I guess that makes sense then [17:38] henninge, I see that bit is also copied code, so let's ignore it for now [17:38] henninge, i.e. we are not making anything worse for now [17:39] * henninge is good at copying other people's code [17:40] henninge, I also see you optimized it a bit, woohoo (i.e. do .encode() only once on the entire file content, and not separately on each message) [17:40] danilos: if the encoding was something like "UTF8" it would be caught on the second run of the method. [17:40] henninge, "run, method, run" [17:41] danilos: yes, it was the obvious thing to do. [17:41] it's probably a fast one, too. ;) [17:41] henninge, good job :) [17:41] henninge, that's all I am saying, no need to justify your actions :P [17:42] henninge, now, a comment [17:43] henninge, why not catch the exception in the callsite for _try_encode_file_content and do an "else" there? [17:43] * henninge tries to remember what else: did with try: ... [17:44] henninge, well, try...except [17:44] yeah, of course. [17:44] It executes if no exception was raised [17:44] or caught? [17:45] henninge, ok, so how can you get None for the return value, that's the question? [17:46] henninge, why not instead of returning none just try UTF-8 inside the method? it would be much saner to me [17:46] danilos: well, there is the logging bit. [17:46] henninge, i.e. pass in a list of encodings to try and just fail if it doesn't work [17:46] henninge, you can still log if it doesn't work with the first encoding :) [17:47] henninge, the only ugly side effect is the "update header in DB" bit [17:47] danilos: and what's in the method is the part that needs to be repeated. [17:47] henninge, good point [17:47] that's why I put it there [17:47] henninge, but the return values seem very weird (i.e. None indicates that it couldn't encode it; that's what exceptions are for) [17:48] danilos: ok, true [17:48] it is only meant to make sense in this context, though. [17:49] henninge, I know, but I'd still prefer it if you get re-raising UnicodeEncodeError out of the method and instead catch it in the callsite [17:49] henninge, and then you can rename the method from _try_encode... to _encode... and put it in a try: except: block [17:50] danilos: well, if it fails to encode using UTF-8, we don't have a plan to handle it. [17:51] danilos: so I don't want to catch it, let it oops. [17:51] henninge, I know, so throwing an exception then is fine [17:51] henninge, right, but what I am saying is that it would be much clearer to me if you didn't catch the exception and conditionally re-raise it in the helper method itself [17:51] we could, of course change that behaviour, log the incident and produce an empty pofile with a message. [17:52] henninge, i.e. assume the method will do the right thing, and catch the exception in the callsite and re-try the method with UTF-8 [17:52] but that is too much for this branch. [17:52] henninge, right, I am not suggesting that [17:52] henninge, so, my suggestion is simple: 1. get try/except out of _try_encode_file_content [17:53] danilos: that makes the method quite short and I'd add another method tod do the catching but that's fine. [17:53] henninge, 2. instead of checking weird value encoded_file_content == None for failure, change that to try...except on 105..122 [17:54] henninge, why would you add another method to do the catching? you change the "_try_encode...(); if encoded_file_content is None:..." to "try: _try_encode() except:..." [17:55] henninge, that would read naturally to me at least, because this code got me confused for a moment (it's not a big deal, but just want to make sure you understand what I mean before we either agree to ignore me or not :) === salgado changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue [salgado:] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:56] EdwinGrubbs, I just added a trivial one to the queue. it's a fix for bug 506581; can you take it? [17:56] Bug #506581: oops template page is out of center [17:56] henninge, you won't have to re-raise exception again, because _try_encode_file will raise it; we will lose the filename though, so that's perhaps an argument against my proposal [17:57] henninge, (because if we want to keep it, we'll have a nested try..except, which is very ugly) [17:57] danilos: http://paste.ubuntu.com/356151/ [17:57] henninge, so, I modify my suggestion in light of what I just said :) [17:58] henninge, yeah, that's what I meant, I like how that makes it much clearer, but we lose the filename now :) [17:58] danilos: can we live with that? [17:59] henninge, I can, but if you can't, it's not hard to fix while keeping code sane :) [17:59] henninge, for instance, reintroduce unconditional exception re-raising in _encode_file_content with the filename [18:00] danilos: I'd just need to add a try .. except to the second call. [18:00] henninge, it's not hard, and it's only slightly less performant, and slightly uglier [18:00] henninge, yeah, but we'd end up with nested try..excepts, I hate those :) [18:00] salgado: sure [18:00] henninge, if you don't, you can do it, but whenever I touch that code myself, I am probably going to kill it :) [18:00] danilos: I don't mind [18:00] loosing the file name [18:01] http://paste.ubuntu.com/356154/ [18:01] henninge, ok (that may mean some failing tests, but in general, we'll know enough context already) [18:01] same as before but with call sites fixed [18:02] henninge, yep, got me wondering for a minute :) [18:02] henninge, looks great if you ask me :) [18:03] danilos: I have another idea [18:03] salgado: where is your branch? [18:03] henninge, I am all ears (well, usually as well, but now specifically :) [18:04] EdwinGrubbs, the branch is lp:~salgado/launchpad/bug-506581. I also have a pastebin on the topic [18:04] EdwinGrubbs, I just added a

around the main content of the page. the rest is just indentation fixes [18:05] salgado: r=me [18:06] thanks EdwinGrubbs! [18:06] henninge, everything else looks good in the branch, so I am just waiting on your new idea before giving you r=me [18:09] danilos: wait ... [18:09] henninge, sure [18:11] henninge, I need to leave soon... very soon :) [18:12] danilos: never mind, it got too complicated [18:12] danilos: let's leave it as it is. [18:12] henninge, heh, ok, you've got approval for http://paste.ubuntu.com/356154/ approach or just re-introducing error message extension there [18:13] danilos: thanks. [18:13] * henninge has to leave, too. === gary_poster is now known as gary-lunch [18:13] henninge, good job with it, thanks a lot! [18:14] danilos: pleasure. === danilos is now known as daniloff === gary-lunch is now known as gary_poster === salgado is now known as salgado-afk [20:49] Hi EdwinGrubbs, still chewing on my monster branch? ;-) [20:49] jml: https://code.edge.launchpad.net/~thumper/launchpad/fix-codeimport-failure/+merge/17315 - diff still generating === matsubara is now known as matsubara-afk [20:51] henninge: yep. I don't think there are any serious problems, just minor tweaks, so I'll just put them in the full review as opposed to IRC, since no discussion is really necessary. [20:52] EdwinGrubbs: cool, thanks ;) [21:06] henninge: review sent === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [21:07] EdwinGrubbs: thank you! :-) [21:36] could someone please review this https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166 [21:36] it's ok, you don't have to be an ISD hacker. [21:44] thumper, https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166 [21:44] * thumper looks [22:32] noodles775: https://code.edge.launchpad.net/~thumper/launchpad/review-clamining-fix/+merge/17323 [22:50] does someone want to review a REALLY EXCITING branch? [22:51] mwhudson: ok [22:52] thumper: bzr+ssh://bazaar.launchpad.net/~bzr/bzr-git/trunk/ [22:52] no [22:52] thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/update-bzr-git-dulwich/+merge/17324 [22:54] mwhudson: done