[02:50] <rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-code-windmill-tests/+merge/17268
[07:00] <stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails.
[09:16] <allenap> henninge: I assume the topic is out of date...?
[09:16] <henninge> allenap: well, bac should be in bed by now ... ;-)
[09:17] <henninge> allenap:  My branch is not reviewed yet and bac has not claimed it.
[09:17] <allenap> henninge: And he's a very naughty boy if he's not.
[09:17] <allenap> henninge: I'll go and have a look.
[09:17] <henninge> ;-) definitely
[09:17] <henninge> allenap: thanks
[09:17] <henninge> allenap: it's over-sized.
[09:22] <allenap> 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.
[09:23] <henninge> allenap: right, I read that, too. Good luck!
[09:24] <henninge> Don't hit the deck...
[09:28] <stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails.
[10:50] <mrevell> hey henninge, do you have a few moments for me?
[10:50] <henninge> mrevell: shure
[10:50] <henninge> ;)
[10:51] <mrevell> henninge, Axctually, don't worry. unping
[10:52] <henninge> mrevell: np, you can unping me any time ... ;)
[10:52] <mrevell> heh
[11:46] <henninge> stub: review done ;)
[11:46] <stub> w00t
[14:35] <adeuring> beuno: could you do a ui review of this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-172501 ?
[14:41] <beuno> adeuring, sure, looking
[14:42] <adeuring> beuno: thanks!
[14:43] <beuno> adeuring, screenshots?   :)
[14:43] <adeuring> beuno: sorry, forgot them. let me make them...
[14:44] <beuno> or do I need to really interact with this to review it?
[14:45] <adeuring> 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] <adeuring> beuno: same thing, if you upload a file without the suffix .diff, but mark it as a patch
[14:46] <beuno> great, screenshots will do fine then
[14:51] <adeuring> beuno: http://people.canonical.com/~adeuring/patch-check-1.png http://people.canonical.com/~adeuring/patch-check-2.png
[14:52] <beuno> adeuring, thanks, that's enough for me to review
[14:53] <beuno> a few layout tweaks come to mind, I'll add them to the MP
[14:53] <adeuring> beuno: OK
[14:55] <beuno> adeuring, so you upload the file, and if it detects either of the cases mentioned above, you get redirected here, correct?
[14:56] <adeuring> beuno: right. And, BTW, the  "patch" checkbox still has the value LP thinks is correct
[14:56] <adeuring> the user must override that value again.
[14:56] <beuno> gotcha, so it will check/unchecs based on our guess
[14:57] <adeuring> beuno: yes
[14:58] <beuno> 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] <adeuring> beuno: yeah...
[14:59] <beuno> I'll chew on it for a bit and propose something, hopefully very easy to change   :)
[14:59] <adeuring> ok
[16:08] <beuno-lunch> adeuring, starting to write the review, I'll finish it after lunch
[16:08] <adeuring> beuno-lunch: ok
[16:09] <EdwinGrubbs> allenap, jtv: are you not on-call today?
[16:09] <allenap> EdwinGrubbs: I am, but I have a semi-critical bug to fix (checkwatches).
[16:09] <EdwinGrubbs> henninge: I will look at your mp now
[16:09] <EdwinGrubbs> ah
[16:10] <allenap> EdwinGrubbs: I saw that you assigned a review to me. I can look at it tomorrow... is that okay?
[16:10] <EdwinGrubbs> that's fine
[16:10] <henninge> EdwinGrubbs: great, thank you! Note: it's over-sized.
[16:10] <henninge> but it would be great to get it reviewed because it is likely to rot quickly.
[17:10] <EdwinGrubbs> henninge: what is the purpose of IPersonRoles?
[17:11] <henninge> EdwinGrubbs: ;-)
[17:12] <henninge> EdwinGrubbs: to reduce noise in code from repeatedly having to do the ILaunchpadCelebrities dance.
[17:13] <henninge> EdwinGrubbs: it has a little history behind it, where I first created a helper file that provided functions to encapsulate this.
[17:13] <henninge> But an adapter on IPerson is much nicer to handle.
[17:15] <henninge> EdwinGrubbs: bug 503454 was the previous one that introuced it (well, the branch linked to it)
[17:15] <mup> Bug #503454: Make checks for celebrity persons and roles easier <tech-debt> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/503454>
[17:16]  * henninge just realized that he forgot to link the branch on that bug.
[17:17] <henninge> done
[17:32] <henninge> 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] <henninge> danilos: I got Edwin busy on my big branch ...
[17:33] <henninge> or anybody else? ;-)
[17:33] <danilos> henninge, sure, if it's not longer than 275 lines ;)
[17:34] <danilos> henninge, whoops, sorry, it's 277 :)
[17:34] <henninge> danilos: hey, you've been peeking.
[17:34] <henninge> ;)
[17:34] <danilos> 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] <henninge> danilos: heh
[17:35] <henninge> danilos: ;)
[17:35] <henninge> danilos: I believe he is older than me.
[17:35] <henninge> ...
[17:37] <danilos> line 29, you should be able to re-raise the exception simply by calling "raise" I think
[17:37] <danilos> henninge, ^
[17:37] <henninge> danilos: I copied that from the previous code but I believe this adds the file name to the message.
[17:37] <danilos> 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] <danilos> henninge, right, I guess that makes sense then
[17:38] <danilos> henninge, I see that bit is also copied code, so let's ignore it for now
[17:38] <danilos> henninge, i.e. we are not making anything worse for now
[17:39]  * henninge is good at copying other people's code
[17:40] <danilos> 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] <henninge> danilos: if the encoding was something like "UTF8" it would be caught on the second run of the method.
[17:40] <danilos> henninge, "run, method, run"
[17:41] <henninge> danilos: yes, it was the obvious thing to do.
[17:41] <henninge> it's probably a fast one, too. ;)
[17:41] <danilos> henninge, good job :)
[17:41] <danilos> henninge, that's all I am saying, no need to justify your actions :P
[17:42] <danilos> henninge, now, a comment
[17:43] <danilos> 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] <danilos> henninge, well, try...except
[17:44] <henninge> yeah, of course.
[17:44] <henninge> It executes if no exception was raised
[17:44] <henninge> or caught?
[17:45] <danilos> henninge, ok, so how can you get None for the return value, that's the question?
[17:46] <danilos> henninge, why not instead of returning none just try UTF-8 inside the method? it would be much saner to me
[17:46] <henninge> danilos: well, there is the logging bit.
[17:46] <danilos> henninge, i.e. pass in a list of encodings to try and just fail if it doesn't work
[17:46] <danilos> henninge, you can still log if it doesn't work with the first encoding :)
[17:47] <danilos> henninge, the only ugly side effect is the "update header in DB" bit
[17:47] <henninge> danilos: and what's in the method is the part that needs to be repeated.
[17:47] <danilos> henninge, good point
[17:47] <henninge> that's why I put it there
[17:47] <danilos> 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] <henninge> danilos: ok, true
[17:48] <henninge> it is only meant to make sense in this context, though.
[17:49] <danilos> 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] <danilos> henninge, and then you can rename the method from _try_encode... to _encode... and put it in a try: except: block
[17:50] <henninge> danilos: well, if it fails to encode using UTF-8, we don't have a plan to handle it.
[17:51] <henninge> danilos: so I don't want to catch it, let it oops.
[17:51] <danilos> henninge, I know, so throwing an exception then is fine
[17:51] <danilos> 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] <henninge> we could, of course change that behaviour, log the incident and produce an empty pofile with a message.
[17:52] <danilos> 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] <henninge> but that is too much for this branch.
[17:52] <danilos> henninge, right, I am not suggesting that
[17:52] <danilos> henninge, so, my suggestion is simple: 1. get try/except out of _try_encode_file_content
[17:53] <henninge> danilos: that makes the method quite short and I'd add another method tod do the catching but that's fine.
[17:53] <danilos> henninge, 2. instead of checking weird value encoded_file_content == None for failure, change that to try...except on 105..122
[17:54] <danilos> 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] <danilos> 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 :)
[17:56] <salgado> EdwinGrubbs, I just added a trivial one to the queue.  it's a fix for bug 506581; can you take it?
[17:56] <mup> Bug #506581: oops template page is out of center <ui> <Launchpad Foundations:New for salgado> <https://launchpad.net/bugs/506581>
[17:56] <danilos> 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] <danilos> henninge, (because if we want to keep it, we'll have a nested try..except, which is very ugly)
[17:57] <henninge> danilos: http://paste.ubuntu.com/356151/
[17:57] <danilos> henninge, so, I modify my suggestion in light of what I just said :)
[17:58] <danilos> henninge, yeah, that's what I meant, I like how that makes it much clearer, but we lose the filename now :)
[17:58] <henninge> danilos: can we live with that?
[17:59] <danilos> henninge, I can, but if you can't, it's not hard to fix while keeping code sane :)
[17:59] <danilos> henninge, for instance, reintroduce unconditional exception re-raising in _encode_file_content with the filename
[18:00] <henninge> danilos: I'd just need to add a try .. except to the second call.
[18:00] <danilos> henninge, it's not hard, and it's only slightly less performant, and slightly uglier
[18:00] <danilos> henninge, yeah, but we'd end up with nested try..excepts, I hate those :)
[18:00] <EdwinGrubbs> salgado: sure
[18:00] <danilos> 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] <henninge> danilos: I don't mind
[18:00] <henninge> loosing the file name
[18:01] <henninge> http://paste.ubuntu.com/356154/
[18:01] <danilos> henninge, ok (that may mean some failing tests, but in general, we'll know enough context already)
[18:01] <henninge> same as before but with call sites fixed
[18:02] <danilos> henninge, yep, got me wondering for a minute :)
[18:02] <danilos> henninge, looks great if you ask me :)
[18:03] <henninge> danilos: I have another idea
[18:03] <EdwinGrubbs> salgado: where is your branch?
[18:03] <danilos> henninge, I am all ears (well, usually as well, but now specifically :)
[18:04] <salgado> EdwinGrubbs, the branch is lp:~salgado/launchpad/bug-506581.  I also have a pastebin on the topic
[18:04] <salgado> EdwinGrubbs, I just added a <div class="yui-d0"> around the main content of the page.  the rest is just indentation fixes
[18:05] <EdwinGrubbs> salgado: r=me
[18:06] <salgado> thanks EdwinGrubbs!
[18:06] <danilos> henninge, everything else looks good in the branch, so I am just waiting on your new idea before giving you r=me
[18:09] <henninge> danilos: wait ...
[18:09] <danilos> henninge, sure
[18:11] <danilos> henninge, I need to leave soon... very soon :)
[18:12] <henninge> danilos: never mind, it got too complicated
[18:12] <henninge> danilos: let's leave it as it is.
[18:12] <danilos> henninge, heh, ok, you've got approval for http://paste.ubuntu.com/356154/ approach or just re-introducing error message extension there
[18:13] <henninge> danilos: thanks.
[18:13]  * henninge has to leave, too.
[18:13] <danilos> henninge, good job with it, thanks a lot!
[18:14] <henninge> danilos: pleasure.
[20:49] <henninge> Hi EdwinGrubbs, still chewing on my monster branch? ;-)
[20:49] <thumper> jml: https://code.edge.launchpad.net/~thumper/launchpad/fix-codeimport-failure/+merge/17315 - diff still generating
[20:51] <EdwinGrubbs> 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] <henninge> EdwinGrubbs: cool, thanks ;)
[21:06] <EdwinGrubbs> henninge: review sent
[21:07] <henninge> EdwinGrubbs: thank you! :-)
[21:36] <jml> could someone please review this https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166
[21:36] <jml> it's ok, you don't have to be an ISD hacker.
[21:44] <jml> thumper, https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166
[21:44]  * thumper looks
[22:32] <thumper> noodles775: https://code.edge.launchpad.net/~thumper/launchpad/review-clamining-fix/+merge/17323
[22:50] <mwhudson> does someone want to review a REALLY EXCITING branch?
[22:51] <thumper> mwhudson: ok
[22:52] <mwhudson> thumper: bzr+ssh://bazaar.launchpad.net/~bzr/bzr-git/trunk/
[22:52] <mwhudson> no
[22:52] <mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/update-bzr-git-dulwich/+merge/17324
[22:54] <thumper> mwhudson: done