=== flacoste is now known as flacoste_afk
=== mup is now known as _mup_
[03:49] <thumper> https://code.edge.launchpad.net/~thumper/launchpad/linked-bug-visibility/+merge/27466 any one?
[04:36] <thumper> stub: can you do a pretty trivial review for me?
[04:36] <thumper> stub: ^^^
[04:43]  * stub looks
[04:46] <stub> wtf happened to lp? All my controls are hidden under the subscribers portlet.
[04:48] <thumper> stub: a "fix" to the template
[04:48] <thumper> a fix for the fix has landed already
[04:49] <stub> approved
[04:49] <thumper> ta
[05:41] <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:50] <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:52] <stub> And TestDiffFormatter would be much more readable as a doctest, but that is old too.
[05:54] <thumper> I'd argue that last point :)
[05:56] <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:57] <stub> Oh... I see. Linebreaks.
[05:58] <stub> r=stub
=== 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
[09:35] <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
=== 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
[09:35] <noodles775> (it's just part one, a second branch will follow it).
[09:36] <henninge> noodles775: Hi! Sure, I will look at it ;)
[09:37] <noodles775> Thanks henninge.
=== 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
[14:16] <abentley> henninge_, sinzui's branch has already been reviewed.
[14:17] <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
=== 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
[16:59] <leonardr> henninge, abentley: i'd like to add https://code.edge.launchpad.net/~leonardr/lazr.restful/cache-unauthorized/+merge/27523 to the queue
[17:00] <abentley> leonardr, I can have a look at it.
[17:00] <leonardr> great
=== 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
[19:37] <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:41] <abentley> leonardr, rockstar is away for a week.
[19:41] <leonardr> ok...
=== 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
[19:41] <leonardr> abentley: i've assembled two incremental diffs, maybe you can review them?
[19:42] <leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/toggle-representation-cache/+merge/26725
[19:46] <abentley> leonardr, rather than catching Exception, can you name the exceptions you want to handle?
[19:47] <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:52] <abentley> leonardr, on line 126 of the real patch you have "ocument" rather than "document".
[19:52] <leonardr> will fix
[20:26] <leonardr> abentley: catching storm.exceptions.ClassInfoError seems to be as good as catching Exception
[20:28] <abentley> leonardr, excellent.
[20:28] <leonardr> abentley, any other comments?
[20:29] <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:31] <abentley> leonardr, the get(key, default) it handles the case where get returns a value that evaluates to False better.
[20:32] <abentley> leonardr, is it always storm.info.get_obj_info that raises storm.exceptions.ClassInfoError?
[20:33] <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:34] <abentley> leonardr, sounds good.
[20:35] <abentley> leonardr, aside from that, it looks fine to me.
[20:35] <leonardr> ok, cool
[20:36] <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:37] <abentley> leonardr, that's fine, then.
[20:44] <abentley> leonardr, btw, even if the except clause didn't return, you could still narrow the scope using try/except/else.
[20:45] <leonardr> abentley: true, thanks
=== matsubara is now known as matsubara-afk