| rockstar | thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-code-windmill-tests/+merge/17268 | 02:50 |
|---|---|---|
| === stub1 is now known as stub | ||
| stub | https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails. | 07:00 |
| allenap | henninge: I assume the topic is out of date...? | 09:16 |
| henninge | allenap: well, bac should be in bed by now ... ;-) | 09:16 |
| === 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 | ||
| 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:17 |
| 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:22 |
| === 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 | ||
| henninge | allenap: right, I read that, too. Good luck! | 09:23 |
| henninge | Don't hit the deck... | 09:24 |
| stub | https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17278 to be reviewed, hopefully to land before the next staging rebuild fails. | 09:28 |
| === 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 | ||
| mrevell | hey henninge, do you have a few moments for me? | 10:50 |
| henninge | mrevell: shure | 10:50 |
| henninge | ;) | 10:50 |
| mrevell | henninge, Axctually, don't worry. unping | 10:51 |
| henninge | mrevell: np, you can unping me any time ... ;) | 10:52 |
| mrevell | heh | 10:52 |
| === matsubara-afk is now known as matsubara | ||
| === danilo_ is now known as danilos | ||
| henninge | stub: review done ;) | 11:46 |
| === 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 | ||
| stub | w00t | 11:46 |
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| adeuring | beuno: could you do a ui review of this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-172501 ? | 14:35 |
| beuno | adeuring, sure, looking | 14:41 |
| adeuring | beuno: thanks! | 14:42 |
| beuno | adeuring, screenshots? :) | 14:43 |
| adeuring | beuno: sorry, forgot them. let me make them... | 14:43 |
| beuno | or do I need to really interact with this to review it? | 14:44 |
| 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:45 |
| beuno | great, screenshots will do fine then | 14:46 |
| adeuring | beuno: http://people.canonical.com/~adeuring/patch-check-1.png http://people.canonical.com/~adeuring/patch-check-2.png | 14:51 |
| === matsubara is now known as matsubara-lunch | ||
| beuno | adeuring, thanks, that's enough for me to review | 14:52 |
| beuno | a few layout tweaks come to mind, I'll add them to the MP | 14:53 |
| adeuring | beuno: OK | 14:53 |
| beuno | adeuring, so you upload the file, and if it detects either of the cases mentioned above, you get redirected here, correct? | 14:55 |
| 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:56 |
| === salgado is now known as salgado-lunch | ||
| adeuring | beuno: yes | 14:57 |
| 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:58 |
| beuno | I'll chew on it for a bit and propose something, hopefully very easy to change :) | 14:59 |
| adeuring | ok | 14:59 |
| === 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 | ||
| beuno-lunch | adeuring, starting to write the review, I'll finish it after lunch | 16:08 |
| adeuring | beuno-lunch: ok | 16:08 |
| EdwinGrubbs | allenap, jtv: are you not on-call today? | 16:09 |
| === 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 | ||
| 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:09 |
| 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. | 16:10 |
| === 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 | ||
| EdwinGrubbs | henninge: what is the purpose of IPersonRoles? | 17:10 |
| henninge | EdwinGrubbs: ;-) | 17:11 |
| henninge | EdwinGrubbs: to reduce noise in code from repeatedly having to do the ILaunchpadCelebrities dance. | 17:12 |
| 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:13 |
| 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:15 |
| * henninge just realized that he forgot to link the branch on that bug. | 17:16 | |
| henninge | done | 17:17 |
| 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:32 |
| 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:33 |
| 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:34 |
| henninge | danilos: heh | 17:35 |
| henninge | danilos: ;) | 17:35 |
| henninge | danilos: I believe he is older than me. | 17:35 |
| henninge | ... | 17:35 |
| 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:37 |
| 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:38 |
| * henninge is good at copying other people's code | 17:39 | |
| 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:40 |
| 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:41 |
| danilos | henninge, now, a comment | 17:42 |
| 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:43 | |
| 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:44 |
| danilos | henninge, ok, so how can you get None for the return value, that's the question? | 17:45 |
| 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:46 |
| 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:47 |
| henninge | danilos: ok, true | 17:48 |
| henninge | it is only meant to make sense in this context, though. | 17:48 |
| 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:49 |
| henninge | danilos: well, if it fails to encode using UTF-8, we don't have a plan to handle it. | 17:50 |
| 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:51 |
| 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:52 |
| 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:53 |
| 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:54 |
| 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:55 |
| === salgado changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue [salgado:<http://paste.ubuntu.com/356150/>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| 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:56 |
| 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:57 |
| 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:58 |
| 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 | 17:59 |
| 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:00 |
| 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:01 |
| danilos | henninge, yep, got me wondering for a minute :) | 18:02 |
| danilos | henninge, looks great if you ask me :) | 18:02 |
| 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:03 |
| 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:04 |
| EdwinGrubbs | salgado: r=me | 18:05 |
| 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:06 |
| henninge | danilos: wait ... | 18:09 |
| danilos | henninge, sure | 18:09 |
| danilos | henninge, I need to leave soon... very soon :) | 18:11 |
| 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:12 |
| henninge | danilos: thanks. | 18:13 |
| * henninge has to leave, too. | 18:13 | |
| === gary_poster is now known as gary-lunch | ||
| danilos | henninge, good job with it, thanks a lot! | 18:13 |
| henninge | danilos: pleasure. | 18:14 |
| === danilos is now known as daniloff | ||
| === gary-lunch is now known as gary_poster | ||
| === salgado is now known as salgado-afk | ||
| 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:49 |
| === matsubara is now known as matsubara-afk | ||
| 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:51 |
| henninge | EdwinGrubbs: cool, thanks ;) | 20:52 |
| EdwinGrubbs | henninge: review sent | 21:06 |
| === 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 | ||
| henninge | EdwinGrubbs: thank you! :-) | 21:07 |
| 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:36 |
| jml | thumper, https://code.edge.launchpad.net/~jml/canonical-identity-provider/upcall-testcase/+merge/17166 | 21:44 |
| * thumper looks | 21:44 | |
| thumper | noodles775: https://code.edge.launchpad.net/~thumper/launchpad/review-clamining-fix/+merge/17323 | 22:32 |
| mwhudson | does someone want to review a REALLY EXCITING branch? | 22:50 |
| thumper | mwhudson: ok | 22:51 |
| 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:52 |
| thumper | mwhudson: done | 22:54 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
