#launchpad-reviews 2009-10-12
* jtv changed the topic of #launchpad-reviews to: on call: â || reviewing: â || queue: [jtv]  || This channel is logged: http://irclogs.ubuntu.com
<stub> rockstar: Do you want to review https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/13203 since it is your bug?
* intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: â || queue: [jtv]  || This channel is logged: http://irclogs.ubuntu.com
* intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: adeuring || queue: [jtv]  || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> adeuring: why `device['E'].get('SUBSYSTEM')` and not `device['E']['SUBSYSTEM']` ?
<intellectronica> adeuring: or is it not a get from a dictionary-like object?
<adeuring> intellectronica: it is a dict. Let me check, if we can guaraty if SUBSYSTEM is always defined....
<intellectronica> right
<adeuring> intellectronica: no, SUBSYSTEM is _not_ always defined.
<adeuring> intellectronica: to reproduce: udevadm info --export-db
<intellectronica> ah ok, so get might return None
<adeuring> intellectronica: exactly
<intellectronica> I'd supply the extra parameter, just for readability
<adeuring> intellectronica: do you mean a sort of an explicit marker?
<intellectronica> adeuring: device['E'].get('SUBSYSTEM', None)
<adeuring> intellectronica: hrmm... I am not generally opposed to that, but IÃ'd gues that other reviewers might suggest to remove the second parameter ;)
<intellectronica> adeuring: the docstring of is_scsi_device can terminate on the first line, no?
<adeuring> intellectronica: right, changed
<intellectronica> adeuring: you think so? i'll leave it for you to decide, but i think explicit is better than implicit
<adeuring> intellectronica: as I said, I don't have a definite opinion on this. Something for the reveiwer meeting?
<intellectronica> sure, or we can even ask barry what he thinks
 * barry thinks it's all wrong!  oh wait...
<intellectronica> i don't feel strongly about this, it meant literally what i would do
<intellectronica> barry: abel writes dict.get('sumfin') and i like dict.get('sumfin', None)
<intellectronica> because i always forget what the default behaviour is
<barry> intellectronica: i don't think you need that.  every python developer knows .get() returns None by default if the key is missing :)
<barry> intellectronica: that's deeply ingrained in python dna :)
<barry> however, if it's possible that 'sumfin' is in the dict with a value of None, then it's much better to do something like:
<barry> missing = object()
<barry> dict.get('sumfin', missing)
<adeuring> barry: that's not the case here
<barry> adeuring: cool.  in that case .get('foo') is fine
<intellectronica> adeuring: you see, i'm talking nonsense. clearly i'm not (genetically speaking) a python developer :)
<adeuring> intellectronica: ;) the point is that you can deevelop Python genes ;)
<barry> intellectronica: too much javascript will rot your brain
<intellectronica> adeuring: other than that docstring it all looks great. r=me
<adeuring> intellectronica: thanks!
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [jtv]  || This channel is logged: http://irclogs.ubuntu.com
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<nhandler> Does anyone know what file sets the 'Invalid value' error message for the subscription fields?
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> bigjools: would you like me to review first-ppa-name-bug-439305 ?
<bigjools> intellectronica: ah yes please, forgot to ask about that!  I have a couple possible changes though, perhaps I can discuss them with you after you've looked at it
<intellectronica> bigjools: sure
<intellectronica> bigjools: your branch looks fine to me. what changes did you want to make?
<bigjools> first, I was considering filling the "name" field with "ppa" if it's the first PPA
<bigjools> so it's a sensible default
<bigjools> second, I might change the interface description to make it more obvious that the name will be part of the repo URL
<bigjools> and also possibly change the displayname description so the user knows it will be part of the GPG key descriptiojn
<intellectronica> bigjools: yes, all three changes sound like very good ideas
<bigjools> intellectronica: okay, thanks, do you want to see another diff with that in?
<intellectronica> bigjools: ye
<intellectronica> yes
<bigjools> coming right up
<bigjools> intellectronica: what's the right way of pre-filling a form field?
<intellectronica> bigjools: in the view, default_values is a dict with an entry for every field you want to pre-fill
<bigjools> cool, ta
<bigjools> intellectronica: hmm is it possible to conditionally change the field descriptions on the form?  part three above only applies to the first created PPA
<intellectronica> bigjools: yes, but it's hackish. you need to create a new field, and then create a new fields object with the original field omitted and the new one inserted instead of it
<bigjools> urg
<bigjools> IIRC there is another way to add more text on the form?
<intellectronica> bigjools: nothing i can think of
<noodles775> intellectronica: I'm guessing you'll be finishing soon too, but if I'm wrong, there'll be an MP of mine arriving in a minute if you've time.
<intellectronica> maybe ask one of the resident zope mavens?
<intellectronica> noodles775: i can fit one more review
<noodles775> intellectronica: thanks! Note - I won't be around, so feel free to re-prioritise :)
* noodles775 changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> noodles775: np
 * bigjools looks for a maven
<bigjools> I think they all have a holiday
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: noodles || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> bigjools: i think gary_poster is around
<bigjools> intellectronica: default_values didn't work
<intellectronica> maybe it's called something different? let me find an example for you
<bigjools> thanks
<intellectronica> bigjools: sorry, i've misled you. it's actually called initial_values. see lib/lp/bugs/browser/bugtarget.py for example
<bigjools> intellectronica: got it, thanks
<gary_poster> bigjools: not sure what I'm a maven of, but I am around.  I am kind of trying to sneak lunch in though.  But if you ask me something or other I'll try to get back quickly
<bigjools> gary_poster: real quick one!  is there a way to conditionally get some extra text on a form, or preferably change the value of a form field's description 
<intellectronica> gary_poster: i suggested creating a new field and and a new fields collection with the new field replacing the old one, but we didn't know if there's an easier way to do it
<gary_poster> bigjools, intellectronica: what intellectronica said sounds like the right basic idea.  it's possible we are already creating a one-off field collection for the request, in which case you can just change it in place, which would make it shorter, at least.
<gary_poster> but that would just be an optimization of the same basic idea
<bigjools> gary_poster: is there a property on the field I can poke then?
 * bigjools boggles at the number of requests in .dev for a simple page
<bigjools> intellectronica: is there a trick to getting the initial_values property called?  Mine isn't :/
<bigjools> oh fuck
<bigjools> I need to spell it right of course
<intellectronica> noodles775: r=me. it would be great to convert that sqlo query to storm, but that's beyond the scope of the branch. everything else looks great
<bigjools> heh the BUDQ
<intellectronica> ?
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bigjools> Big Ugly Dispatcher Query
<bigjools> intellectronica: http://pastebin.ubuntu.com/291713/
<intellectronica> bigjools: i suspect that the way you've written initial_values, the value will be overridden even if filled by the user before submissions (say if there's an error in another field). is that the behaviour you want?
<bigjools> intellectronica: I don't understand
<intellectronica> bigjools: if you submit the form and make an error, the form is displayed again with the values you've used before. if you always override initial_values, the previously entered value won't be retained (which is the default behvaiour), because you're looking at the value on the context, which hasn't changed
<bigjools> ah i see
<intellectronica> bigjools: i suspect that's what wil happen. i'm not 100% sure, but i haven't tried
<bigjools> I'll check it out and see what happens
<intellectronica> cool
<intellectronica> everything else looks fine. i take you've decided that manipulating the field's description dynamically is to much of a hassle?
<bigjools> yes, the return on investment is small
<intellectronica> i agree
<bigjools> intellectronica: actually the value is retained, despite initial_values
<bigjools> so it's all good!
<intellectronica> bigjools: excellent. r=me then
<bigjools> intellectronica: great, thanks for your advice and review
<intellectronica> and with that i shall end my review shift. i think i've been more trouble than help as a reviewer today :-/
<bigjools> hehe
* intellectronica changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bigjools> fud time here
<gary_poster> bigjools-afk: if you can get to a widget then you are home free: set the .label or the .hint, depending on what you want.
<gary_poster> if that doesn't do what you want, back to the drawing board.  You can do a custom widget easily-ish.
<leonardr> gary, can you review this tiny lazr.restful diff so i can make a new release for launchpad?
<leonardr> https://pastebin.canonical.com/23226/
<gary_poster> yes
<leonardr> i've also got a small launchpad branch coming up. (my ec2 test just passed)
<gary_poster> leonardr: r=gary
<gary_poster> great
<leonardr> can i commit my changes to the download cache?
<gary_poster> leonardr: yes
<leonardr> all right
<leonardr> i love it when a plan comes together
<gary_poster> :-)
<leonardr> gary: fyi, here's what i'm adding
<leonardr> added:
<leonardr>   dist/grokcore.component-1.6.tar.gz
<leonardr>   dist/lazr.restful-0.9.11.tar.gz
<leonardr>   dist/martian-0.11.tar.gz
<leonardr>   dist/wsgiref-0.1.2.zip
<gary_poster> leonardr: cool
<leonardr> gary: https://code.edge.launchpad.net/~leonardr/launchpad/help-me-gary/+merge/13245
<gary_poster> heh
<leonardr> that's what it was called when i couldn't figure out what those tarballs were for
<gary_poster> heh, I see
<gary_poster> leonardr: r=gary
<leonardr> all right
 * leonardr has not pqm-submitted a branch for months. does it still work the same way?
<gary_poster> it can still work with bzr pqm-submit.  There are other new tricks, but that should be fine.
<leonardr> all right
<leonardr> i'm off, i'll see you tomorrow
<gary_poster> see you
<gary_poster> thanks 
#launchpad-reviews 2009-10-13
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bac> hi gmb.  can i put a review on your queue?
<bac> noodles775: can you do a UI review for me?
<noodles775> bac: sure
<bac> noodles775: thanks, i just added you to https://code.edge.launchpad.net/~bac/launchpad/bug-436980-mugshots/+merge/13250
<gmb> bac: Sure.
<bac> gmb: cool. it's the one i just posted
<gmb> Righto.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<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
<bac> thanks gmb
<noodles775> bac: r=me (one suggestion on the MP).
<noodles775> Looks great!
<bac> thanks noodles775. do i have to have beuno give it a look too?
<noodles775> bac: if he's around, it'd be great (for my learning). beuno ?
<matsubara> !
<matsubara> oops
<beuno> hi bac, noodles775 
<noodles775> hi beuno :)
<noodles775> beuno: I'm just heading for lunch, but it'd be great if you could any comments you have to the MP, thanks!
<beuno> noodles775, bug-436980-mugshots?
<noodles775> beuno: yep
<beuno> noodles775, on it
<beuno> bac, noodles775, I'm suprised how many things I managed to squeeze out of that page, but, anyway, reviewed
<bac> beuno: squeeze?
<beuno> bac, long review for such a small and fairly unused page
<bac> beuno: it already uses 32
<bac> beuno: so much for not reading the merge proposal!  :)
<beuno> bac, as you can see, I follow my guidelines to the letter
<beuno> ah, 8 en dev
<beuno> :)
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bac> hi noodles775
<noodles775> Hi bac
<bac> noodles775: thanks for the CSS suggestion.  much nicer.
<noodles775> bac: np! glad it was useful.
<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.
<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
<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
 * gmb -> afk for a while
<mrevell> bac hi, on a call atm
<bac> see ya gmb
<bac> mrevell: ok, i'm looking at your editpgp review.  give me a shout when you have a sec.
<abentley> bac: Could you please review https://code.launchpad.net/~abentley/launchpad/reuse-refactor/+merge/13283 ?
<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
<bac> abentley: you aren't using the refactored code anywhere yet, right?  so the only demo is bug comments?
<abentley> bac: Right.
<abentley> bac: I'm just polishing up the branch that uses it for merge proposals.
<bac> abentley: ok
<bac> hi rockstar
<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.
<abentley> bac: Yes, they now do.
<bac> abentley: that seems strange to me.  you lose the historical context for the review.
<abentley> I resisted it at first, too, but users demanded it.
<abentley> bac: We will be linking reviews to the particular diff they relate to.
<bac> abentley: an incremental added to the MP would be fantastic.
<bac> abentley: ok.  glad to know my eyes weren't deceiving me.
<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.
<rockstar> bac, hi.
<bac> hi rockstar -- i was going to ask you about MP diffs but had the chat with abentley.
<rockstar> bac, great.  I didn't realize that my IRC client wasn't connected until just now.
<rockstar> (no wonder I was being so productive)
<mrevell> hi bac, off my call now
<bac> abentley: r=bac.  wonderful change.
<bac> hi mrevell. i had some questions about your editgpg branch
<mrevell> bac: Ah, great, I hope I can answer them
<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.
<mrevell> bac: I'll fix that (almost certainly is fault). Thanks.
<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.
<abentley> bac: Thanks!
<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
<mrevell> bac: Hmm, yes, good suggestion. I'll do that.
<bac> mrevell: ok, great.  i'll put that in your MP and approve it now.  thanks for the branch.
<abentley> bac: What would you change the inline comment to?
<mrevell> thank you bac
<bac> abentley: just move it up to precede the line.
<abentley> bac: Will do.
<bac> abentley: i like those little end-of-the-line comments myself...
<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
<bac> mrevell: right now it suggests submitting directly to our keyserver, but even that has a delay
<mrevell> Ah, right. thanks bac, I'll take a look at that.
<bac> mrevell: it's in c/l/browser/logintoken.py
<mrevell> thanks bac
<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
<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
<bac> bigjools: your review is done
<bac> salgado: i will if you ask over here!
<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
<bac> adeuring: i will.  add me to the MP?
<adeuring> bac: thanks, done
<achuni> hi, is it normal to get mails about Launchpad Internal Error when notifying people about proposals to merge?
<achuni> (it was a proposal to merge in to Launchpad itself, which is why I ask here)
<salgado> achuni, I just got one.  can you give me the OOPS ID of yours?
<achuni> salgado: OOPS-1382MPCJ5
<salgado> abentley, around?  I just got https://lp-oops.canonical.com/oops.py/?oopsid=1382CMP1 when submitting a merge proposal
<salgado> and achuni is a completely different one, it seems
<achuni> anybody up for reviewing https://code.launchpad.net/~canonical-isd-hackers/launchpad/sso-new-branding/+merge/13304 ?
<bigjools> thanks bac!
<abentley> salgado: That is bug #436325
<mup> Bug #436325: Diffstat generation chokes on binaries (and others) <oops> <Launchpad Bazaar Integration:Triaged by abentley> <https://launchpad.net/bugs/436325>
<salgado> abentley, and did you see achuni's OOPS? OOPS-1382MPCJ5
<bac> achuni: i'll review it in a bit
<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
<salgado> achuni, ^
<achuni> abentley: should I report?
<achuni> abentley: thanks for looking in to it
<abentley> achuni: up to to you.
<bac> adeuring: for pci_ids the docstring says you return a tuple but you return a list
<adeuring> bac: argh... fiexd.
<bac> adeuring: i think returning the tuple would be best
<adeuring> bac: hmm, that requires another tuple([]), or I replace the list comprehension
<bac> adeuring: can't imagine that call to tuple is expensive.
<adeuring> bac: well, OK, I'll do that
<bac> adeuring: abel at line 74 can you compare against UDEV_ROOT_PATH instead of the string?
<adeuring> bac: sure
<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.
<adeuring> bac: sure
<adeuring> bac: diff so far: http://pastebin.ubuntu.com/292561/
<bac> adeuring: I'd like to make a suggestion to see what you think.
<abentley> bac: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/bmp-inline/+merge/13299 ?
<bac> abentley: sure.  please add me to the MP
<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?
<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.
<bac> abentley: oh, ok
<achuni> bac: thanks :)
<adeuring> bac: nice idea!
<bac> abentley: except i don't see a "Claim" button any more!
<bac> adeuring: ok, i'll make that broad suggestion in the review and let you figure out the details
<adeuring> bac: sure, np
<abentley> bac: Me neither, so I've reassigned it to you instead.
<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
<abentley> rockstar: I can has UI review of https://code.edge.launchpad.net/~abentley/launchpad/bmp-inline/+merge/13299 ?
<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.
<bac> achuni: i'm here all afternoon!
<achuni> oh good, because it's a biggie :P
<achuni> bac: https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip/+merge/13309
<bac> achuni: how big?
<bac> achuni: please subscribe me (bac) to that branch as i cannot see the MP
<achuni> on it
<achuni> bac: done
<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.
<achuni> bac: I'm afraid it is... we weren't aware of the limit
<achuni> bac: I guess I'll split this in to a few branches and come back
<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.
<achuni> thanks bac.  understood, and I promise future merges will be more reasonable
<beuno> bac, it's mostly because it's a multi-person branch
<beuno> and I've been holding it back because a lot of the UI stuff
<beuno> I think there's like 4 or 5 different people who worked on it
<beuno> so it's partially my fault  :)
<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
<abentley> rockstar: I can has review of https://code.edge.launchpad.net/~abentley/launchpad/bmp-inline/+merge/13299
<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
<bac> thoughts?
<abentley> bac: That's because diff generation oopsed.
<bac> abentley: ok.
<abentley> bac: That's the oops you pointed out to me earlier.
<bac> abentley: not me.
<abentley> bac: Sorry, salgado reported it.
<abentley> bac: https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1382MPCJ5
<rockstar> abentley, looking now.
<abentley> thumper: Chat?
<bac> hi sinzui
<sinzui> hi bac
<adeuring> bac: reply sent
<thumper> abentley: sure
<bac> adeuring: s/rott/root
<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'...}
<bac> abentley, rockstar, thumper: whoever moved the edit icon for MP status back to being in-line is my new hero.
<rockstar> bac, that was me.
<thumper> bac: I'll be making is javascript soon
<bac> rockstar: thanks!
<bac> thumper: and that'll be super sweet
<bac> rockstar: oh, i'm in on your legal defense fund for any face punching you do.
<bac> well, not any random punching, just the one you promised
<rockstar> bac, :)
<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
<bac> adeuring: ok
<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
#launchpad-reviews 2009-10-14
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/script-error-reporting-utility/+merge/13329 perchance?
<mwhudson> thumper: why not run all diff tests in the zopeless layer>
<mwhudson> ?
<mwhudson> i guess some of the code runs in the webapp
<thumper> umm...
<thumper> possibly?
<thumper> I could do  I gess
 * thumper smacks forehead
<mwhudson> thumper: did you know https://code.edge.launchpad.net/launchpad-project/+activereviews is oopsing? 
<thumper> no
<mwhudson> TypeError: can't compare datetime.datetime to NoneType<br /> 
<thumper> fuck
<mwhudson> something to do with the sorting, i think
<thumper> ya think?
<thumper> and no losa :(
<thumper> was hoping for a db query
<mwhudson> thumper: pjdc can do them
<thumper> who is pjdc?
<thumper> mwhudson: do you think it better to run all the tests in the zopeless layer?
<thumper> all the diff ones anyway
<mwhudson> thumper: wellington based sysadmin
<thumper> ah, I forgot about him
 * mwhudson looks at the diff testsx
<mwhudson> thumper: i guess it makes sense to divide up the tests depending on whether they'll be running in a script or the webapp
<mwhudson> thumper: (and in the medium term, let's kill more and more of the zopeless/webapp description)
<mwhudson> distinction!
<mwhudson> not description
 * mwhudson says stuff on the mp
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Hi allenap, got time for this one: https://code.launchpad.net/~michael.nelson/launchpad/450124-findBuildCandidate_improvements/+merge/13334
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: Yeah, sure.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: noodles || queue: [] || This channel is logged: http://irclogs.ubuntu.com
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Thanks allenap 
<allenap> noodles775: You're welcome.
<allenap> noodles775: Do you know where we use YUI 2.7?
<noodles775> allenap: yep - the date/time picker :/
<noodles775> allenap: see the inclusion in templates/sprint-new.pt for example.
<allenap> noodles775: Thanks. By the way, it's in reference to https://code.edge.launchpad.net/~sinzui/launchpad/canonical-fonts-bug-435356/+merge/13318 where sinzui has updated the font-family definitions in the YUI 2.7 code. I wondered if it was necessary.
<noodles775> allenap: right. fwiw, afaik we're only using yui2.7 for the calendar, so updating those two css files would be necessary, not sure about the others.
<allenap> noodles775: I'll add that to the review. Thanks.
<noodles775> np.
<achuni> hi, I'm looking for a reviewer for https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip/+merge/13309
<achuni> it's a huge monster of a diff
<achuni> bac started on it yesterday but didn't get round to finish it
<achuni> I was wondering if some kind soul could look at it, or if we'll need to set out and split it up
<allenap> achuni: I'll take a look.
<allenap> achuni: Ah, I don't have permission to see that.
<achuni> allenap: I'll subscribe you in a sec
<achuni> allenap: subscribed
<allenap> achuni: Thanks.
<allenap> achuni: I think it would be better if you could break it up into three changes, two at least. There's a lot of new stuff there, and it's not to a codebase that I'm intimately familiar with. On another day I might feel brave and do it, but my history on mammoth reviews is that the time spent shoots up as the diff gets bigger, and the quality goes down.
<allenap> achuni: Do you think you'll be able to break it up?
<achuni> allenap: I'll do my best
<allenap> achuni: Cool, and sorry for now :-/
<achuni> allenap: np, it's all for the best
<sinzui> allenap: Thanks for the review. noodles775: I updated all the YUI css because mt reopened the bug for U1 because the JS widgets were not in the official style. I think it is safer to convert them all in the off chance we use another 2.0 widget.
<allenap> sinzui: You're welcome, and that sounds reasonable.
<noodles775> allenap: I don't seem to be getting an email notification - so maybe you didn't either, but jfyi, I've added an incremental after your review to https://code.edge.launchpad.net/~michael.nelson/launchpad/450124-findBuildCandidate_improvements/+merge/13334
<allenap> noodles775: I'll have a look. I haven't received an email either.
<allenap> noodles775: Still r=me :)
<noodles775> allenap: thanks.
<allenap> noodles775: Thanks for replying; not everyone does if I have some comments but +1 anyway.
<noodles775> allenap: np - I always like to know how people finish up branches too - after having poured over the code :)
<adeuring> allenap: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-5/+merge/13350 ?
<allenap> adeuring: Sure.
<adeuring> allenap: thanks!
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<al-maisan> jml: it would be nice if we could make some headway wrt bug #347768
<mup> Bug #347768: Allow anyone with upload rights to write to a package branch <package-branches> <Launchpad Bazaar Integration:In Progress by jml> <https://launchpad.net/bugs/347768>
<jml> al-maisan, yes, I very strongly agree.
<al-maisan> would you have time for a call?
<jml> al-maisan, yes I would. Can you give me 5-10 minutes to gather my thoughts?
<al-maisan> jml: most certainly!
<allenap> adeuring: The diff is rather long; I assume it's meant to be against devel rather than db-devel?
<adeuring> allenap: argh, of course... Let me paste te real diff
<allenap> adeuring: It's okay, I have it.
<adeuring> allenap: ok, thanks
<jml> al-maisan, ok, let's do it.
<al-maisan> jml: yup
<jml> al-maisan, nothing is happening.
<al-maisan> jml: I tried calling but got a "call refused"
<allenap> adeuring: All looks good.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> allenap: thanks!
* allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<jml> al-maisan, http://pastebin.ubuntu.com/293241/
<adeuring> allenap: could you add your review to my MP?
<allenap> adeuring: I sent an email review...
<adeuring> allenap: Ah, OK. Seems that processing needs some time...
<allenap> adeuring: Mmm, I sent it nearly 20 minutes ago.
<leonardr> edwin, i'd like your views on https://code.edge.launchpad.net/~leonardr/lazr.restful/optional-compression/+merge/13358
<rockstar> EdwinGrubbs, may I stick a branch in your queue.  It's seriously a one line change.
<EdwinGrubbs> rockstar: sure
<achuni> allenap: I've split the merge out in to three MP:
<achuni> https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip1/+merge/13351
<achuni> https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip2/+merge/13362
<achuni> https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip/+merge/13363
<achuni> I'm afraid the second one is still quite large
<leonardr> EdwinGrubbs, did you see my review request?
<EdwinGrubbs> leonardr: no
<leonardr> edwingrubbs: https://code.edge.launchpad.net/~leonardr/lazr.restful/optional-compression/+merge/13358
<EdwinGrubbs> rockstar: where is your MP?
<rockstar> EdwinGrubbs, still prepping it.  leonardr can go first.
<EdwinGrubbs> leonardr: reviewing it now
<leonardr> great
<bac> hi edwin -- can i add a super simple one to your queue?
<EdwinGrubbs> bac: sure
* bac changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com
<bac> EdwinGrubbs: https://code.edge.launchpad.net/~bac/launchpad/bug-436978-pg-index/+merge/13368
<bac> beuno: UI review?  https://code.edge.launchpad.net/~bac/launchpad/bug-436978-pg-index/+merge/13368
<beuno> bac, sure
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: leonardr || queue: [rockstar, bac] || This channel is logged: http://irclogs.ubuntu.com
<bac> beuno: scroll down for screenshots
<beuno> bac, reviewed
<bac> beuno: fast!
<bac> thanks
<EdwinGrubbs> leonardr: Is BaseWSGIWebServiceConfiguration supposed to be used somewhere, or is it just a resource for other applications?
<leonardr> edwingrubbs: it's just for other applications, but it is used in the wsgi example service
<EdwinGrubbs> leonardr: I see that it sets set_hop_to_hop_headers to False, but that variable defaults to true in the tests. Is it not using that base in class in that test?
<leonardr> edwin: right. all the tests except the ones in wsgi/example/tests use the default behavior
<leonardr> the tests in wsgi/example/tests will have it set to false, but as i mentioned in the merge proposal, it doesn't seem to make a difference, because my test wsgi server is less strict than real wsgi servers
<leonardr> i can add a test that shows that the TE header is ignored in wsgi services
<EdwinGrubbs> leonardr: I don't think that's necessary. I was just confused.
<leonardr> ok
<rockstar> EdwinGrubbs, https://code.edge.launchpad.net/~rockstar/launchpad/fix-ensure-login/+merge/13373
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing:- || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com
<EdwinGrubbs> rockstar: it's a good thing you told me it was one line, since I almost went to lunch. r=me
<rockstar> Edwin-lunch, thanks!
<nhandler> Does anyone know where the error message 'Invalid value' that you get when you try to subscribe a non-existant user to a bug is defined?
<mwhudson> nhandler: it's something terrible deep in the zope form machinery i think
<mwhudson> nhandler: do you know what a 'vocabulary
<mwhudson> ' is in the context of launchpad?
<nhandler> mwhudson: Not really. I'm still learning as I go. I was working on bug #231168, and in order to properly fix it, the error message should really be modified
<mup> Bug #231168: Subscribe someone else says: "...if they have an active account." <trivial> <ui> <Launchpad Blueprints:In Progress by nhandler> <https://launchpad.net/bugs/231168>
<mwhudson> nhandler: ok
<mwhudson> nhandler: basically, i can't remember
<mwhudson> nhandler: but i think i knew once, so i can probably find out quicker than you :)
<nhandler> :)
<nhandler> Thanks a lot mwhudson 
<mwhudson> nhandler: can you tell me where the view class is for this form?
<nhandler> mwhudson: The view class? My python abilities are very minimal I'm afraid.
<mwhudson> nhandler: it's ok, i think i found it
<mwhudson> nhandler: i *think* you need to specify a validator for the "person" Field in IBugSubscription
<nhandler> mwhudson: Thanks. Now I have somewhere to work from. My goal is to get this bug fixed and ready to be reviewed/merged by the end of the week
#launchpad-reviews 2009-10-15
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing:- || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<noodles775> bigjools: just looking at the comment that you added to DistributionPPASearchView - "There really should be a way for the form to reject unexpected...." - isn't the issue there really that we're not using an LPForm?
<noodles775> (ie. we'er using request.get - which is accessing the data directly from the GET dict?)
<bigjools> noodles775: what else could it do?
<noodles775> bigjools: if the view was inheriting from LaunchpadFormView we could be using request.form?
<noodles775> ... at least - I think - checking now.
<noodles775> OK, so if we used a safe_action in an LPFormView the validation would take care of this automatically (as it validates against the form schema).
<noodles775> bigjools: r=me (just a question about the comment).
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: EdwinGrubbs || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bigjools> noodles775: umm I have no answer to that :)
<noodles775> bigjools: no answer (as to whether we should use LPFormView)?
<bigjools> it seemed like a follow-on from the part above so it made sense at the time to run on the comment
<noodles775> Ah, I'd forgotten about that question :), ok.
<bigjools> :)
<adeuring> noodles775: could you review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-6/+merge/13402 ?
<noodles775> adeuring: sure!
<adeuring> noodles775: thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: EdwinGrubbs || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com
<bigjools> noodles775: I have changed the comment in that code
<noodles775> bigjools: k.
<bigjools> I removed the XXX and explained that it's better to be helpful and join the terms
<noodles775> bigjools: ah - great.
<bigjools> http://pastebin.ubuntu.com/293807/ if you're interested
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<noodles775> adeuring: r=me - sent with a few comments :)
 * noodles775 -> lunch
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> noodles775: thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<salgado> noodles775, no need to review that branch of mine.  barry's on it
<noodles775> salgado: ah ok - I was just about to start :) Thanks.
<noodles775> aw, and it's so small :)
<barry> ocr's: you don't have to review any merge into lp:~launchpad-dev/launchpad/python-migration.  we're sprinting on that in #launchpad-sprint
<noodles775> k, thanks barry.
<adeuring> noodles775: fancy to review a sequel of my last branch?
<noodles775> adeuring: sure!
<adeuring> noodles775: thanks! https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-7/+merge/13413
<noodles775> adeuring: yikes, 1070 lines - I'm hoping that includes the -6 branch?
<noodles775> *phew* looks like it.
<adeuring> noodles775: sigh, I screwed up the "bzr send" command. Should 273 lines...
<noodles775> aha, np.
<adeuring> noodles775: clould you use the diff between revisions 9694 and 9695?
<noodles775> adeuring: yep, np.
<adeuring> noodles775: (the branch is based on the one you reviewed earlier. Has not yet landed)
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<noodles775> adeuring: r=me, there was one change that I was unsure of (see the comment). Thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> noodles775: thanks!
<noodles775> np
<adeuring> rockstar: fancy a review of a branch with 20 line diff?
<rockstar> adeuring, oh yeah, it's thursday, I forgot...
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> rockstar: no problem ;) https://code.edge.launchpad.net/~adeuring/launchpad/bug-446600/+merge/13430
<rockstar> adeuring, shoot it on over.
<rockstar> adeuring, r=me
<adeuring> rockstar: thanks!
<gary_poster> rockstar: you up for a 46-line lazr.restful diff?  https://code.edge.launchpad.net/~gary/lazr.restful/fix-wadl-root-template/+merge/13429
<rockstar> gary_poster, sure.
<gary_poster> thanks
<rockstar> gary_poster, r=me, come back when you have a real diff.  :)
<gary_poster> rockstar lol thanks :-)
<gary_poster> salgado: ping?
* rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
#launchpad-reviews 2009-10-16
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<allenap> jml: Are you still part of the OCR rotation? In any case, https://dev.launchpad.net/ReviewerSchedule still has you down for AsicPac.
<jml> allenap, I'm taking a temporary leave of absence
<jml> allenap, I'll reschedule myself sometime soon.
<danilos> adeuring: hi, I've got a branch up for review, can you take it? :)
* danilos changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> danilos: sure
<danilos> adeuring: thanks, it's https://code.edge.launchpad.net/~danilo/launchpad/bug-435398/+merge/13462 (I've hit a bug submitting via email, so diff should be there in a minute or two if not already up)
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: danilo || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring>  danilos: nice work! just one question about a change you did not describe: In line 205 of the diff you remove "/+translations" from a link. Is this intentional, and if so, why?
<danilos> adeuring: yes, it doesn't make much difference in practice, because that's the default anyway, but would allow us to change the default easier in the future without worrying of breaking unrelated tests (such as this one)
<adeuring> danilos: ah, thanks. So, r=me
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Hi adeuring! I've got a straight-forward view/template change that should arrive in your inbox in a minute or two.
<adeuring> noodles775: ok
<noodles775> adeuring: how are you going with those hwdb changes? Looks like a huge change (great to see lots of small branches!)
<noodles775> it's been going for a couple of weeks now right?
<adeuring> noodles775: I noticed that I made some embarassingly wrong assumptions in the test data, so I'm updating tests again ;)
<adeuring> noodles775: yes, I'm working on this tsuff since two weeks or so. I hope that the changes will be finished next week
<noodles775> adeuring: is there anyone else who's familiar with all the udev stuff that could review those type of things for you?
<adeuring> noodles775: i don't think so...
<noodles775> s/that/who
<noodles775> ok.
<danilos> adeuring: thanks
<noodles775> adeuring: here it is: https://code.launchpad.net/~michael.nelson/launchpad/430459-link-build-failures-updates-portlet/+merge/13463
<noodles775> thanks!
<noodles775> intellectronica: would you have time for a UI review? It's "quite straight forward". See ^^^ before you answer :)
<intellectronica> noodles775: sure
<noodles775> Thanks!
<intellectronica> noodles775: it looks really nice. the only problem i have with it is that the error icon looks just like the remove icon. i guess that's out of scope for this branch, though?
<intellectronica> is this icon new, or something that is all over the place in soyuz?
<noodles775> intellectronica: do you mean the failed to build icon?
<intellectronica> noodles775: yes
<noodles775> intellectronica: yes, according to https://edge.launchpad.net/+graphics it's specifically for build-failed...
<noodles775> so we use it wherever we indicate failed builds. (part of the new icons that landed during 3.0)
<intellectronica> noodles775: well, it's worth asking beuno what he thinks of that critique of the icon. i know he's been focusing on icon semantics quite a lot lately. other than that the change looks really good, so ui=me*
<noodles775> intellectronica: thanks. It's one of the set that beuno introduced a few months ago, but yep, it'd be good to let him know of that confusion.
<adeuring> noodles775: r=me
<noodles775> thanks adeuring !
<noodles775> hey jml: just noticed that you're a ui* reviewer? Would you have 5mins to do a second ui* review for me (it already has ui*=intellectronical, but most of the other ui reviewers are sprinting/not here yet).
<noodles775> https://code.launchpad.net/~michael.nelson/launchpad/430459-link-build-failures-updates-portlet/+merge/13463
<noodles775> s/tronical/tronica ;)
<jml> noodles775, sure thing. fwiw, I don't get it done in the next 30 mins, I won't be able to get around to it today.
<jml> _if_ I don't get it done, rather.
<noodles775> jml: thanks. np, if you're too busy - just leave it - I can grab one of the US guys later.
<jml> noodles775, well, I'd like to at least _try_ to do it :)
<jml> noodles775, sorry, I've failed.
<noodles775> jml: no probs, thanks for trying :)
<noodles775> rockstar: are you able to do a second ui review for me?
<noodles775> https://code.launchpad.net/~michael.nelson/launchpad/430459-link-build-failures-updates-portlet/+merge/13463
<barry> adeuring: i'm not sure how much reviewing salgado and i are going to do today.  we're sprinting.  if it's okay with you i'd like to avoid non-sprint related reviews at least until you go offline
<barry> adeuring: but ping me if the queue gets too long
<adeuring> barry: OK, thnkas for the heads-up. But up to now it was a really quit day
<noodles775> adeuring: could you look at another trivial branch for me?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/219222-edit-ppa-dependencies-rename/+merge/13468
<adeuring> noodles775: sure
<noodles775> intellectronica: would you mind doing a ui review for the above too? (sorry, no one else is around)
<noodles775> adeuring: thanks.
<noodles775> s/around/around or available due to sprints etc.
<bac> adeuring: may i add a small branch to your queue?
<adeuring> bac:  sure
<bac> adeuring: great.  i'll paste the MP when it arrives
* bac changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: - || queue: [noodles,bac] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> noodles775: r=me
<noodles775> Thanks again adeuring.
<adeuring> welcome :)
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: bac || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> bac: bug 436986 says that the map is already limited to 24 items. You set self.limit = 24 for class TeamMapLtdData. Couldn't you recycle the constant mentioned in the bug?
<mup> Bug #436986: Person:+index (map_portlet_html) timeout <post-3-ui-cleanup> <timeout> <Launchpad Registry:In Progress by bac> <https://launchpad.net/bugs/436986>
<bac> adeuring: the limit mentioned in the bug was being realized at line 62 of the diff, but it did not work
<bac> adeuring: i should have a constant so i don't re-use 24 at lines 84 and 99
<adeuring> bac: Ah, thanks, I missed that line in the diff. And right, please use a constant for both classes.
<bac> gladly
<adeuring> bac: one more question: your tests do not really show that the map is indeed limited to 24 items. Could you change the test in team-map.txt so that we really see that no more than TeamMap.limit items are returned? (probably with a modified version of TeamMap, where limit is 2)
<adeuring> ...erm, I meant class TeamMapData....
<bac> adeuring: i should have worded that test more clearly.  i'd like to use it to show the URL works but not necessarily the limit.  the limit is exercised in the team-views.txt test. with additional explanation in the first do you think that'd be sufficient?
<adeuring> bac: well, you show the constant there, but you don't show that the constant has any effect ;)
<bac> which there?
<bac> in team-views.txt the constant is there.  it is then set to 1 to show it does have affect?
<adeuring> bac: Ahhh, I missed that.
<adeuring> bac: thanksfor the explanantions. r=me
<bac> adeuring: np.  thanks for the review!
<bac> with non-batched email from LP it is weird to see the email arrive before the web page refreshes when you update a MP
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> adeuring: barry: I have a mechanical branch for review: https://code.edge.launchpad.net/~sinzui/launchpad/package-link-validation/+merge/13484
<adeuring> sinzui: I'll look
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<barry> adeuring: thanks.  i'm doing a hairy one for gary on #launchpad-sprint (barry does hairy gary branch?  ewwee)
<gary_poster> heh
<adeuring> sinzui: r=me
<sinzui> adeuring: thank you very much
<adeuring> sinzui: welcome :)
<bac> adeuring: you have time for another or should i wait for barry?  it's complicated by the fact i'm about to leave for lunch.
<adeuring> bac: one more branch is fine, but after that I'll leavem I think ;)
<bac> hey barry are you going to be reviewing after lunch?
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bac> thanks abel.  i'll answer any questions you have when i return.
<adeuring> bac: r=me
<abentley> adeuring: Could you please review https://code.launchpad.net/~abentley/launchpad/ignore-md-diff/+merge/13491 ?
<adeuring> abentley: sure
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<abentley> adeuring: Thanks.  Btw, have you tried the inline reviewing?
<adeuring> abentley: no. How do I do this?
<abentley> There is a comment/review box below the comments.
<adeuring> abentley: ah, that's what you meant ;) I'll try it
<adeuring> abentley: r0me
<abentley> adeuring: Thanks!
* adeuring changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bac> thanks adeuring
<bac> hi beuno, can you do a UI review?  i've got screenshots!
<beuno> bac, hit me
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-452491-captcha2-boogaloo/+merge/13487
<bac> beuno: it's adding a captcha to the 'i forgot my password' page.  also fixes some spacing on the error message from the login page
<beuno> bac, why are the forms different than everywhere else?
<beuno> where the label is on top
<beuno> I know, I know, nothing to do with your branch
<bac> beuno: don't know.  these pages do not use LaunchpadFormView -- they do everything by hand.  a huge PITA
<beuno> bac, gotcha
<bac> beuno: converting them to LPFV would be a good tech-debt branch
<beuno> approved
#launchpad-reviews 2010-10-18
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/code-import-event-garbo/+merge/38675
<thumper> lifeless: this branch adds garbo for code import evenst
<thumper> lifeless: which I'd love to get cp'ed to loganberry asap
<mwhudson> drop table codeimportevent?
<mwhudson> it was kind of a nice idea, but never fully implemented and now it seems to be just a hindrance
<lifeless> thumper: it looks fine to me; no need to cp it in a hurry though - just put the rev that it lands in on LPS special deployment instructions
<thumper> mwhudson: useful really for machines, not for code imports themselves
<thumper> mwhudson: I've been thinking of refactoring the code import model code to not use it
<mwhudson> yeah, that's true
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> hi henninge
<henninge> Hi lifeless ! ;)
<lifeless> I have some parallel testing stuff that is moderately working
<lifeless> would you like to eyeball it ?
<henninge> lifeless: I can have a look ...
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/uniqueconfig/+merge/38689 and https://code.edge.launchpad.net/~lifeless/launchpad/databasefixture/+merge/38694
<lifeless> I'm not particularly worried about polish here - we've such a long trip to go, I'm primarily interested in *getting there* and then filling things out
<lifeless> otoh I don't want to land stuff that isn't understandable either
<henninge> lifeless: for starters, why are you adding stuff to lib/canonical when the doctring says it's launchpad specific? ("Create a unique launchpad config.")
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lifeless || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> henninge: because, for all intents and purposes, canonical.config *is* launchpad specific.
<lifeless> henninge: its a YAGNI to claim otherwise. lazr.config is generic, and its in a different tree
<lifeless> henninge: that said, my code is -so far at least- agnostic, so i can alter the docstring if you like
<henninge> lifeless: why not put it in lp.testing?
<lifeless> henninge: two reasons; its able to be used outside of test code, and its tightly coupled to the config concept, which lives in canonical.config
<lifeless> it would split a single concept across the code base to split them up
<henninge> lifeless: do you know what tha project is called that holds the production configs?
<lifeless> launchpad-production-configs
<lifeless> bzr+ssh://bazaar.launchpad.net/~launchpad-pqm/lp-production-configs/trunk/
<henninge> Cheers
<henninge> lifeless: where does "EnvironmentVariableFixture" come from? I cannot find it in the fixtures source code?
<henninge> ah, you are using 0.3.2
<bac> hi henninge
<lifeless> henninge: hey, so how are you going with those branches; I worry that I've sent you off into a tailspin or something :)
<henninge> lifeless: otp
<lifeless> jml: you can see in backlog two branches for parallel testing; henninge has had them for a few hours; I don't know what outcome will be; you might be interested regardless, or if henninge is too busy, to just review em ;)
<lifeless> gnight all
<jml> lifeless: sure :)
<henninge> lifeless: sorry
<bac> hi henninge, will you have time for another review?
<bac> henninge: unfortunately i probably won't be around to chat about it.  but if you do have time:
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705
* bac changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lifeless || queue: [lifeless,bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bac: I am off to lunch now but can look at it later ...
<bac> henninge: ok
<leonardr> henninge, if you get to it i'd appreciate a review of https://code.edge.launchpad.net/~leonardr/launchpad/oauth-doctest-to-unit-test/+merge/38715
<henninge> leonardr: sure
* leonardr changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lifeless || queue: [lifeless,bac,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> henninge: could you please review https://code.edge.launchpad.net/~abentley/launchpad/detect-xen/+merge/38718 ?
* henninge changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: lifeless || queue: [abentley,bac,lifeless,lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: lifeless || queue: [abentley,bac,lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> jml: your comment about "If you change the constructor of NewBuildersScanner ..." makes no sense to me.  Can you explain please?
<jml> bigjools: the constructor currently does a query to populate current_builders
<jml> bigjools: basically, you don't have to do that
<jml> *and*
<jml> you don't have to have most of the code in startService
<jml> just schedule the NewBuilders... call
<jml> and the first run will be exactly equivalent to the query in the constructor
<bigjools> jml: ok - I don't want to change this right because there's a bug somewhere that causes the manager to crash when a new builder is detected
<bigjools> s/right/right now/
<bigjools> I can address this with the bug if you think that's ok?
<jml> bigjools: sure.
<bigjools> well I say crash - it stops dispatching builds
<bigjools> but the log continues as if there were no issues
<bigjools> which is utterly bizarre
<abentley> rockstar: could you please review https://code.edge.launchpad.net/~abentley/launchpad/detect-xen/+merge/38718 ?
<rockstar> abentley, sure.
<abentley> rockstar: thanks.
<henninge> leonardr: approved
<leonardr> henninge, great
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [abentley,bac,lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> abentley: you just found yourself another reviewer?
<henninge> Hi rockstar! ;-)
<abentley> henninge: yes.
<henninge> ok, I'll move on to bac then.
<rockstar> henninge, hi!
<leonardr> henninge: i think i need to recreate request_token a lot because the lifespan of a request_token is not very long
<leonardr> well, i guess that's not what you were saying...
<leonardr> henninge: if i should use a mixin instead of a base class, but a mixin can't have a setup method, what should i do?
<henninge> leonardr: I am just saying that this pattern (a TestCase base class) cannot be used if the base class defines test methods, too.
<henninge> The test machinery will try to run them ,too.
<henninge> leonardr: for your case, you can leave it as it is. Sorry for the confusion.
<leonardr> ok, thanks
<leonardr> henninge: sorry, i'm going to keep bugging you
<leonardr> what do you mean that that "key = " line is needed for all tests? i don't think it's ever used again
<leonardr> do you want that to be the consumer key for self.consumer?
<henninge> let me look again, maybe I got mixed up.
<henninge> I mean ... never mind.
<henninge> ^ignore that line
<henninge> leonardr: yes, if you split the tests (in TestConsumerSet) up, you will need "key" in all but the "verifyObject" test.
<leonardr> henninge: i see what you mean. but, if i split up the tests, i can use self.consumer for all but the first one
<henninge> leonardr: My main theme is that the tests can be split up.
<henninge> Each test will need to be set up of course.
<leonardr> ok, i will split them up as much as possible and have you take another look
<henninge> So you'll have to decide if the objects you need for the tests to run are created in the test method or in the setUp.
<leonardr> got it
<henninge> good ;-)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bac || queue: [lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> adeuring1, hey, my MP is up to date now.  https://code.edge.launchpad.net/~deryck/launchpad/better-testing-for-status-changes/+merge/38552
<adeuring1> deryck: ok, I'll look
<deryck> adeuring1, I went with two tests, one for regular user and one for privileged users, even though there is some overlap because it read better.  And too avoid `if regular_user` type constructions.
<deryck> adeuring1, but if you want one test with that type check, I can do that.
<adeuring1> deryck: nah, I think that's fine
* henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring1> deryck: r=me; two nitpicks
<deryck> adeuring1, great, thanks!
<deryck> adeuring1, ah, I forgot about the other celebrities.  Sorry.
<adeuring1> deryck: no problem -- that's what reviews are for ;)
<deryck> indeed! :-)
<bigjools> jml: I pushed up a load of changes - I've not done everything you talked about, my will to live is slowly being sapped by this branch.  I've got some more changes to make tomorrow as described in the MP reply.
<jml> bigjools: sure
<jml> bigjools: I've got to go now, I think I've reviewed all of your changes from today though.
<bigjools> jml: thanks muchly
<bigjools> I am offski too
<henninge> leonardr: can you please give me an incremental diff?
<leonardr> henninge, sure
<leonardr> henninge_: http://pastebin.ubuntu.com/515781/
<leonardr> sorry for the delay
<abentley> rockstar: ping?
<rockstar> abentley, hi
<abentley> rockstar: chat?
<rockstar> abentley, sure.
<abentley> rockstar: Skype or mumble?
<rockstar> abentley, skype
* bdmurray changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [lifeless,lifeless, sinzui, sinzui, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> sinzui, bdmurray: please ask someone to perform an on-call review before adding yourself to the on-call review queue.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [lifeless,lifeless, sinzui, sinzui, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> abentley: okay
<abentley> bdmurray: thanks.
<abentley> sinzui: I do not know what foaf means.  Can you please explain it?
<abentley> sinzui: why are you writing the same module for each import, e.g. 25-26?
<sinzui> abentley, FOAF is an RDF vocabulary to describe a Friend of a Friend. It about 7 years old. It is one of the oldest RDF vocabs and many tools like python-rdflib support it out-of-the-bix
<abentley> sinzui: What is Friend of a Friend?
<sinzui> abentley, that is a by-product of the script's expansion of identifiers identifiers
<sinzui> abentley, http://xmlns.com/foaf/spec/
<sinzui> abentley, our RDF for projects and people mix our proprietary vocab with standard vocabs
<sinzui> We do not want to reinvent FOAF.
<sinzui> We do not want to reinvent DOAP (Description of a Project), but DOAP does not define concepts like series
<sinzui> So we ignore DOAP
<abentley> sinzui: Several of your examples say "Accoun" rather than "Account".
<sinzui> oh!
<sinzui> I think that means the script is bad
<sinzui> abentley, I ran the entire test suite without error Sunday!
<abentley> sinzui: I guess you need to make sure those scripts are being run, then...
<sinzui> abentley, sorry. I just had a panic. I was thinking of my apocalypse branch. That is importing correctly.
<abentley> sinzui: Ah, sorry.
<abentley> sinzui: for the apocalypse branch, have you considered updating the script to avoid repeating modules?  It seems like a small effort to avoid lots of work later.
<sinzui> abentley, I did and considered it an extra 4 hours to run and test to fix imports in tests I would rather delete
<sinzui> I certainly will investigate sorted multiline imports it you want me too
<abentley> sinzui: would you please?  I agree that it would be nice to convert/delete these.
<sinzui> okay
<sinzui> abentley, I must have been running tests in the wrong branch. This RDF branch has another idiotic problem be defining a user as a group. the tests rightly fails
<sinzui> s/be defining/by defining/
<abentley> sinzui: Ah.  Okay.  Please ping me when it's working.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: bdmurray|| queue: [lifeless,lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> bdmurray: was there a preimplementation call for https://bugs.edge.launchpad.net/malone/+bug/114766 ?
<_mup_> Bug #114766: Only bug supervisor should be able to nominate a bug for a release <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/114766>
<abentley> bdmurray: or rather https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-nominate-for-release/+merge/38733?
<bdmurray> abentley: yes, deryck and I discussed it
<abentley> bdmurray: it's helpful to note that in your merge proposall.
<abentley> bdmurray: did you check lint?
<bdmurray> abentley: no, doing so now
<abentley> bdmurray: If you install lpreview_body, then lp-propose will supply a cover letter template with the lint stuff filled in.
<bdmurray> abentley: where is lpreview_body?  I hadn't heard of this
<abentley> bdmurray: it's in the Launchpad team PPA and on Launchpad.
<abentley> bdmurray: https://edge.launchpad.net/lpreview-body
<abentley> bdmurray: on line 9, why are you checking target.bug_supervisor before doing check_permission("launchpad.Driver", target)?
<bdmurray> abentley: hmm, that does seem backwards
<abentley> bdmurray: The idea is that the BugDriver or bug supervisor can always do this, but if the bug supervisor is set, normal users cannot?
<bdmurray> abentley: yes, that is correct
<abentley> bdmurray: that seems a) complicated and b) like it could be provided as a permission.
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/databasefixture/+merge/38694
<abentley> bdmurray: it also means error messages that say "Only bug supervisors can nominate bugs." are not entirely accurate.
<bdmurray> abentley: okay, its not clear to me what you mean by providing it as a permission
<abentley> bdmurray: I mean "launchpad.NominateBug" as a permission that encapsulates the logic you're doing in the view code.
<bdmurray> abentley: hmm, if you look at security.py there is a "launchpad.BugSupervisor" permission that checks to see if the user is in obj.owner or in_admin. so perhaps the check_permission for launchpad.Driver is redundant?
<abentley> bdmurray: lemme see...
<sinzui> abentley, I fixed the two tests and fixed the person-rdf template: https://code.launchpad.net/~sinzui/launchpad/rdf-links-1/+merge/38573
<abentley> bdmurray: It seems like launchpad.Driver is meant to be "launchpad.NominateBug", doesn't it?
<abentley> sinzui: r=me.  There are some blank lines introduced at 64.  If they were not added deliberately, you may want to remove them before submitting.
<bdmurray> abentley: I was under the impression that sinzui was reluctant to add lots of permissions
<sinzui> abentley, thanks. I also need to fix the 2-space indentation in several of the tests. I will fix blank lines too
<abentley> bdmurray: I don't know that sinzui's preferences are authoritative here.  My own preference is to have a single point of truth.  We can't reuse that browser code everwhere we might want to check that permission.
<bdmurray> abentley: to be clear this is the modification I'm suggesting https://pastebin.canonical.com/38756/
<abentley> bdmurray: in fact, you could eliminate most of 100-113 if you did that.
<sinzui> bdmurray, abentley. We should not introduce new kinds of permissions [flacoste]. But we have always had bug_supervisor and it is not clearly defined in security.py, so I advocate defining it so I do not need to guess
<abentley> sinzui: I am getting confused about the difference between launchpad.Driver and this.  Is it the ability to unset bugs that would differentiate launchpad.Driver?
<sinzui> abentley, bdmurray: I believe there is agreement that models can define security checks that are imported in security.py
<abentley> sinzui: I would be fine with that.
<sinzui> bug_supervisors are a specialisation of drivers.
<sinzui> I think (because there is know authoritative checker) that bug_supervisors are effectively drivers with elevated permission to manage bugs
<sinzui> There is no requirement that a bug_supervisor be a driver, or vice-versa
<abentley> sinzui: This permission would allow all to nominate bugs unless a supervisor is set, in which case only the supervisor and driver would be able to.  Does that seem reasonable?
<bdmurray> abentley: all can already nominate bugs for a release, this restricts it if a bug supervisor is set
<abentley> bdmurray: that is what I was trying to day.
<abentley> s/day/say
<bdmurray> okay cool
<sinzui> abentley, I must defer to deryck, gmb, and allenap on that issue. I certain do not want anyone to ever make a nomination in any project I run
<deryck> yes, I feel convinced after talking to bdmurray that nominations are useless since anyone can make them.
 * sinzui looks for a bat to hit someone every time someone uses +nominate to vote for his personal issue
<abentley> deryck: if so, is it a good idea to allow anyone to make nominations if the bug supervisor is unset?
<sinzui> And why can someone nominate to fix a bug in trunk or an obsolete series? +nominate only works for projects that create forward series or have to manage backports to maintained series
<deryck> abentley, yeah, that's a good point.  I think bdmurray was trying to have as little impact as possible on other projects.
<deryck> abentley, bdmurray -- I think only driver or bug supervisor should be able to nominate for series.
<deryck> we ACL assign to milestone, why not series?
<bdmurray> deryck: drivers have their nominations approved automatically so that effectively removes nominations if there is no bug supervisor
<deryck> bdmurray, right
<deryck> that's how milestones work.  Shouldn't this be the same?
<bdmurray> but nominations are a way of escalating (in my mind) a bug report.  this removes this escalation path for projects w/o bug supervisors
<bdmurray> s/this removes/this would remove/
 * deryck is thinking....
<abentley> bdmurray: I don't know what nominations are, but according to the bug, normal people don't know how to use them correctly.
<deryck> I don't think anyone actually uses them as escalation.  ubuntu users do, but that's why we're adding this restriction. :-)
<deryck> sinzui, do you see any issue in treating series like milestone, and not make it available at all if you're not the owner or bug supervisor?
<sinzui> deryck, I think they should be the same
<deryck> ok, cool.
<sinzui> deryck, I think they are not the same because we implemented 2, not 1, way of planing bugs and features
<deryck> bdmurray, since sinzui and I are agreed, I think we should do as abentley suggests and make this only for owners and bug supervisors.
<deryck> sinzui, right, agreed
<bdmurray> okay, got it
<sinzui> I favour one implementation that allows users to say this should be fix by z, or is inclusive in x without needing to know how milestones or series are implemented
<deryck> sinzui, I get what you mean now.  but until we can get to that, do you have any concerns about having series behave slightly more like milestones in this way?
<sinzui> no. similar is easier to learn
<deryck> ok, cool.
<deryck> thanks
<sinzui> deryck, if we made series "look like" milestones, I could target a bug to series and then undo it!
<deryck> indeed!  Though that wouldn't be covered by bdmurray's current work.
<deryck> ok, so I need to bail for the day.
<deryck> Catch you all tomorrow.
<bdmurray> abentley: so is okay to use check_permission in model/bug.py to cleanup lines 100-113 ?
<rockstar> abentley, https://code.edge.launchpad.net/~rockstar/launchpad/merge-queues-model/+merge/38762
<abentley> bdmurray: works for me.
<henninge> leonardr: Hi! Now I know what got me confused - you did not push you changes.
<henninge> leonardr: but what I see in the incremental diff looks good ;-)
<rockstar> abentley, hello.
<abentley> rockstar: hi.  OTP
<rockstar> abentley, ack. Just wanted to say "Can I please get a review?"  That is all.  :)
* abentley changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar: You need another blank line at 73
<rockstar> abentley, indeed I do.
<abentley> rockstar: I would rather see BranchMergeQueue.new as a classmethod, not a staticmethod, so you don't need to repeat the classname.
<rockstar> abentley, ack.  Will make that change as well.
<abentley> rockstar: r=me with those changes.
<rockstar> abentley, thank you sir!
<wallyworld_> lifeless: you may want to claim this one https://code.edge.launchpad.net/~wallyworld/launchpad/fix-hardcoded-test-urls/+merge/38779
<wallyworld_> i decided to land it independent of your uniqueconfig one - i can come back and fix the hard coded config name once both have landed
<lifeless> thats great
#launchpad-reviews 2010-10-19
<thumper> anyone feel like reviewing some branches?
<mwhudson> thumper: maybe
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/rename-created-job/+merge/38681
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/needs-review-event/+merge/38686
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/defer-wip-email/+merge/38791
<thumper> a pipeline to defer the initial review email until it needs review
<mwhudson> thumper: why do you reset the root message id in the last one?
<thumper> mwhudson: because it will start a new threaded conversation
<thumper> if someone had commented on a work in progress
<thumper> that sets the root message id
<mwhudson> i guess that makes sense
<thumper> by resetting to send out the main message, we get a new thread
<mwhudson> also if you flip between needs review -> wip -> needs review again, i guess a new thread is appropriate
<thumper> yeah, that's my thought too
<thumper> mwhudson: were you going to comment on the third one above?
<mwhudson> thumper: oh right, yeah, one sec
<mwhudson> otp
<thumper> ack
<thumper> another simple one for someone: https://code.edge.launchpad.net/~thumper/launchpad/kill-bad-form-preloads/+merge/38800
<bac> hi lifeless
<bac> jtv, could i trouble you for a review?
<lifeless> hi bac
<bac> hi lifeless, you'd pinged me earlier.  so i have been unavailable until now
<lifeless> I did?
<bac> yeah, during the middle of my night.
<bac> lifeless: so i was just replying
<lifeless> I don't see a ping from me
<lifeless> are you thinking of the email one ?
<jtv> hi bac, still need that review?
<bac> lifeless: no, but i am in the wrong channel.  at 19:15Z in #launchpad-dev there was "lifeless: bac: hi"
<bac> lifeless: i'm not making it up, but if you don't need to chat that's a-ok too.  :)
<lifeless> hmm, oh right it was re that mail I think
<bac> lifeless: i'm replying to your email now, so i guess we are covered.
<lifeless> coolio
<bac> jtv: https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705
<jtv> gotcha
<bac> jtv: henninge grabbed it yesterday but i don't think he actually did anything
<jtv> bac: he won't be back for another few hours
<jtv> bac: is it okay to inherit both from LaunchpadFormView and LaunchpadView?
<bac> jtv: probably not.  i didn't realize that was happening.
<jtv> BranchVisibilityPolicyView inherits from LaunchpadView.
<bac> jtv: thanks for catching that.  i think it was a late change.  i'll investigate.
<jtv> bac: it looks as if for your use you really want BVPV to be a mixin.
<bac> jtv: yeah, i may have to go that route.  it pre-existed and i thought i could get lucky and re-use it as is
<jtv> It might work just fine, it just scares me a bit.
<bac> yes, i think it should
<jtv> bac: wasn't it possible for TestProjectGroupBranchesPage to render the view directly, without creating a browser?
<bac> jtv: i was burned so badly by trying to get a rendered translations view last week that i went with what i knew would work
<jtv> bac: IIRC it should (knock on wood) be a matter of soup = BeautifulSoup(view())
<jtv> Seems worth trying despite the trouble with our views!
<bac> jtv: good point.  i will try.
<jtv> thanks
<jtv> bac: on a sidenote, there's something in the docstring for that test that confuses me: "This is the default page shown _for a person_ on the code subdomain."  Should that be "to" a person (who by implication I guess must be logged in)?
<bac> jtv: fixed
<jtv> cool
<jtv> bac: also in your test, since you're using the whitespace-ignoring assertion helper, can you get away with multi-line strings for the text you expect in the privacy portlet?
<bac> jtv: what do you mean?
<bac> and why would i want to?
<jtv> bac: the indentation is a bit ugly and uses up a lot of horizontal space that you could use for meaningful content.
<jtv> 75+        expected = ("By default, new branches you create for projects in .* "76+                    "are Public initially. Individual "77+                    "projects may override this setting.")78+        self.assertTextMatchesExpressionIgnoreWhitespace(79+            expected, text)
<jtv> Hmmm... I bet I can do better:
<jtv> 75+        expected = ("By default, new branches you create for projects in .* "
<jtv> 76+                    "are Public initially. Individual "
<jtv> 77+                    "projects may override this setting.")
<jtv> 78+        self.assertTextMatchesExpressionIgnoreWhitespace(
<jtv> 79+            expected, text)
<jtv> How about:
<jtv> expected = """
<jtv>     By default, new branches you create for projects in .*
<jtv>     are Public initially.  Individual projects may override this setting.
<jtv> """
<jtv> I don't know if moving the line breaks is a problemâ¦?
<jtv> bac: ^^
<jtv> is what I meant
<bac> jtv: it is not a problem
<jtv> Then I think the multi-line string is slightly more natural.
<jtv> Also, if the BeautifulSoup tests you did are a problem, might it be worth putting more of the portlet's logic in the view and unit-testing the view directly?
<jtv> FWIW I got an order-of-magnitude speedup on one of our slowest pages that way.
<jtv> I don't know if those tests are still a major time sink for the test though; just a thought.
<bac> they are not
<bac> the portlet isn't really doing much
<jtv> ok nm
<henninge> bac: sorry, I totally forgot your review ...
<bac> henninge: np
<bac> you were busy
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> henninge: could i get you to do a UI review of the branch from yesterday?  https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705
<bac> henninge: i'll provide some screenshots in just a moment
<henninge> bac: I already saw it.
<bac> henninge: actually hold off a bit on a UI review.  i need to tweak something
<henninge> bac: I remember that the link in the portlet looked odd.
<henninge> i.e. the icon was not properly aligned with the text
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: I've got a large MP with a sadistic titleâ¦ interested?  https://code.launchpad.net/~jtv/launchpad/recife-pofile-owner-privs/+merge/38827
<gmb> jtv: Sure; I'm always willing to have pain brighten my afternoon.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * jtv laughs
<henninge> jtv: there are text conflicts in your mp. Did you merge the latest stable into recife and push that?
<jtv> henninge: ah, not yetâthanks
<jtv> I merged db-stable into recife earlier today.
<jtv> â¦so recife got lots of changes.
 * jtv merges & twiddles
<jtv> gmb: just pushed an update to resolve two very minor conflicts.
<gmb> jtv: Okay, thanks.
<jtv> My diff has updated.
<jcsackett> gmb: have time for another review today?
<gmb> jtv: r=me
<gmb> jcsackett: Sure.
<jtv> gmb: great, thanks!
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> gmb: great, thanks. mp is here: https://code.edge.launchpad.net/~jcsackett/launchpad/new-releases-636060/+merge/38773
 * gmb looks
<gmb> jcsackett: A couple of nitpicks w.r.t styling but otherwise r=me.
<gmb> https://code.edge.launchpad.net/~jcsackett/launchpad/new-releases-636060/+merge/38773
<jcsackett> gmb: cool, thanks. I'll make those changes.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> mars, i need to go to a doctor's appointment, but i'd like to put https://code.edge.launchpad.net/~leonardr/launchpad/temporary-integration/+merge/38836 on the queue, with the understanding that if it turns out you need any input from me to do the review, you can just put it on hold until i come back
<gmb> leonardr: Can I assume that that message applies to me as well?
<leonardr> gmb, yes indeed
<gmb> leonardr: Okay, thanks.
<EdwinGrubbs> gmb: can I get a code review? I've already gotten ui reviews on this branch: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-652232-person-code-action-links/+merge/38574
<gmb> EdwinGrubbs: Sure. I'll put it in the queue after leonardr-afk's branch
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: - || queue: [leonardr, EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * gmb -> getting a drink. bbi10m.
<mars> morning gmb
<mars> gmb, I'll take leonardr-afk's branch
* mars changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: -,leonardr || queue: [EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> mars: Cool.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: EdwinGrubbs, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * gmb hates at the '+junk' moniker for the umpteenth time.
<gmb> EdwinGrubbs:  r=me with a tiny little nitpick
* gmb changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: -, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> gmb: thanks
* mars changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: -, leonardr || queue: [matsubara(mars)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<matsubara> thanks mars
<bdmurray> could I get https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-modify-distro-official-tags/+merge/38743 added to the review queue?
<mars> bdmurray, sure
* mars changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: -,afk || queue: [bdmurray,matsubara(mars)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: afk || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> gmb: did my branch get reviewed or dropped from the queue?
<gmb> bdmurray: Whoops, I stomped on it. Sorry.
* gmb changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: afk || queue: [bdmurray, matsubara] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bdmurray: By way of making it up to you I'll take a look now :)
<bdmurray> gmb: it should be an easy one
<gmb> bdmurray: Indeed. r=me
<bdmurray> thanks!
<jml> need a review on a very simple CP: https://code.edge.launchpad.net/~jml/launchpad/codebrowse-config-cp/+merge/38858
* mars changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: afk || queue: [matsubara] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: - || queue: [matsubara] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: matsubara || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> mars, any advice on where to put the screenshot for the ui review? and who might be around to do it right now?
<mars> leonardr, sinzui is the head reviewer, he may punt to mrevell.  people.c.c or devpad are good for screenshots, edit the description to add the link.
<sinzui> I am a head reviewer?
 * sinzui wonders what super powers that conveys
 * mars anoints sinzui as one of the head UI reviewers
<sinzui> leonardr, screenshots are often linked in the review comments. the images/movies may be bug attachments or uploaded to people.canonical.com
<leonardr> sinzui: it's like a phrenologist
<mars> sinzui, it conveys the superpower of mentee delegation, the ability to move mountains by giving the task to other people.
<sinzui> leonardr, :)
<leonardr> sinzui: if you can do the review today-ish, maybe i can just put the screenshot on my personal site?
<sinzui> leonardr, yes and yes
* mars changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> sinzui, the mp is https://code.edge.launchpad.net/~leonardr/launchpad/temporary-integration/+merge/38836
<mars> Out for a lunch errand, back shortly
* bryceh changed the topic of #launchpad-reviews to: On call: mars || Reviewing: lunch || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> salgado, are you about
<salgado> hi sinzui
<mars> bryceh, wow, uhm, a 14,000 line diff - might take me a while
<sinzui> salgado, do you have time for a UI review?
<mars> bryceh, is any of this stuff generated?
<salgado> sinzui, not today, but I can do it early tomorrow if it's not urgent
<bryceh> mars, no but that seems excessively large
<bryceh> mars, it's against db-devel, but perhaps the diff was made against devel?
<mars> bryceh, it was made against your previous branch
<mars> and db-devel
<mars> bryceh, have a look: https://code.edge.launchpad.net/~bryce/launchpad/lp-617691-retrieve/+merge/38804
<sinzui> leonardr, I would like salgado to do the first UI review of your branch? Can you wait until tomorrow?
<mars> bryceh, it /really/ looks like a lot of generated JavaScript arrays and generated HTML
<leonardr> sinzui, no problem
 * sinzui is feeling ill
<sinzui> thanks leonardr
<bryceh> mars, oh right, there's a testfile html file which is a copy of a page from bugs.freedesktop.org for testing the parser
<mars> bryceh, and the JavaScript looks strange.  Is line 840 even valid JS?
<bryceh> mars, it's intentionally broken, however come to think of it I don't think I added a test to check that so guess I could just drop the file
 * bryceh doublechecks
<mars> lets see what lsdiff says here
<mars> ok, that's better
<bryceh> mars, ok dropped
<mars> bryceh, http://pastebin.ubuntu.com/516417/
<mars> 803 lines, much nicer
<lifeless> henninge: so here; I'm surprised you want me to land code in a higher entropy state than I presented it; thats something I am finding very hard to understand
<henninge> lifeless: sorry, entropy?
<henninge> ;)
<lifeless> henninge: worse off ;)
<lifeless> henninge: you want me to split two things that are tightly related
<lifeless> in preparation for future work that would combine them
<lifeless> why is that better ?
<henninge> lifeless: well ...
<henninge> ... it could serve as an incentive to clean it up quickly.
<lifeless> thats not the goal of reviews though
<lifeless> I have tonnes of incentive to fix things in the code base
<lifeless> I need more time in the day, not more incentives
<henninge> same here ;)
<lifeless> *we* need understandable code and what you are proposing makes things less understandable until the later action is take.
<henninge> True. I would very much prefer to do the move first.
<lifeless> I think that that needs a benefit that is greater than the cost
<henninge> What I suggested was the second-best solution.
<lifeless> in which case we're holding an improvement to the system hostage to existing tech debt
<henninge> lifeless: I won't force this on you. Please add the files as you propsed but also add a bug for moving the code.
<lifeless> I'm delighted to do that
<lifeless> I'll put an XXX in the config/__init__ and in the new files.
<henninge> thanks
<lifeless> would you like to see an incremental of the XXX ?
<lifeless> I realise it will be getting late for you
<henninge> lifeless: I started late today
<henninge> let me look at the mp I think there were a few other issues that I did not mention yet.
<henninge> lifeless: ah yes, can you please use "dedent" in fixture.py and test_fixture.py?
<lifeless> henninge: not easily
<henninge> why not?
<lifeless> henninge: it has to be byte-for-byte
<lifeless> my experience with dedent in the past on that sort of thing has been traumatic
<lifeless> is the tag techdebt or tech-debt
<henninge> tech-debt, I believe
<henninge> yup
<henninge> lifeless: I am not aware of those problems with dedent.
<lifeless> ok, will try
<henninge> The other option would be to but the string in a global variable that can be defined at file level.
<henninge> has less indention.
<henninge> It's just not very readable the way it is now. Thanks for trying.
<lifeless> I think thats a good idea regardless
<lifeless> perhaps class scoped.
<henninge> could work, too.
<henninge> but as I said, I'd like to know more about those dedent problems you mentioned. I never heard of those.
<henninge> lifeless: also, I try to avoid multiline if conditions at all costs, as they read badly, too.
<henninge> that's in scripts/__init__.py and logger.py
<lifeless> I don't love them either, but its better than a temp variable most of the time
<henninge> why is a temp variable not a good solution?
<lifeless> they generally wrap as well
<henninge> thanks for converting from unittest.TestCase to testtools, btw. ;)
<lifeless> and you then have an assigned name which can be (mis)used, because its scoped too broadly
<lifeless> I'd prefer to extract a helper method if the if gets *that* hairy
<lifeless> but we often have a lot of infunction state in LP, or so it seems to me
<henninge> but that is even more overhead.
<lifeless> yes, but it can be directly tested
<henninge> Also, a temp variable can help documentation.
<lifeless> and isn't bound to a local so can't be misued
<henninge> good point.
<lifeless> for instance those ifs that you notice
<henninge> but I don't see such a big risk of misuse
<lifeless> in another pass at this, I'd be inclined to push that condition onto config
<lifeless> 'if config.is_testing:
<lifeless> or 'if config.is_testing():'
<henninge> Yes! Brilliant! ;)
<henninge> lifeless: Also, I think you can do without the second "if not BaseLayer.persist_test_services:" in layers.py but I may be misreading that.
<lifeless> probably fallout, I wavered a couple of times each way there
<lifeless> ok, dedent is working for this
<henninge> cool
<lifeless> I'm using the python reference doc style
<lifeless> thanks for spotting that unneeded if
<lifeless> yes, I'd missed nuking it
<lifeless> I've pushed this up; I haven't done the config change - its fairly small, but I'd want to look around for similar stuff etc, and I'm sure there's more, so its not trivial to do well.
<henninge> lifeless: looks good now. Thanks a lot. r=me ;-)
<lifeless> my pleasure, thanks for the feedback
<jcsackett> mars, have room for another in that queue?
<jcsackett> or, actually, does anyone have time? it's 89 lines, but most of that is headings updated in a doctest.
<mars> jcsackett, link?
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [bryce, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> mars: https://code.edge.launchpad.net/~jcsackett/launchpad/add-member-648476/+merge/38887
<mars> thanks
<mars> jcsackett, rs=mars
<mars> jcsackett, no tests?
<jcsackett> jcsackett: debatable need--it's exactly the same snippet that's tested elsewhere.
<jcsackett> er, mars ^
 * jcsackett must need more coffee.
<mars> ah, ok,
<mars> jcsackett, well, I approved it
<jcsackett> thanks, mars.
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> abentley: I've updated the nomination branch per yesterday's discussion
<abentley> bdmurray: past EOD, but I'll look at it first thing before me.
<bdmurray> abentley: okay, thanks
* mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: bryce || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-10-20
<thumper> mwhudson: ping
<mwhudson> thumper: pong
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/defer-wip-email/+merge/38791 ?
<lifeless> henninge: I've done config.isTestRunner - there was another site that needed updating, so refactor + JFDI
<mwhudson> thumper: looking
<mwhudson> thumper: i was sure i reviewed that yesterday :/
<mars> StevenK, online?
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> jtv, was just going to point out to StevenK that I saved a bunch of reviews for him in the +activereviews queue
<jtv> mars: I get a vague sense that you're hinting at something
<jtv> It must be lack of coffee.
<mars> jtv, well, if he is an OCR mentee today, then perhaps you and he can work out some way to split the workload so that he can gain more experience, but not be overwhelmed with the volume
<jtv> mars: I don't see anything of yours under Launchpad's "requested reviews I can do"â¦
<mars> jtv, I was OCR last shift - I left the reviews on the +activereviews queue so StevenK could pick some of them up
<jtv> ah so
<thumper> jtv: hi
<jtv> hi thumper
<jtv> is this about reviews?
<thumper> jtv: trivial one for you: https://code.edge.launchpad.net/~thumper/launchpad/kill-bad-form-preloads/+merge/38800
<thumper> jtv: yes... yes it is
<jtv> I hate that word: "trivial"
<jtv> because it never is.
<thumper> sure it is
<jtv> Growing JS in the TAL, I see.
<jtv> thumper: is the LP.client.links['me'] !== undefined check something standardized?
<thumper> jtv: used elsewhere
<thumper> I'm just using it again
<thumper> standardized? prolly not
<jtv> This is something I don't like about "undefined": what if someday it becomes a null?  Will we notice that this code breaks?
<thumper> jtv: only with a shed load more 404s :)
<thumper> jtv: like we get now
<jtv> :/
<jtv> I know our coding guidelines like explicit comparisons, but it'd be so much more robust to test if (!foo)â¦
<jtv> And adding 2 windmill tests for this is a hefty price to pay as well.  Crap.
<jtv> thumper: have you considered solving the problem in TAL?
<thumper> which problem?
<thumper> the logged in one?
<thumper> no, not really considered it at all
<jtv> I was wondering if we could have a marker class on #portlet-subscribers, so that there's simply nothing to hook the lazr widget into if it's not appropriate to show it.
<jtv> The marker class would appear only if it's appropriate to show the widget.
<jtv> When the marker class is absent, installing the widget AIUI should do nothing because it won't find the bit of HTML that it's trying to hook into.
<wgrant> StevenK: Is there an existing constant for that?
<wgrant> I couldn't find one.
<StevenK> wgrant: There could be, I meant to add "i'm not certain"
<StevenK> Perhaps move the constant to same places as the others?
<wgrant> StevenK: Ah. I guess the "It's" could have been "Is" or "Isn't", and I read it as the latter.
<wgrant> I could.
<wgrant> IMHO it belongs next to the publisher.
<wgrant> But I could be wrong.
<jtv> thumper: I think my modem wanted to be alone for a while.  What was the last you saw of me?
<thumper> <jtv> When the marker class is absent, installing the widget AIUI should do nothing because it won't find the bit of HTML that it's trying to hook into.
<thumper> but I wasn't listening :)
<jtv> thumper: that's a shame, because leaving this free to regress untested isn't great either, and two windmill tests would be overkill.
<StevenK> thumper: I've got two mentor reviews I've just subscribed you too if you have a few seconds.
<thumper> StevenK: ack
<thumper> jtv: I'll think on it and discuss with my JS expert, rockstar
 * rockstar huhs
<jtv> thumper: sure.  What I'm suggesting is not going to be as pretty as it sounds, because pure TAL won't help you inject conditional classes.  So a bit of help from the view would probably be needed.
<jtv> rockstar: hi
<rockstar> hi jtv.  Am I up that late?
<jtv> rockstar: yup.  :)
<jtv> We're just talking about this MP: https://code.edge.launchpad.net/~thumper/launchpad/kill-bad-form-preloads/+merge/38800
<rockstar> Ooh, I hope this is what I think it is.
<jtv> It conditionally disables subscription widgets if the user is not logged in.
<jtv> But that's awkward to test (windmill!  I'm Dutch and I don't even like to work with it)
<jtv> and currently untested, which I don't care much for either.
<rockstar> jtv, that's okay, since the subscription widget works without javascript.
<rockstar> So it only disables the progressive enhancement.
<jtv> Yes.
<rockstar> Although I guess we could teach the widget to look for lp.me, and if it doesn't exist, don't create the activator for the form overlay.
<jtv> That'd be good too.  After all, nobody who isn't logged in has any business editing anything at all in LP.
<jtv> Is this an LP widget or a lazr widget?
<rockstar> jtv, it's an LP widget that inherits from a lazr widget.
<jtv> rockstar: then what you say would be ideal
<rockstar> jtv, yeah, as I look at this patch, I wonder if it's fair to kill all/most javascript for those not logged in.
<jtv> That's a good question.
<rockstar> Most of our javascript is for editing anyway.
<jtv> Definitely not all JS, but the only thing you could sensibly do with the API in JS is log in.
<rockstar> jtv, thumper's change here is good, methinks.  The subscription portlet's progressive enhancement is specifically for handling inline subscriptions.  If you're not logged in, why bother with the javascript?
<jtv> rockstar: I agree with that part.  What bothers me is leaving it untested.
<jtv> But I don't see a justification for several additional windmill tests either.
<rockstar> jtv, I don't think they're necessary.
<rockstar> jtv, also, windmill is at the top of my shit list right now.
<jtv> I hear you.
 * StevenK points at his thread on -dev about windmill's other problem
<jtv> StevenK: which thread is that?
<StevenK> jtv: 'Failures in Hudson'
<StevenK> on, even
<jtv> No wonder I didn't recognize it.  :)
<jtv> I thought it was just people complaining about Michael.  :-)
<StevenK> Bwahaha
<StevenK> It couldn't be that, mwhudson didn't reply with "How dare you!?"
<jtv> Oh well, I don't suppose windmill's individual failings are relevant hereâwe'd probably never finish.
<StevenK> jtv: And see -dev, since lifeless rocks
<jtv> rockstar: one final attempt at getting this testable _without windmill_ though.
<jtv> I was thinking we could add a marker class to the subscription portlet that only shows up if it's appropriate for the user to have the subscription editing widget.
<rockstar> jtv, that seems to be a hack to get around the fact that our tools are stupid.
<jtv> More a hack to get around the fact that JS will let you get away with anything and in this case, potentially produce a regression that lots of users will notice but we may not.
<rockstar> jtv, what's the regression potential here?
<jtv> It keeps most of the logic in slightly more tightly controlled areas.
<jtv> The "me" check goes wrong.
<rockstar> jtv, there's a windmill test already that makes sure that you have the JS when you are logged in.
<jtv> rockstar: eparse
<jtv> What does "have the JS" mean?
<rockstar> jtv, the progressive enhancement has tests for logged in users already.
<rockstar> So the test you're proposing is merely "create a windmill test that makes sure there ISN'T javascript executed."
<jtv> As I've been saying again and again, I'm not proposing a windmill test.  I hate windmill tests!
<jtv> I'm saying I don't like logic in places where only windmill can cover it.
<jtv> The reason being that it can fail visibly without us ever noticing.
<jtv> Don't we have a plain "user is logged in" function or variable somewhere?
<jtv> That would address the same problem.
<rockstar> jtv, yeah, but I don't think the test is going to do any good at all without testing the client (where the fix is occurring)
<jtv> True.
<jtv> A "user is logged in" function could at least be unit-tested instead of integration-tested though; if we had one, it'd take just that bit more risk out of the JS.
<jtv> I'm approving as-is, but it's clear that we have some desirable improvements to the JS infrastructure.
<rockstar> jtv, considering that this bug only manifests itself as an error in a javascript console that most users won't look at, and that if we find an issue in it, we've still got a test to assert the progressive enhancement works when a user is logged in, I think the risk is pretty small.
<jtv> I wasn't aware that it wasn't user-visible.
<rockstar> jtv, yeah, it sounds like the rest of Canonical has jumped the windmill ship already.  We're a bit behind on the times.  :)
<jtv> Jumping the windmill ship okay, but where to?  We always knew windmill sucked, but at the time there was simply no alternative.
<rockstar> jtv, landscape is using jstestdriver, and u1 has set a course to jstestdriverland as well.
<rockstar> And lazr-js already uses jstestdriver
<jtv> That looks nice.
<jtv> thumper: I approved your branch but I'm thoroughly disoriented by the UI.
<jtv> I give an Approved vote for "js."  Now I can't add an Approved vote for "code."
<jtv> So I request another review, from myself, for "code."
<jtv> Now my js approval shows up as a code approval.
<bac> yeah, you need to combine them "code js"
<jtv> Gah.
<bac> double gah
<jtv> So now I change the type again by requesting yet another review from myself.
<bac> you can just add another comment to the review
<bac> with 'approve' / 'code js'
<rockstar> bac, do you still need me to chair the meetings tomorrow?
<bac> yes, rockstar.  and i thank you.
<bac> i meant to remind you but forgot
<rockstar> bac, do you have a wiki page to point mo to with old bidness?
<bac> hope the 8am meeting isn't too too early for you
<bac> rockstar: just the ReviewerMeetingAgenda
<bac> on dev
<bac> rockstar: i'll update it this afternoon
<rockstar> bac, no, I have a 8:10 class every weekday now.
<bac> in person?
<bac> can you do your class and the meeting?
<rockstar> bac, no, but I can miss it.  I'm pretty ahead in that class.
 * bac contributes to truantism
<rockstar> It's the Tues/Thurs PM class that I can't miss much more of.
<bac> you can tell i used to watch "Little Rascals"
 * bac pedals off in search of lunch
<rockstar> Set up canonical.testing.layers.AppServerLayer in 3 minutes 11.145 seconds.
<rockstar> What. The. Hell.
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: mvogt || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> henninge: i've made changes to my branch and need a UI review.  can you give it a look?  screenshots are attached.  https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705
<bac> henninge: also, you mentioned the edit icon being misplace, but i'm rendering that link as done elsewhere
<henninge> bac: otp now but will look at it later.
<bac> henninge: thanks!
<thumper> StevenK: ping
<thumper> StevenK: I'll go over https://code.edge.launchpad.net/~wgrant/launchpad/no-lucilleconfig-directories/+merge/38650 once wgrant has got back to you and you've approved the review
<jtv> bigjools: I'm looking at mvogt's branch from a code-review perspective.  Have you looked at the design and if so, any problems with it from that angle?
<StevenK> thumper: Sorry, I was out walking. I've commented on it.
<bigjools> jtv: what branch?  (can you tell I know nothing? :))
<jtv> bigjools: https://code.edge.launchpad.net/~mvo/launchpad/support-timeframe-fix-660433/+merge/38503
<bigjools> jtv: that is not ready for review, I need to evaluate it first
<thumper> StevenK: but you haven't approved it, I'll look then
<thumper> StevenK: it seems to be good for now
<thumper> I've got nothing extra to add
<StevenK> thumper: Sorry, let me do so.
<thumper> StevenK: are you happy with it now?
<bigjools> jtv: I told him I'd need to test it on dogfood before it got further, I didn't know there was an MP because it's not linked to the bug
<jtv> bigjools: I'll fix that
<jtv> bigjools: if convenient, I'd be grateful if you could have a look so that I'll be free vote Approved if appropriate.
<bigjools> jtv: sure thing, after my standup in 30 mins
<jtv> cool
<jtv> linked.
<StevenK> thumper: I've approved it
<bigjools> jtv: FWIW I don't like the mocks that live in files all over the place
<jtv> bigjools: I noticed somebody moved some of our test-only objects into main code...
<bigjools> jtv: ?
<StevenK> bigjools: 25 minutes until the stand-up?
<bigjools> StevenK: no :)
<thumper> StevenK: approved too
<StevenK> thumper: Danke
<thumper> np
<jtv> bigjools: I thought that was what you meant with "mocks that live in files all over the place."
<bigjools> jtv: no, he's created a ton of new directories with weird stuff (TM)
 * jtv likes Weird Stuff
<bigjools> cronscripts/publishing/tests/mock-data/mock-bin/germinate for example
<jtv> And by the way, <compose>tm
<bigjools> that does nothing here
<jtv> Isn't that the part where he also noted this himself, and cleaned it up later?
<bigjools> jtv: it's in the current diff
 * bigjools -> standup
<bigjools> back in a bit
<jtv> Ah there
<jtv> OK
<bigjools> jtv: I asked cjwatson to take al ook
<bigjools> a look, too
<jtv> Well a good Al Ook is nice too.
* jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer  || Reviewing: mvogt || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv, jelmer  || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jelmer: we've got 2 more on the queue, but I'm having a call soon followed by EOD.
<jelmer> jtv: Ah, ok
<jelmer> jtv: I'll have a call soon as well, but I'll take a look at wgrants branch first.
<jtv> jelmer: great, thanks
<jtv> I'll see if I can squeeze in Ian's.
<wgrant> jelmer: Thanks.
* jtv changed the topic of #launchpad-reviews to: On call: jelmer  || Reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jelmer: I'll still try to review that other branch, just not open for OCR business any more for the day.
<jtv> jelmer: I'm afraid I won't be able to review Ian's branch tonight, sorry!
<henninge> bac: ui review done. Sorry for the delay, I was hunting misalignments ...
<jelmer> jtv: No worries; I'll have a look after I finish reviewing William's branch.
 * jelmer lunches
<deryck> hi allenap.  wb!
<deryck> allenap, can we get qa done for bug 655567?
<_mup_> Bug #655567: Wire up getSubscriptionsForBug() into the notifications machinery <qa-needstesting> <story-subscribe-to-search> <Launchpad Bugs:Fix Committed by allenap> <https://launchpad.net/bugs/655567>
<allenap> deryck: Sure.
<deryck> allenap, thanks.  I meant to ask on #launchpad-dev.  irc client tab fail. :-)
<deryck> jelmer, hi.  I have one for review.  Can I get in line on the queue?
<jelmer> deryck: yes, of course
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer  || Reviewing: wgrant || queue: [deryck]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> jelmer, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/lock-fix-released-status-126516/+merge/38931
<leonardr> jelmer, i request a review of https://code.edge.launchpad.net/~leonardr/launchpadlib/remove-broken-code/+merge/38935
<jelmer> leonardr: Wow, that removes a lot of code.. I'll have a look after Deryck's branch.
 * leonardr regrets ever writing that code in the first place... should have stuck to my guns
* leonardr changed the topic of #launchpad-reviews to: On call: jelmer  || Reviewing: wgrant || queue: [deryck,leonardr]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: deryck || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> deryck: Hi
<deryck> hi jelmer
<jelmer> deryck: Is your MP linked to the right bug? As I read it the bug report asks for disallowing unprivileged users to not be allowed to set the bugstatus to FIXRELEASED, whereas the branch (as I understand it) disallows them from moving away from FIXRELEASED.
<deryck> jelmer, well, it's the bug I claimed as being for my work, understanding it wasn't a perfect fit.  See my last comment on the bug.  but I certainly could have misread the bug.
 * jelmer apologizes for his bad English.. there's an unnecessary use of the word allow in that senstence
 * deryck looks closer
<deryck> jelmer, yes, that bug is not really related as my fix stands now.  I was originally going to go the full route of making fix released a bug supervisor status, but then changed my mind.
<deryck> jelmer, I'd like to land this as an incremental fix then, and think further about allowing FIXRELEASED as a fully limited bug supervisor status.
<deryck> or I can just unlink the branch and file a new bug.
<jelmer> deryck: Either seems reasonable, just making sure that bug wouldn't be closed accidentally when this branch lands.
<deryck> jelmer, yes, thanks.  I'll follow up and make sure about that.  good catch.
<abentley> bdmurray: ISTM that NominateBugForProductSeries and NominateBugForDistroSeries vary only in their usedfor attribute.  Correct?
<jelmer> deryck: r=me
<deryck> jelmer, thanks!
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: leonardr || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, would appreciate your help on https://code.edge.launchpad.net/~leonardr/launchpad/temporary-integration/+merge/38836
<bdmurray> abentley: yes, that's correct.  is there another way to do that?
<abentley> bdmurray: You could have a base class that didn't have a usedfor attribute, and provide  NominateBugForDistroSeries and NominateBugForProductSeries as subclasses.
<bdmurray> abentley: I think there are some other places I could use that base class too so maybe that'd be better for a separate branch?
<abentley> bdmurray: Sure.
<abentley> bdmurray: Around line 45, did you consider using "if check_permission() or check_permission()" instead of if/elif?  That would avoid repeating the Link code.
<rockstar> jelmer, hi. I'd like to hop on your queue.  Do you have time?
<abentley> bdmurray: Oh, I see that the actual text of the link varies.
<jelmer> rockstar: yep, go ahead
* rockstar changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: leonardr || queue: [rockstar]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> abentley: right, I consolidated it as best I could
<abentley> bdmurray: the text variation is because the bug driver's "nomination" is automatically approved?
<bdmurray> abentley: yes, that is correct
<abentley> bdmurray: Your change introducing LinkNotFound reminds me that we do not normally disable links for anonymous users, on the assumption that they can log in if they get an Unauthorized error.
<abentley> bdmurray: around 217, why have you changed specific imports to imports from canonical.launchpad.interfaces?
<bdmurray> abentley: That might have been from a merge conflict I had.  It is better to have the specific imports then?
<abentley> bdmurray: yes, it is.  It reduces the chance of circular import problems, and it reduces our dependence on the deprecated canonical.launchpad namespace.
<bdmurray> abentley: okay, fixing
<abentley> bdmurray: I also see that around 68
<abentley> bdmurray: sinzui is working to eliminate use of canonical.launchpad: https://code.edge.launchpad.net/~sinzui/launchpad/apocalypse-interface-imports-7/+merge/38727
<jelmer> leonardr: r=me
<leonardr> yay
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> deryck: our usual pattern is to show all links to anonymous users, and if they get Unauthorized, they can log in.  Are you wanting to change that for this branch? https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-nominate-for-release/+merge/38733
<deryck> abentley, hmmm, my instinct is to say show the link, though I did say don't show it initially.  I was thinking of how we do milestones.  but that is different, given it's an edit item in the bugtask table.
<abentley> deryck: thanks.
<abentley> bdmurray: please change it so that a link is shown for anonymous users.
<bdmurray> abentley: will do
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || Reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> abentley: okay changes made and pushed
<abentley> bdmurray: looking.
<abentley> bdmurray: don't logged-in users have launchpad.View?
* jcsackett changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || Reviewing: rockstar || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> jelmer, EdwinGrubbs: mp when you have time is here: https://code.edge.launchpad.net/~jcsackett/launchpad/join-not-allowed-244527/+merge/38949
<rockstar> jelmer, did you find my mp?
<jelmer> rockstar: yep - it's the merge-queues-api one, right?
<rockstar> jelmer, yup
<jelmer> EdwinGrubbs: Can you review jcsackett's branch? I will probably not have time for another review after rockstars.
<EdwinGrubbs> jelmer: I'll do it.
<bdmurray> abentley: right.  okay, fix pushed.  I was on a call.
<abentley> bdmurray: r=me
<salgado> leonardr, do you still need help with that branch or did sinzui's reply clarify things?
<leonardr> salgado: i'm working on it now, i'll ask you if i have questions
<salgado> ok, cool
* jelmer changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: -  || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, any ideas how to word the first sentence so that it mentions "all applications"? would it be enough to put that information in the header?
<salgado> leonardr, well, the first sentence I suggested already mentions "all applications"
<leonardr> salgado: that's in the second sentence
<leonardr> first sentence is "the (whatever) called (whatever) wants access to your launchpad account"
<bdmurray> EdwinGrubbs: could I get https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-bug-expiry/+merge/38958 added to the queue?
<EdwinGrubbs> bdmurray: sure
<salgado> leonardr, sorry if I wasn't clear, but I meant to drop that
<salgado> I don't think it's of any help
<leonardr> salgado: now i'm quite confused. you suggest "Would you like to give all applications running
<leonardr>   on <mycomputer> full access (including making changes) to your Launchpad
<leonardr>  account?"
<leonardr> but now you don't think that's helpful? i think it is
<leonardr> otherwise it's not clear to a naive user exactly what it means to give "your computer" access, is it something on the kernel or hardware or what
<salgado> leonardr, I meant that that should be the first sentence
<leonardr> ah, the current first sentence should be removed
<salgado> yes
<leonardr> i respectfully disagree--i think the wording is fine now (although the heading could be changed), and that part of the ui was already reviewed by someone else. how can we resolve this?
<leonardr> salgado -^
<salgado> leonardr, well, I don't think there's much value in reviewing parts (in this case, one or more sentences) of a UI -- they ought to be reviewed as a whole.  I suggested you drop it because it doesn't (IMHO) help the user in deciding what to do, but if you disagree that's fine, although it's good to state your rationale in the MP for further reviewers to know
<leonardr> salgado: ok. my rationale is that "this computer wants access to your launchpad account" is very vague, especially to a naive user
<leonardr> and even to a knowledgeable user, because it's not technically accurate. your *desktop session* wants access to your launchpad account
<leonardr> but explaining that will confuse naive users even more
<salgado> is that why you want to keep the vague first sentence?
<leonardr> salgado: yes. the vague first sentence makes a naive user say 'oh, ok', and the second sentence makes an experienced user say 'oh, that's what they mean by 'my computer''
<salgado> leonardr, I buy that
<salgado> leonardr, I can't think of a way to include "all applications" in the first sentence, though, but maybe sinzui or mrevell can?
* bdmurray changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: -  || queue: [jcsackett, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> EdwinGrubbs: https://code.edge.launchpad.net/~lifeless/launchpad/bug-663828/+merge/38967
* lifeless changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: -  || queue: [jcsackett, bdmurray, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, sinzui, now i have a real ui question
<leonardr> i'm repeating all the "allow access" buttons as per your suggestion, and grouping them separately from the "deny access"
 * sinzui channels beuno
<leonardr> i need to do two things: 1) indent those buttons with respect to the text they're grouped under
<leonardr> 2) put some vertical space between those buttons
<leonardr> i have no idea how to do 1)
<leonardr> for 2), putting each button in its own paragraph gives the right amount of space, but that seems weird. sinzui's suggestions were to use a <div> (which doesn't add vertical space) or a <br /> (which doesn't add enough)
<sinzui> I anticipated 2 and wrote the css for 1
<leonardr> any ideas?
 * sinzui looks as CSS
<leonardr> sinzui: the only css you have is class='actions', and that's not doing anything for me
<sinzui> leonardr,  the class .subordinate to indent them 2em. we have forms that are generating <ol class="subordinate"> that do extra indentation
<leonardr> all right
<sinzui> leonardr, there is a ul.buttons class. That sounds like you want, but i cannot think about where it is used. It ensures padding around them
<lifeless> EdwinGrubbs: are you still ocr?
<sinzui> leonardr, so maybe <ul class="buttons subordinate"> and a li around each button
<EdwinGrubbs> lifeless: yes
<leonardr> sinzui: class="buttons subordinate" presents all the buttons on one line with no indentation...
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: jcsacket || queue: [jbdmurray, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> EdwinGrubbs: ok cool
<leonardr> as does "buttons" on its own. so maybe "buttons" is not what we want
<sinzui> leonardr, sorry, I see the li rule for button is causing that
<sinzui> leonardr, try subordinate by itself
<leonardr> sinzui: looks great
<leonardr> thanks
<leonardr> sinzui, salgado: one more idea. i could change [allow foo to access my launchpad account] "Permanently" to "Until I Disable It". good idea?
<sinzui> +1
<salgado> +1
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: bdmurray || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, sinzui, if you refresh the screenshot on my personal site it should be more to your liking now
<salgado> leonardr, what's the url again?
<leonardr> salgado: let me just attach it to the bug
<leonardr> salgado, sinzui: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703638/+files/Screenshot-1.png
<_mup_> Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436>
<leonardr> s/Permanently/Until I Disable it/
<sinzui> leonardr, have you seen what the first four buttons look like horizontally? I wonder if allow on one line and disallow on another line is easier to read
<leonardr> sinzui: i can show it to you and you can decide. i think separate lines work better
<salgado> yeah, I was thinking about that: [Permanently], [For One Hour], [For One Day], [For One Week]\n or [Do not Allow...]
<sinzui> leonardr, I need to pickup my children.
<leonardr> ok
<leonardr> sinzui, salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703651/+files/Screenshot-2.png
<_mup_> Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436>
<salgado> leonardr, maybe with the commas and the "or" before the [Do not Allow]  button, as I suggested?
<salgado> leonardr, maybe also placing the "accept" buttons on the same line as the sentence?
<leonardr> salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703661/+files/Screenshot-3.png
<_mup_> Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436>
<leonardr> looks weird to me
<salgado> leonardr, indeed.  can you try just on the same line as the sentence, without commas?
<leonardr> salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703663/+files/Screenshot-4.png
<_mup_> Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436>
<salgado> leonardr, I think that looks nicer; just need some more vertical space between the buttons
<leonardr> salgado: how can i do that? even <br> isn't helping
<leonardr> ok, got it
<leonardr> however, you should know that the "For One Week" button is wrapping around to the next line
<leonardr> is that ok with you?
<salgado> oh, in that case it's probably better to leave them all on a separate line
<salgado> is it wrapping after your change?  maybe we can do something else that adds the v spacing but don't cause the wrapping
<leonardr> salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703675/+files/Screenshot-5.png
<_mup_> Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436>
<leonardr> salgado: even if we do something fancy, a long computer name like "Salgado's Computer" might make it wrap
<salgado> good point; it doesn't even need to be that long
<salgado> I like the -5 screenshot best, I think
<leonardr> ok
<leonardr> i will run that through ec2 and land tomorrow
<salgado> leonardr, cool, but I'm still not a graduated UI reviewer, so sinzui must give another thumbs up
<leonardr> ok, put a comment in the review and ask sinzui to weigh in when he comes back
<sinzui> leonardr, salgadoI like ...-5 too
<sinzui> leonardr, If I was mocking this page in text I would have typed...
<leonardr> sinzui: i pushed my revisions to achieve screenshot #5
<sinzui> leonardr, http://pastebin.ubuntu.com/517028/
<sinzui> leonardr, I would have used "or" to introduce the other kind of decision
<bdmurray> EdwinGrubbs: the configure-bugtracker link does appear on launchpad.net
<bdmurray> EdwinGrubbs: in the "Configuration Progress" portlet area - https://staging.launchpad.net/bughelper
<salgado> sinzui, as in http://launchpadlibrarian.net/57938554/Screenshot-3.png?
<sinzui> leonardr, also, check capitalisation. Button are title case so i think "It" and "Not" are preferred
<sinzui> salgado, I see. I do think or on the same line as the button looks odd
<sinzui> Trying to make the options into a sentence look odd in itself
<sinzui> 5 is definitely easier to read than 3
<leonardr> sinzui: "it" is an article. i think https://dev.launchpad.net/UserInterfaceWording is wrong in saying you have to capitalize the last word (i think it means the first word?) but i'd just as soon capitalize everything
<leonardr> but /me is far beyond the point of caring and will do whatever will get this thing approved
<sinzui> leonardr, I think it is a pronoun
<leonardr> yeah, you're right
<leonardr> sinzui, salgado: http://launchpadlibrarian.net/57940682/Screenshot-6.png
<salgado> looks good to me
<leonardr> actually, /me no longer clear why "Allow foo to Access my Launchpad Account" is the case it is
<sinzui> I like 6 best, but I do not want to fight anyone over it
<salgado> leonardr, it shouldn't
<salgado> that's from when it was a button
<salgado> "Allos foo to access my Launchpad account:"
<sinzui> salgado, ? 'Until I Disable "foo"'
<salgado> sinzui, the capitalization of the sentence preceding the buttons is wrong.  it should be "Allow foo to access my Launchpad account:"
<sinzui> okay
<sinzui> salgado, leonardr which layout do you like best, 5 or 6
<salgado> I prefer 6
<leonardr> either is fine w/me
<EdwinGrubbs> bdmurray: but the configure-bugtracker link only shows up on the lp.net/$project page if the user has launchpad.Edit permission. It does not show up if the user has launchpad.BugSupervisor permission.
<sinzui> leonardr, salgado, okay 6 is the layout. Thanks you both for taking time to do this extra work.
<bdmurray> EdwinGrubbs: ah, okay then
<leonardr> sinzui, salgado, i have pushed final version
<sinzui> leonardr, I approved your branch. land away
<leonardr> ok
<leonardr> thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> back
<lifeless> EdwinGrubbs: so that branch is entirely mechanical
<lifeless> EdwinGrubbs: I'd hope you don't need to spend much time on it
<EdwinGrubbs> lifeless: I just started on it a little while ago.
<lifeless> kk
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-10-21
<lifeless> anyone reviewing?
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/39011
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/39011] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> oh bugger, prereq branch
<lifeless> right, thats better
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/39013
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/39013] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/39013 ,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> lifeless: out of pure, unenlightened self-interest I'm grabbing yours off the queue and reviewing it.
<lifeless> why thank you
<StevenK> allenap: O hai, aren't you OCR today?
<allenap> StevenK: Oh, yes I am!
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> allenap: yay, you have an empty queue again!  Unfortunately I didn't get around to Ian's branch last night, but I took Robert's kiloline branch off the queue today to make up for it.
<allenap> jtv: Woohoo, thank you :)
<jtv> allenap: got one for ya!  Simple API branch.  https://code.launchpad.net/~jtv/launchpad/bug-664327/+merge/39036
<allenap> jtv: Cool. I'm doing wallyworld's branch right now, but I'll do yours straight after.
<jtv> great, thanks
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: wallyworkd || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: wallyworld || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> beat me to it :)
<gmb> allenap: Got another one for you if you've time: https://code.edge.launchpad.net/~gmb/launchpad/include-bnl-bug-651108/+merge/39047
<allenap> gmb: Yeah, I've got time.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: wallyworld || queue: [jtv, gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: Thanks: https://code.edge.launchpad.net/~gmb/launchpad/include-bnl-bug-651108/+merge/39047, when it comes round.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: wallyworld || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> jtv: wallyworld's review is taking a long time so I did yours quickly.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> rockstar: Would you mind doing a js review of https://code.edge.launchpad.net/~wallyworld/launchpad/improved-broken-link-handling/+merge/37095? I've done one already, but my javascript is a bit rusty, especially in the context of Launchapd.
<allenap> gmb: I'm probably not going to get your branch reviewed until much later this evening. Is that okay?
<allenap> gmb: There are conflicts in tales-cache.txt.
<gmb> Aaa
<gmb> allenap: Please ignore them. I've already fixed them once, not sure why they'd show up again. I'll look into it but that's more to do with mars' branch than mine.
<allenap> gmb: Okay.
<gmb> Ta
* allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado or sinzui, maybe one of you wants to review my very small branch https://code.edge.launchpad.net/~leonardr/wadllib/package-to-include-sample-data/+merge/39072 ?
<lifeless> leonardr: why is that branch private?
<leonardr> lifeless: probably a wadllib setting
<lifeless> anyhow it looks fine to me
<leonardr> lifeless et al: it is a wadllib setting, and i don't see where to change it. (branches off of wadllib are private by default)
<lifeless> branch policy then - ask a losa to change it
<leonardr> ok
<abentley> rockstar: around?
<rockstar> abentley, about to head out to lunch
<abentley> rockstar: okay, ping me later.
<rockstar> abentley, ack
<rockstar> abentley, heya.
<abentley> rockstar: hi.  OTP
<rockstar> abentley, okay.
#launchpad-reviews 2010-10-22
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> good <localtimeofday> everyone
<mwhudson> hi bac
<bac> hey mwhudson
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: On call: bac || Reviewing: gmb || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> bac: Hai, are you still reviewing, or is the topic a bit stale?
<lifeless> StevenK: what do you need reviewed
<StevenK> lifeless: Small 42 line branch: https://code.edge.launchpad.net/~stevenk/launchpad/reject-mail-ppa-name/+merge/39119
<lifeless> done
<bac> StevenK: i am reviewing
<bac> StevenK: but i am about to be lunching
<bac> oops, didn't read far enough.
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* lifeless changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> adeuring: i leave the reviewing to your capable hands
<bac> i did clear out +activereviews for you
<adeuring> bac: ok, have a nice weekend!
<bac> you too
<EdwinGrubbs> sinzui: besides the losas, who should review a change to qastaging-penid-config? https://code.edge.launchpad.net/~edwin-grubbs/lp-production-configs/qastaging-openid-config/+merge/39153
<sinzui> I do not think anyone else needs to
<sinzui> EdwinGrubbs, why use staging's login? It is a test server. If we use the production one, it will be more reliable. And new users can also login
<EdwinGrubbs> sinzui: No good reason.
<sinzui> gary_poster, can you foresee cases where qastaging has to use ISD's staging SSO server
<gary_poster> qastaging is not something I've had any involvement with other than the original discussions, but thinking...
<gary_poster> ...yes...
<gary_poster> maybe
<gary_poster> description:
<gary_poster> we want a system (staging or qastaging, not sure for this brave new world) that people can use to QA/test their webservice scripts
<gary_poster> the one thing that we can possibly enable in the future on staging-type machines that is not OK on production machines is creating users with escalated privileges
<gary_poster> for tests
<gary_poster> we would want those users to be discarded
<gary_poster> If we implemented it that way
<gary_poster> So, that's scifi
<gary_poster> In the abstract, I'd use the staging openid
<gary_poster> because of that use case and maybe others like it
<gary_poster> but if the staging openid server has been problematic that's a fair argument
<gary_poster> sinzui ^^^
<sinzui> gary_poster, I think that system is staging. qastaging is fast. SSO staging is not. if SSO staging is broken for a week, Launchpads QA is also broken
<gary_poster> sinzui: I'm good with that as an initial setting
<sinzui> oh, I guess I really do want to use production SSO with QA staging. We need to ensure our changes work in production
<gary_poster> I thought that's what you were sating anyway
<gary_poster> saying
<sinzui> gary_poster, I often have to write and speak to understand myself
<gary_poster> ah ok, understood.  I sometimes need the same
<EdwinGrubbs> sinzui: ok, I'll change it to use login.launchpad.net.
<sinzui> thanks
<gmb> adeuring: Do you have time to take a look at https://code.edge.launchpad.net/~gmb/launchpad/make-bnl-descriptions-readable-bug-664566/+merge/39158 for me?
<gmb> It's not a huge branch.
<adeuring> gmb: sure
<gmb> adeuring: Thanks.
<adeuring> gmb: what about letting the display text for BugNotificationLevel.COMMENTS start with  "Any change..." too? (instead of "a change")
<EdwinGrubbs> sinzui: can you approve this branch? https://code.edge.launchpad.net/~edwin-grubbs/lp-production-configs/qastaging-openid-config/+merge/39153
<sinzui> Edwin, I cannot. I guess only a losa can.
<EdwinGrubbs> sinzui: I added you as a reviewer just now. I will also get a losa to review it. It seems like all the other changes on that project had two reviews like that.
<sinzui> There is a bug!
<sinzui> I was able to give an approval via a comment, but I could not claim a  review an comment
<adeuring> gmb: I think @cachedproperty would be better for _bug_notification_level_field. I understand that it is used just once, but just in case it is used later somewhere else, we can/should use the same Choice instance
<gmb> adeuring: Okay, sure.
<adeuring> gmb: thanks, r=me.
<gmb> adeuring: Thanks!
* adeuring changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
