=== abentley1 is now known as abentley
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== mrevell is now known as mrevell-lunch
[12:34] <bac> hi gmb.  can i put a review on your queue?
[12:35] <bac> noodles775: can you do a UI review for me?
[12:35] <noodles775> bac: sure
[12:35] <bac> noodles775: thanks, i just added you to https://code.edge.launchpad.net/~bac/launchpad/bug-436980-mugshots/+merge/13250
[12:36] <gmb> bac: Sure.
[12:37] <bac> gmb: cool. it's the one i just posted
[12:37] <gmb> Righto.
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== ursula is now known as Ursinha
[12:47] <gmb> bac: Looks good. r=me.
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
[12:47] <bac> thanks gmb
=== mrevell-lunch is now known as mrevell
[13:00] <noodles775> bac: r=me (one suggestion on the MP).
[13:00] <noodles775> Looks great!
[13:00] <bac> thanks noodles775. do i have to have beuno give it a look too?
[13:01] <noodles775> bac: if he's around, it'd be great (for my learning). beuno ?
[13:22] <matsubara> !
[13:22] <matsubara> oops
[13:30] <beuno> hi bac, noodles775 
[13:30] <noodles775> hi beuno :)
[13:34] <noodles775> beuno: I'm just heading for lunch, but it'd be great if you could any comments you have to the MP, thanks!
[13:34] <beuno> noodles775, bug-436980-mugshots?
[13:34] <noodles775> beuno: yep
[13:34] <beuno> noodles775, on it
[13:57] <beuno> bac, noodles775, I'm suprised how many things I managed to squeeze out of that page, but, anyway, reviewed
[13:57] <bac> beuno: squeeze?
[13:58] <beuno> bac, long review for such a small and fairly unused page
[13:58] <bac> beuno: it already uses 32
[13:58] <bac> beuno: so much for not reading the merge proposal!  :)
[13:58] <beuno> bac, as you can see, I follow my guidelines to the letter
[13:59] <beuno> ah, 8 en dev
[13:59] <beuno> :)
=== bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== ursula is now known as Ursinha
[15:04] <bac> hi noodles775
[15:06] <noodles775> Hi bac
[15:06] <bac> noodles775: thanks for the CSS suggestion.  much nicer.
[15:06] <noodles775> bac: np! glad it was useful.
[15:06] <bac> noodles775: were you recommending changing to "Member photos" for the label, which affects h1, breadcrumbs, and page title?  that's what i've done and it seems much better.
[15:08] <noodles775> bac: yes, that seems to be the consensus on the lp-dev email discussion I sent out last wednesday... (I personally prefer the h1 to be different from the leaf breadcrumb, but that's beside the point :) ).
=== bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -,mrevell || queue: [] || This channel is logged: http://irclogs.ubuntu.com
[15:37] <bac> hi mrevell
=== gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: mrevell || queue: [] || This channel is logged: http://irclogs.ubuntu.com
[15:38]  * gmb -> afk for a while
[15:38] <mrevell> bac hi, on a call atm
[15:38] <bac> see ya gmb
[15:38] <bac> mrevell: ok, i'm looking at your editpgp review.  give me a shout when you have a sec.
[15:52] <abentley> bac: Could you please review https://code.launchpad.net/~abentley/launchpad/reuse-refactor/+merge/13283 ?
[15:53] <bac> abentley: sure
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: mrevell || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com
[15:58] <bac> abentley: you aren't using the refactored code anywhere yet, right?  so the only demo is bug comments?
[15:58] <abentley> bac: Right.
[15:58] <abentley> bac: I'm just polishing up the branch that uses it for merge proposals.
[15:59] <bac> abentley: ok
[16:11] <bac> hi rockstar
[16:14] <bac> abentley: this is unrelated to your branch, but do merge proposals now update the diff when new revisions of the branch are pushed?  i think i just saw that happen on one of my MPs.
[16:14] <abentley> bac: Yes, they now do.
[16:15] <bac> abentley: that seems strange to me.  you lose the historical context for the review.
[16:16] <abentley> I resisted it at first, too, but users demanded it.
[16:16] <abentley> bac: We will be linking reviews to the particular diff they relate to.
[16:16] <bac> abentley: an incremental added to the MP would be fantastic.
[16:16] <bac> abentley: ok.  glad to know my eyes weren't deceiving me.
[16:20] <abentley> Incremental diffs have been requested, too.  We would like to do them, but they are not easy to add, because if there was a merge, you don't want to include changes from that merge.
=== EdwinGrubbs is now known as Edwin-afk
[16:40] <rockstar> bac, hi.
[16:40] <bac> hi rockstar -- i was going to ask you about MP diffs but had the chat with abentley.
[16:41] <rockstar> bac, great.  I didn't realize that my IRC client wasn't connected until just now.
[16:41] <rockstar> (no wonder I was being so productive)
[16:42] <mrevell> hi bac, off my call now
[16:45] <bac> abentley: r=bac.  wonderful change.
[16:45] <bac> hi mrevell. i had some questions about your editgpg branch
[16:45] <mrevell> bac: Ah, great, I hope I can answer them
[16:46] <bac> mrevell: first, not your fault,but i saw a typo in that help section.  "a keyservers" i think it was.  it'll be obvious when you see it.
[16:46] <mrevell> bac: I'll fix that (almost certainly is fault). Thanks.
[16:47] <bac> mrevell: more to your content,  having the note saying it'll take a while followed by the next set of instructions beginning with "now (do this)" reads funny.
[16:47] <abentley> bac: Thanks!
[16:47] <bac> i wonder if the contents of the note shouldn't just be moved into the next step and worded so that it makes sense as a next step
[16:47] <mrevell> bac: Hmm, yes, good suggestion. I'll do that.
[16:48] <bac> mrevell: ok, great.  i'll put that in your MP and approve it now.  thanks for the branch.
[16:48] <abentley> bac: What would you change the inline comment to?
[16:49] <mrevell> thank you bac
[16:49] <bac> abentley: just move it up to precede the line.
[16:49] <abentley> bac: Will do.
[16:49] <bac> abentley: i like those little end-of-the-line comments myself...
[16:54] <bac> mrevell: also, it might be good if you were to reword the error message one gets when trying to import a GPG key that isn't yet available
[16:54] <bac> mrevell: right now it suggests submitting directly to our keyserver, but even that has a delay
[16:55] <mrevell> Ah, right. thanks bac, I'll take a look at that.
[16:55] <bac> mrevell: it's in c/l/browser/logintoken.py
[16:56] <mrevell> thanks bac
[16:56] <bac> mrevell: oops, it's in lp/registry/templates/person-editpgpkey.pt
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: mrevell || queue: [] || This channel is logged: http://irclogs.ubuntu.com
[17:02] <mrevell> ah, that sounds familiar, thanks bac
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== salgado is now known as salgado-lunch
=== beuno is now known as beuno-lunch
=== matsubara is now known as matsubara-lunch
=== salgado-lunch is now known as salgado
[18:45] <bac> bigjools: your review is done
=== beuno-lunch is now known as beuno
[18:48] <bac> salgado: i will if you ask over here!
[18:49] <adeuring> bac: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-4/+merge/13300 ?
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: salgado || queue: [abel] || This channel is logged: http://irclogs.ubuntu.com
[18:51] <bac> adeuring: i will.  add me to the MP?
[18:52] <adeuring> bac: thanks, done
[19:02] <achuni> hi, is it normal to get mails about Launchpad Internal Error when notifying people about proposals to merge?
[19:03] <achuni> (it was a proposal to merge in to Launchpad itself, which is why I ask here)
=== matsubara-lunch is now known as matsubara
[19:06] <salgado> achuni, I just got one.  can you give me the OOPS ID of yours?
[19:07] <achuni> salgado: OOPS-1382MPCJ5
[19:07] <salgado> abentley, around?  I just got https://lp-oops.canonical.com/oops.py/?oopsid=1382CMP1 when submitting a merge proposal
[19:08] <salgado> and achuni is a completely different one, it seems
[19:12] <achuni> anybody up for reviewing https://code.launchpad.net/~canonical-isd-hackers/launchpad/sso-new-branding/+merge/13304 ?
[19:27] <bigjools> thanks bac!
[19:46] <abentley> salgado: That is bug #436325
[19:46] <mup> Bug #436325: Diffstat generation chokes on binaries (and others) <oops> <Launchpad Bazaar Integration:Triaged by abentley> <https://launchpad.net/bugs/436325>
[19:47] <salgado> abentley, and did you see achuni's OOPS? OOPS-1382MPCJ5
[19:48] <bac> achuni: i'll review it in a bit
[19:48] <abentley> salgado: That's a bug introduced by thumper.  I'm not sure whether it has been reported.
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel || queue: [achuni] || This channel is logged: http://irclogs.ubuntu.com
[19:49] <salgado> achuni, ^
[19:49] <achuni> abentley: should I report?
[19:49] <achuni> abentley: thanks for looking in to it
[19:50] <abentley> achuni: up to to you.
[19:53] <bac> adeuring: for pci_ids the docstring says you return a tuple but you return a list
[19:54] <adeuring> bac: argh... fiexd.
[19:54] <bac> adeuring: i think returning the tuple would be best
[19:55] <adeuring> bac: hmm, that requires another tuple([]), or I replace the list comprehension
[19:55] <bac> adeuring: can't imagine that call to tuple is expensive.
[19:55] <adeuring> bac: well, OK, I'll do that
[19:56] <bac> adeuring: abel at line 74 can you compare against UDEV_ROOT_PATH instead of the string?
[19:56] <adeuring> bac: sure
[20:00] <bac> adeuring: at line 77 I was unsure what the value would be if both were None.  I now see that 'None or None' is None.  (conceivably it could be False.)  anyway since that construct is confusing perhaps you can write it in a more readable manner.
[20:01] <adeuring> bac: sure
[20:03] <adeuring> bac: diff so far: http://pastebin.ubuntu.com/292561/
[20:04] <bac> adeuring: I'd like to make a suggestion to see what you think.
[20:04] <abentley> bac: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/bmp-inline/+merge/13299 ?
[20:04] <bac> abentley: sure.  please add me to the MP
[20:05] <abentley> bac: I don't want to add you the the mp, as you are a member of launchpad code reviewers, which is already listed.  Could you please claim the review instead?
[20:05] <bac> adeuring: a lot of your code is based on selecting between vendor and product.  if your methods were returning dictionaries with those as the keys you could get rid of a lot of the conditionals and it would be much cleaner.
[20:05] <bac> abentley: oh, ok
[20:05] <achuni> bac: thanks :)
[20:06] <adeuring> bac: nice idea!
[20:06] <bac> abentley: except i don't see a "Claim" button any more!
[20:07] <bac> adeuring: ok, i'll make that broad suggestion in the review and let you figure out the details
[20:07] <adeuring> bac: sure, np
[20:07] <abentley> bac: Me neither, so I've reassigned it to you instead.
[20:07] <bac> abentley: thanks
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel || queue: [achuni, abentley] || This channel is logged: http://irclogs.ubuntu.com
[20:12] <abentley> rockstar: I can has UI review of https://code.edge.launchpad.net/~abentley/launchpad/bmp-inline/+merge/13299 ?
[20:13] <achuni> bac: If you've still got time left after that, I've just proposed a merge for a matching canonical-identity-provider branch, ui approved already.
[20:13] <bac> achuni: i'm here all afternoon!
[20:13] <achuni> oh good, because it's a biggie :P
[20:14] <achuni> bac: https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip/+merge/13309
[20:14] <bac> achuni: how big?
[20:15] <bac> achuni: please subscribe me (bac) to that branch as i cannot see the MP
[20:15] <achuni> on it
[20:16] <achuni> bac: done
[20:18] <bac> achuni: is that MP accurate? 3200+ lines?  on launchpad we have a limit of 800 lines -- after that we strongly consider doing multiple branches.
[20:19] <achuni> bac: I'm afraid it is... we weren't aware of the limit
[20:20] <achuni> bac: I guess I'll split this in to a few branches and come back
[20:20] <bac> achuni: ok, i'll do the review if i have time but will favor any smaller branches that come in.  we've agreed that's the way the on-call reviews should work to ensure the highest throughput.
[20:22] <achuni> thanks bac.  understood, and I promise future merges will be more reasonable
[20:22] <beuno> bac, it's mostly because it's a multi-person branch
[20:22] <beuno> and I've been holding it back because a lot of the UI stuff
[20:22] <beuno> I think there's like 4 or 5 different people who worked on it
[20:22] <beuno> so it's partially my fault  :)
[20:23] <bac> beuno: np.  it is what it is.
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [abentley, achuni et al] || This channel is logged: http://irclogs.ubuntu.com
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [achuni et al] || This channel is logged: http://irclogs.ubuntu.com
[20:39] <abentley> rockstar: I can has review of https://code.edge.launchpad.net/~abentley/launchpad/bmp-inline/+merge/13299
[20:42] <bac> hi abentley, this MP from achuni has no diff generated:  https://code.edge.launchpad.net/~canonical-isd-hackers/launchpad/sso-new-branding/+merge/13304
[20:42] <bac> thoughts?
[20:46] <abentley> bac: That's because diff generation oopsed.
[20:46] <bac> abentley: ok.
[20:47] <abentley> bac: That's the oops you pointed out to me earlier.
[20:48] <bac> abentley: not me.
[20:48] <abentley> bac: Sorry, salgado reported it.
[20:49] <abentley> bac: https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1382MPCJ5
[20:56] <rockstar> abentley, looking now.
[21:00] <abentley> thumper: Chat?
[21:03] <bac> hi sinzui
[21:03] <sinzui> hi bac
[21:09] <adeuring> bac: reply sent
[21:11] <thumper> abentley: sure
[21:33] <bac> adeuring: s/rott/root
[21:35] <bac> adeuring: actually root_device_ids doesn't need to be a method.  it can just be a property, i.e. root_device_ids = {'vendor'...}
[21:39] <bac> abentley, rockstar, thumper: whoever moved the edit icon for MP status back to being in-line is my new hero.
[21:39] <rockstar> bac, that was me.
[21:40] <thumper> bac: I'll be making is javascript soon
[21:40] <bac> rockstar: thanks!
[21:40] <bac> thumper: and that'll be super sweet
[21:40] <bac> rockstar: oh, i'm in on your legal defense fund for any face punching you do.
[21:40] <bac> well, not any random punching, just the one you promised
[21:48] <rockstar> bac, :)
[21:49] <adeuring> bac: no, root_device_ids needs to access self.dmi, which is only defined once the constructor has been called, so we need a method
[21:49] <bac> adeuring: ok
[21:49] <adeuring> bac: type is fixed
=== bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== salgado is now known as salgado-afk
=== salgado is now known as salgado-afk
=== matsubara is now known as matsubara-afk
=== flacoste is now known as flacoste_afk