| === flacoste is now known as flacoste_afk | ||
| === mup is now known as _mup_ | ||
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/linked-bug-visibility/+merge/27466 any one? | 03:49 |
|---|---|---|
| thumper | stub: can you do a pretty trivial review for me? | 04:36 |
| thumper | stub: ^^^ | 04:36 |
| * stub looks | 04:43 | |
| stub | wtf happened to lp? All my controls are hidden under the subscribers portlet. | 04:46 |
| thumper | stub: a "fix" to the template | 04:48 |
| thumper | a fix for the fix has landed already | 04:48 |
| stub | approved | 04:49 |
| thumper | ta | 04:49 |
| thumper | stub: https://code.edge.launchpad.net/~thumper/launchpad/diff-tales/+merge/27469 fixes the diff formatter where sql comments are removed :) | 05:41 |
| thumper | if you feel like another review | 05:41 |
| stub | thumper: I find it worrying we are munging HTML rather than munging strings before we generate the HTML (break_long_words), and that the method name doesn't indicate it accepts HTML input rather than plain text. | 05:50 |
| stub | But that is old code moved | 05:50 |
| stub | And TestDiffFormatter would be much more readable as a doctest, but that is old too. | 05:52 |
| thumper | I'd argue that last point :) | 05:54 |
| stub | At the very least, tripple quoted strings in the asserts using dedent("""\ | 05:56 |
| stub | Or better, expected=dedent("""\ then 'assertEquals(expected, formatterapimagic)' | 05:56 |
| stub | Oh... I see. Linebreaks. | 05:57 |
| stub | r=stub | 05:58 |
| === henninge changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Morning henninge ! | 09:35 |
| noodles775 | I've got an MP ready if you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/536700-present-packagebuilds-in-ppa-context/+merge/27477 | 09:35 |
| === noodles775 changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: - || queue: [sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | (it's just part one, a second branch will follow it). | 09:35 |
| henninge | noodles775: Hi! Sure, I will look at it ;) | 09:36 |
| noodles775 | Thanks henninge. | 09:37 |
| === henninge changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: noodles775 || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === flacoste_afk is now known as flacoste | ||
| abentley | henninge_, sinzui's branch has already been reviewed. | 14:16 |
| abentley | On Call: henninge, abentley || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | 14:17 |
| sinzui | yes, rockstar reviewed it yesterday | 14:17 |
| === abentley changed the topic of #launchpad-reviews to: On Call: henninge, abentley || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === henninge_ is now known as henninge | ||
| leonardr | henninge, abentley: i'd like to add https://code.edge.launchpad.net/~leonardr/lazr.restful/cache-unauthorized/+merge/27523 to the queue | 16:59 |
| abentley | leonardr, I can have a look at it. | 17:00 |
| leonardr | great | 17:00 |
| === abentley changed the topic of #launchpad-reviews to: On Call: henninge, abentley || reviewing: noodles775, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-lunch | ||
| === bac` is now known as bac | ||
| === matsubara-lunch is now known as matsubara | ||
| leonardr | rockstar, can i get you to look at https://code.edge.launchpad.net/~leonardr/launchpad/toggle-representation-cache one more time? i've made a few more changes to get the tests to pass | 19:37 |
| abentley | leonardr, rockstar is away for a week. | 19:41 |
| leonardr | ok... | 19:41 |
| === abentley changed the topic of #launchpad-reviews to: On Call: abentley || reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| leonardr | abentley: i've assembled two incremental diffs, maybe you can review them? | 19:41 |
| leonardr | https://code.edge.launchpad.net/~leonardr/launchpad/toggle-representation-cache/+merge/26725 | 19:42 |
| abentley | leonardr, rather than catching Exception, can you name the exceptions you want to handle? | 19:46 |
| leonardr | abentley: let me check. maybe there's just one storm exception | 19:47 |
| leonardr | i won't know for sure until i run the ec2 test though | 19:47 |
| abentley | leonardr, on line 126 of the real patch you have "ocument" rather than "document". | 19:52 |
| leonardr | will fix | 19:52 |
| leonardr | abentley: catching storm.exceptions.ClassInfoError seems to be as good as catching Exception | 20:26 |
| abentley | leonardr, excellent. | 20:28 |
| leonardr | abentley, any other comments? | 20:28 |
| abentley | leonardr, in get_by_key, why not use self.client.get(key, default)? | 20:29 |
| leonardr | abentley: i think that should be fine | 20:29 |
| abentley | leonardr, the get(key, default) it handles the case where get returns a value that evaluates to False better. | 20:31 |
| abentley | leonardr, is it always storm.info.get_obj_info that raises storm.exceptions.ClassInfoError? | 20:32 |
| leonardr | abentley: probably, but i'm not sure. now that the except clause returns, i can move the other code outside the try block and see if ec2 takes it | 20:33 |
| abentley | leonardr, sounds good. | 20:34 |
| abentley | leonardr, aside from that, it looks fine to me. | 20:35 |
| leonardr | ok, cool | 20:35 |
| leonardr | abentley, the reason for .get(key) or None is that memcached 'get' doesn't take a default | 20:36 |
| leonardr | how about this: | 20:36 |
| leonardr | value = self.client.get(key) | 20:36 |
| leonardr | if value is None: | 20:36 |
| leonardr | value = default | 20:36 |
| leonardr | return value | 20:36 |
| abentley | leonardr, that's fine, then. | 20:37 |
| abentley | leonardr, btw, even if the except clause didn't return, you could still narrow the scope using try/except/else. | 20:44 |
| leonardr | abentley: true, thanks | 20:45 |
| === matsubara is now known as matsubara-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!