#launchpad-reviews 2009-09-21
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: [] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: [] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
<intellectronica> hey henninge
<intellectronica> wie leuft's?
<henninge> Hallo intellectronica
<henninge> es ist ruhig
<henninge> ;)
<intellectronica> can i ask you to review a very small branch for me? a fix for the bugs page redesign which i hope to get r-c for?
<intellectronica> henninge: ^^^
<henninge> sure
<intellectronica> henninge: https://code.edge.launchpad.net/~intellectronica/launchpad/hot-bugs-summary/+merge/12150
<intellectronica> henninge: http://pastebin.ubuntu.com/275130/ if you don't want to wait for lp to generate the diff
<henninge> intellectronica: line 13 of the diff: s/display/displayed/
<henninge> intellectronica: other than that: r=me ;-)
<intellectronica> henninge: thanks!
<wgrant> Huh. Does the new project bugs page show the bugtask ID?
<wgrant> Er, wrong chan.
<henninge> intellectronica: I have a branch, too ;-)
<henninge> it's a bit longer than yours, though...
<henninge> intellectronica: would you please be so kind? :)
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-433824/+merge/12152
<intellectronica> henninge: you don't scare me
<henninge> lol
<intellectronica> henninge: what's with the extra linebreak in the docstring of initialize?
<henninge> just saw that myself ...
<henninge> intellectronica: I had meant to write a bit more in the docstring ...
<henninge> forgot.
<intellectronica> henninge: so where does self.featured_projects come from, now that you've removed it?
<intellectronica> is it in the superclass?
<henninge> intellectronica: no, it's initialized in initialize
<intellectronica> ah right, i can see now
<intellectronica> henninge: space between operator and operands
<henninge> right
<intellectronica> so you've added assume_date just for testing?
<cprov> henninge: hi, can you review a trivial UI fix (RC) ?
<henninge> cprov: yes, sure!
<henninge> trivial is always good ...
<cprov> henninge: https://code.edge.launchpad.net/~cprov/launchpad/bug-432979-copy-archives/+merge/12136
<cprov> henninge: thank you.
<intellectronica> henninge: could you not avoid that by creating data on the fly using the object factory, instead of relying on sample data?
<henninge> intellectronica: the view uses time.gmtime() to select a project from the list. So the test depends on what day it is run.
<henninge> intellectronica: *would depend*
<intellectronica> right, but if you generated the data in the test, then that wouldn't matter, no?
<intellectronica> henninge: ^^^
<henninge> intellectronica: I don't see the difference. I am not checking and date in the test data.
<henninge> s/and/any/
<intellectronica> henninge: right, but you've added quite a lot of code just for testing. i'm trying to understand if we could do without these additions
<henninge> intellectronica: the only way I see it could be done is doing the same selection in the test as the view does and compare the output programmatically.
<henninge> intellectronica: but that would mean parsing the output of the page test into a list ...
<henninge> intellectronica: "a lot of code"?
<henninge> intellectronica: I see three lines ...
<intellectronica> yes, but if you could do without them, would it not be preferable?
<intellectronica> b.t.w if you leave it there, at least add a comment explaining that it's just for testing
<henninge> I just don't see how without making the test much more complicated and not readable.
<henninge> It could be more easily tested in  unit test for the view, I guess.
<henninge> But that would still make the current page test meaningless.
<henninge> at least for the list.
<intellectronica> henninge: tbh i think it would be better to even just remove that test
<henninge> ok ...
<henninge> intellectronica: there should still be a test for the admin interface, though...
<intellectronica> henninge: i mean just the part that uses assume_date
<henninge> intellectronica: btw, having the assume_date parameter might also serve other purposes ...
<intellectronica> yeah, what purposes?
<intellectronica> time travel? :)
<henninge> henninge: kind of
<henninge> intellectronica: 2
<henninge> intellectronica: ^
<henninge> me and my keyboard ...
<henninge> intellectronica: Well, if you ever want to demonstrate or check how a certain project looks when it is featured, or show it to the maintainers etc...
<henninge> length of summary, logo ...
<allenap> intellectronica: Fancy reviewing a fix for the iharness breakage? https://code.edge.launchpad.net/~allenap/launchpad/fix-iharness-bug-432665/+merge/12154
<intellectronica> henninge: it would be better to just be able to pass the project, then, instead of the date
<intellectronica> allenap: sure. but i can't test it - i don't use ipython
<intellectronica> henninge: so, basically, i think we're in disagreement :) i really don't want that parameter and you really don't want to change it
<henninge> intellectronica: only because I don't see a way around it ...
<henninge> but yes, agree to differ ...
<henninge> intellectronica: is passing the project name a compromise, then?
<intellectronica> henninge: just don't do the test. i rather it landed without that particular test than with that specialized code for pretending that the date is different in a particular context
<henninge> intellectronica: Could I check in the view if it is running in a test?
<intellectronica> henninge: i don't understand
<allenap> intellectronica: Okay, I'll ask someone who does. Thanks anyway.
<henninge> intellectronica: Those were two suggestions, not related ... ;-)
<intellectronica> allenap: i can still review it, just wanted to point that i won't know what to do with it
<henninge> intellectronica: for the latter I think of somthing like:
<henninge> if running_in_test: Always pick first project.
<allenap> intellectronica: Heh, actually it doesn't matter about using it or not. The changes are to buildout.cfg, so you need to sanity check those.
<henninge> else: pick by date.
<intellectronica> allenap: r=me
<allenap> intellectronica: Thanks :)
<intellectronica> henninge: i already like that better, but how will you know if you're running in a test?
<henninge> intellectronica: yes, that's what I am wondering, too. What's the host name in the test, is it .dev, too?
 * henninge tries
<henninge> oh, it is. I just remembered ...
<intellectronica> henninge: you know what you should do? move the code that selects the project into a method, and monkey-patch that method in your test
<henninge> intellectronica: oh, is that possible from a page test? I'd have to patch the class, not the instance, I guess. But it could work.
<intellectronica> henninge: why not from a pagetests? and why do you have to patch the class and not the instance?
<henninge> intellectronica: how do I get the view instance, then?
<intellectronica> ah good point
<intellectronica> that would work if you created your own view
<intellectronica> but not in a pagetest
<intellectronica> still, patching the class might work
<cprov> henninge: any problem with my review ? 
<henninge_> intellectronica: It Works. ;)
<intellectronica> yay!
<henninge_> intellectronica: http://paste.ubuntu.com/275184/
<intellectronica> henninge_: that's great, just add a docstring to that method (explaining that it's there so that you can override it) and r=me
<henninge> intellectronica: thanks a lot! Great idea, btw ;)
<intellectronica> i'm like that, good with monkeys
<henninge> ;-)
 * henninge lunches now
<noodles775> salgado: if you've a minute, can I get you to take a look at the patch that I've attached to bug 433852?
<mup> Bug #433852: Breadcrumbs display for IHasMajorHeading views <Launchpad Foundations:In Progress by michael.nelson> <https://launchpad.net/bugs/433852>
<salgado> noodles775, sure, checking now
<noodles775> salgado: thanks!
<salgado> noodles775, what is the definition of a root context?
<noodles775> salgado: As far as I understand, pillars + a few things - I'm not sure why sprints are one of them?
 * noodles775 looks for an email from sinzui
<noodles775> salgado: I think I've written something incorrect in the description of that bug...
<noodles775> salgado: so the issue is nothing to do with IRootContext, but just the fact that the breadcrumbs display when IMajorHeadingView is provided.
<salgado> noodles775, and is that against the rules?
<salgado> or is it something we didn't account for, which is now causing some pages to look ugly?
<noodles775> salgado: yes, barry thought so... (see the first section of the rules)
<noodles775> salgado: and I just checked - ISprint does indeed inherit IRootContext.
<salgado> I see that ISprint inherits from IRootContext, but according to the docstring of IRootContext, ISprint should not be one
<noodles775> salgado: hrm - I'm not sure why/who decided that, but it seems to make sense of the application tabs at https://edge.launchpad.net/sprints/uds-l
<noodles775> ie, being able to switch between blueprints/overview.
<salgado> I don't see what you mean, sorry
<noodles775> salgado: if you go to that url, you can use the application buttons to switch between the 'blueprints' app (for that sprint) and the 'overview' of the sprint
<salgado> noodles775, right, but is that because sprints provide IRootContext?
<noodles775> salgado: I'm assuming it's *why* sprints provide IRootContext - but I'm far from certain. Perhaps sinzui can clarify later.
<salgado> noodles775, a distro source package, for example, does not provide IRootContext but still has (some of) the application tabs
<noodles775> salgado: yeah, you're right 
<salgado> I think we may want sprints to be IRootContexts, but currently they aren't
<salgado> maybe BjornT knows more about this.  BjornT?
<salgado> do you know why sprints provide IRootContext?
<noodles775> Yep, good idea.
<BjornT> salgado: i would guess because sprints aren't contained in anything
<BjornT> salgado: Product also implements IRootContext. that interface seems to be used to determine which headings and icons to display in the top of the page, but i'm not sure exactly what it's useful for
<noodles775> sinzui: Hi, sorry to jump on you like this, but from memory you've thought about the IRootContext for things like sprints. If you've time, can you take a look at bug 433852?
<mup> Bug #433852: Breadcrumbs display for IHasMajorHeading views <Launchpad Foundations:In Progress by michael.nelson> <https://launchpad.net/bugs/433852>
<noodles775> sinzui: salgado and I were unsure if ISprint is really meant to provide IRootContext (it inherits from it but isn't mentioned in the def of IRootContext).
<salgado> and neither is it served off of the root, as IRootContexts should
<sinzui> noodles775: I think the issue pertains to now verses future.
<sinzui> noodles775: sprints are always shown under sprints, but they do have icons and they are independent of pillars and people. So I think they should not have breadcrumbs. savvy users who read the url know that the sprint is beneath /sprints
<sinzui> salgado: noodles775: sprints are unique also in that in we sould be making a breadcrumb to a collection, not to a pillar/person...that seems extraordinary
 * sinzui back to sprint
<noodles775> ;), thanks.
<allenap> intellectronica: Fancy reviewing a vocab picker performance thing? https://code.edge.launchpad.net/~allenap/launchpad/bug-index-performance-bug-430288-vocab-picker/+merge/12156
<intellectronica> allenap: yes!
<intellectronica> allenap: test?
<allenap> intellectronica: There isn't one so far, afaik.
<intellectronica> oh well, we just have to test it will manually
<intellectronica> allenap: and how is var picker isolated? is it simply enclosed in the use body?
<allenap> intellectronica: Yep, which should be fine even if we start sharing Y instances.
<intellectronica> cool
<intellectronica> there's a lot that can be done to improve this. why are we repeating all that javascript code inlined in a template?!
<intellectronica> but as a performance improvement, this is fantastic. r=me
<intellectronica> allenap: ^^^
<allenap> intellectronica: Yep. I guess this plumbing code seems better to not be in a library.
<allenap> intellectronica: Thanks :)
<noodles775> intellectronica: I've got an incremental to a branch that barry had approved, but I've just realised that he might be sprinting (is the registry team sprinting this week?). If so, do you mind looking at it for me?
<intellectronica> noodles775: sure
<noodles775> intellectronica: thanks - it's the blueprint conversion at: https://code.launchpad.net/~michael.nelson/launchpad/sprint-index-and-attend-3.0/+merge/12044
<intellectronica> noodles775: looks good to me
<noodles775> intellectronica: great - thanks. Now who's available to approve rc's?
<intellectronica> noodles775: i guess francis will be around any minute now
<noodles775> flacoste: Hi! When you're available can you pls check the above MP (converts sprint-index + attend to 3.0) for RC? It's got a fix for a heading rule bug - which I'd ideally like barry to check, but i'm not sure if he'll be around.
<flacoste> morning noodles775
<noodles775> Hi :)
<flacoste> noodles775: you should discuss this with bac i think ;-)
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: [] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
<flacoste> noodles775: but i agree that remaining 3.0 template conversion are r-c material
<noodles775> flacoste: great! bac - can you take a look at the above MP, particularly the last incremental (I was keen for barry to check it, but am not sure if he'll be around - flacoste thinks you might also be a good person to check?)
<flacoste> noodles775: hmm, did you want me to review the thing, or approve the r-c aspect of it?
<flacoste> noodles775: bac is release manager this week, so primary person to contact for r-c
<noodles775> flacoste: it's already reviewed, I was just keen for r-c.
<noodles775> flacoste: oh, sorry!
<bac> noodles775: please add me as a reviewer with 'release-critical' as the review type
<noodles775> bac: Thanks! Done.
<henninge> intellectronica: can you please approve my MP from before lunch? I added the incremental diff.
<henninge> intellectronica: https://code.edge.launchpad.net/~henninge/launchpad/bug-433824/+merge/12152
<intellectronica> henninge: done
<henninge> cheers
<henninge> intellectronica: sorry, but you need to go to your reviewer line on that page to approve it ... ;-)
<henninge> intellectronica: I guess you are usually using the email interface, right? ;-)
<intellectronica> henninge: there you go
<intellectronica> and no, i rarely use the email interface
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: [cprov,-,-] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
<henninge> intellectronica: Thank you! ;) Now bac will have it easier ... ;)
<henninge> cprov: you realize that for an RC you will have to merge into db-devel, don't you?
<henninge> bac: is that still correct? ^
<cprov> henninge: yes, Sunday branches ... sorry 
<cprov> henninge: I will re-base it.
<bac> henninge: according to https://dev.launchpad.net/PolicyAndProcess/ReleaseManagerRotation we only are required to land directly on db-devel on the last day before the rollout
<henninge> cprov: ^
<intellectronica> henninge: devel gets merged regularly into db-devel, so unles it's very late in the game, it should be fine to land in devel
<cprov> okay, that's better, thx guys
<henninge> intellectronica: I was just repeating from memory. I didn't know if that stops when pqm closes.
<henninge> that=stable->db-devel updates
<henninge> cprov: have you tried simply adding the tal:condition to the 'metal:ppa-upload-hint' tag?
<henninge> cprov: I am not sure if that works but would be less noise.
<cprov> henninge: I can try, one sec
<cprov> henninge: <metal:... tal:condition=...> doesn't work, pitty.
<henninge> cprov: ok, thanks for trying.
<henninge> cprov: r=me
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: [-,-,-] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
<bac> barry: when you have a moment could you do a follow-up on https://code.edge.launchpad.net/~michael.nelson/launchpad/sprint-index-and-attend-3.0/+merge/12044
<barry> bac:  sure
<cprov> henninge: thank you.
<bac> henninge, intellectronica, abentley: please do not mark any MPs that are RC candidates as 'approved'.  leave them in the 'Needs Review' state.
<abentley> bac: Okay.
<henninge> bac: understood
<abentley> bac: Will they have separate RC review requests for flacoste?
<henninge> abentley: bac is doing the rc's
<bac> abentley: i am doing release-critical for this release.  flacoste only as my backup
<henninge> bac: He was scared to hear the answer ... ;-)
<bac> :)
<intellectronica> bac: ok
<bac> henninge: rc=bac.  as i note in the MP, please submit to devel.
<henninge> bac: yes, thank you
<noodles775> bac: did you get a chance to look at my sprint conversion RC? (sorry, had connection issues earlier)
<bac> henninge: please add to https://dev.launchpad.net/CurrentRolloutBlockers also
<bac> cprov: approved
<bac> cprov: please add an entry under 'in progress' on CurrentRolloutBlockers
<cprov> bac: ok
<bac> noodles775: is there a bug open for your work on sprint-index and sprint-attend?
<noodles775> bac: no - I didn't create one - but can do so now.
<bac> noodles775: please.  thanks.
<noodles775> bac: bug 434038
<mup> Bug #434038: convert sprint-index and sprint-attend to 3.0 <Launchpad Blueprints:In Progress by michael.nelson> <https://launchpad.net/bugs/434038>
<cprov> bac: thanks for the RC, playing on ec2
<bac> cprov: cool
* noodles775 changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: [-,-,-] || queue: [noodles] (pqm is release-critical; I am happily reviewing blueprint conversions)
<intellectronica> henninge, abentley: if it's cool with you guys i'd like to take myself off for today. i have a phone call and then only a bit more time left to work
<abentley> intellectronica: okay by me.
<henninge> intellectronica: I can't believe that you are leaving us alone out here with *that* long a queue!
<henninge> ... ;-)
<henninge> intellectronica: sure, enjoy the evening ;)
<intellectronica> cheers
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: [-,-,-] || queue: [noodles] (pqm is release-critical; I am happily reviewing blueprint conversions)
 * henninge notices that there actually is an entry in the queue ...
<henninge> ok, I give up....
<henninge> How do I land a release-critical branch?
<abentley> noodles775: What merge proposal do you want reviewed?
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: [-,-] || queue: [noodles] (pqm is release-critical; I am happily reviewing blueprint conversions)
<noodles775> abentley: sorry - I was waiting for the mp to arrive:
<noodles775> https://code.launchpad.net/~michael.nelson/launchpad/434000-copy-archive-ui/+merge/12164
<abentley> henninge: I believe you need to put rc=kiko/flacoste in the message.
<henninge> The commit_re does not mention that, though ...
<henninge> [(?is)^\\s*(:?\\[testfix\\])?\\[(?:release-critical|rs?=[^\\]]+)\\]\\[ui=(?:.+)\\]]
<abentley> henninge: Okay, it's release-critical, not rc.  Been a while.
<henninge> abentley: but I tried that ...
<abentley> henninge: It looks like the release-critical in that regex doesn't accept an =
<henninge> abentley: I tried these: http://paste.ubuntu.com/275316/
<henninge> abentley: isn't that "release-critical OR rs" followed by a "="?
<henninge> flacoste: Help! ;-)
<abentley> henninge: Yes, that's how it looks.  Though I can't actually get python to compile the re.
<henninge> bac: any clues?
<flacoste> henninge: what is your commit messag?
<henninge> flacoste: tried these http://paste.ubuntu.com/275316/
 * bac looks
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: [-,noodles] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
<bac> henninge: what did the error message say
<bac> henninge: ah, i see it up there
<henninge> bac: that it did not match the commit_re
 * bac remembers we had this problem before...
<bac> does it think we're in testfix?
<BjornT> henninge: this should work: [r=intellectronica][ui=none][release-critical=bac][bug=433824] Implemented foo.
<henninge> BjornT: thanks, I'll try that ...
<abentley> noodles775: Is your change valid yui Grid?
<noodles775> abentley: yui-u (the unit) is only re-positioned if it's inside a grid. So without the yui-gc, it's just a div with a class.
<abentley> noodles775: And we know that it's not itself int a yui-u?
<abentley> s/int a/in a/
<henninge> BjornT: looking good ...
<noodles775> abentley: yep - the yui-gc div there is a child of the main slot.
<abentley> r=me
<abentley> noodles775: Are you wanting an RC for this?
<noodles775> abentley: yes.
<abentley> Okay, I'll leave it at "needs review", and you can request an RC.
<noodles775> Thanks abentley... I'll do that now, so you can mark it approved if you like.
<abentley> noodles775: The person who gives the RC can change the status, or you can, after you get the RC.
<noodles775> ah, gotcha. Thanks.
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: [-,-] || queue: [] (pqm is release-critical; I am happily reviewing blueprint conversions)
<abentley> henninge: Are you the "I" in "I am happily reviewing blueprint conversions"?
<henninge> abentley: yes, that was first thing this morning ... ;-)
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: [-,-] || queue: [] (pqm is release-critical; henninge is happily reviewing blueprint conversions)
<henninge> abentley: but since bac is putting them on the rollout blockers list now, that phrase can go.
* henninge changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: [-,-] || queue: [] (pqm is release-critical)
<abentley> henninge: cool
<henninge> bac, BjornT, abentley: now I get this :( http://paste.ubuntu.com/275331/
<bac> henninge: i'd ask mthaddon in #launchpad-code
<henninge> ah, them losas ...
<bac> noodles775: if the MP is marked approved it doesn't show up in my list of things to do.
<bac> henninge: when you figure out the magic incantation let me know!
<noodles775> bac: so the MP is here: https://code.edge.launchpad.net/~michael.nelson/launchpad/434000-copy-archive-ui/+merge/12164/comments/31501
<noodles775> bac: but it might be best waiting to barry goes over the branch...
<noodles775> bac: (I didn't realise he'd be around today).
<henninge> bac, abentley: "you can't submit to devel - only db-devel"
<noodles775> bac: but in the mean-time, I've got another one ready for RC which is correctly marked as pending :)
<bac> henninge: that's what tom said?
<henninge> yup
<bac> henninge: that's interesting
<henninge> bac: just like I suspected earlier: when pqm closes, commit to devel are closed, too.
<bac> flacoste: ^^
<flacoste> hmm
<flacoste> salgado: why did you land your breadcrumb branch on db-devel?
<salgado> flacoste, I didn't
<flacoste> you didn't, rockstar said you did
<flacoste> and since he was depending on your branch he landed it on db-devel
<flacoste> also
<rockstar> salgado, when I merged your branch in, it was definitely based on db-devel.
<salgado> rockstar, I don't see how that'd have happened
<salgado> I based it on devel and landed it on devel
<salgado> if it was based on db-devel I wouldn't be able to land it on devel
<henninge> rockstar: since you are a code guy I asume that you do know that we *stack* on db-devel for all branches? .. ;)
<rockstar> henninge, that's unrelated though.
<rockstar> salgado, no idea, but when I proposed for merge to abentley, there was obvious db-devel stuffs in there.
<salgado> rockstar, that's really weird.  the revision email of my branch's landing doesn't show anything from db-devel
<rockstar> salgado, hm.  I'll sort it out in about an hour.
<abentley> rockstar: skype?
<rockstar> abentley, skype - it doesn't see you.
<rockstar> abentley, I can't hear/see you.
<noodles775> bac: so barry is happy with all the changes in my first RC request (the one that's already in the approved state) at:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/sprint-index-and-attend-3.0/+merge/12044
<noodles775> bac: thanks again!
<bac> noodles775: just approved the RC on that one
<noodles775> bac: heh, too quick :)
<bac> noodles775: please hold off on landing until we get a decision on the devel/db-devel issue
<noodles775> bac: Will do.
<salgado> henninge, how does http://pastebin.ubuntu.com/275350/ look as a fix for https://bugs.edge.launchpad.net/launchpad-foundations/+bug/433991 ?
<mup> Bug #433991: Reverse breadcrumb titles don't expand zope.i18nmessageid.message.Message strings <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/433991>
<henninge> salgado: are you asking for i18n expertise or for a review?
<salgado> henninge, just a review
<henninge> salgado: I am not sure what the test is showing, especiall using the "mapping" feature of the message.
<henninge> -ly
<henninge> salgado: I don't see any actual translation happening.
<salgado> that's it -- it shows that the mapping is used
<henninge> but that is not translating ...
<bac> noodles775: don't you need a UI review for https://code.edge.launchpad.net/~michael.nelson/launchpad/434000-copy-archive-ui/+merge/12164
<bac> noodles775: or are you going to ui=rs it?
<salgado> henninge, we just happen to use the i18n.translate() method to do the interpolation, if one day we localize LP, things should be translated
<henninge> salgado: ah, I understand
 * henninge reads bug again
<henninge> salgado: the code looks fine to me, r=me
<henninge> salgado: but I will have to understand better how that works ...
<salgado> henninge, there's one thing I forgot to tell you
<henninge> salgado: you *will* need an mp though and add bac as another reviewer of type "release-critical"
<henninge> salgado: yes?
<salgado> henninge, the fmt:pagetitle formatter in webapp/tales.py joins all the breadcrumbs' texts in a single string, but it assumes that they're all unicode objects.  when it does that, we end up with a title containing non-interpolated strings
<salgado> that may help you understand how this works
<henninge> salgado: yes, I always thought _() produces an unicode string ...
<henninge> maybe it can simply be casted?
<salgado> this is what happens when it is casted
<salgado> ''.join() will implicitly cast it, and that's when the bug bites
<henninge> salgado: I just tried that out. But this behviour still does not make sense to me ...
<salgado> henninge, you mean that casting it to unicode doesn't do the interpolation?
<henninge> salgado: yes, like you described.
<henninge> salgado: btw, I find the page_title much too verbose in the first place. It should just be "Ask a question"
<henninge> it will be "Evolution >> Answers >> Ask a question"
<henninge> I mean, should be
<henninge> it *will* be "Evolution >> Answers >> Ask a question about Evolution"
<henninge> which is tmi
<henninge> or worse :
<henninge> "Evolution >> Answers for Evolution >> Ask a question about Evolution"
<henninge> Argh!
<salgado> henninge, agreed, but this is the standard for 3.0
<henninge> No, it is not.
<henninge> salgado: it is a matter of what you put in the page_title property.
<salgado> oh, you mean just the first bit of the actual <title>
<henninge> no need to mention that translations pages are a shining example of that ... ;-)
<henninge> we have "label" which takes the actual heading of the page above the bread crumb and it can be verbose.
<henninge> we have "page_title" which goes into the bread crumb and must be consise, not repeating information found in the breadcrumbs anyway - like the name of the project.
<salgado> henninge, agreed, but this is out of the scope of this branch, I'm afraid.  these are nice improvements but not really RC
<henninge> finally, the <title> is the invers bread crumb be default. I don't have a problem with that ... ;)
<henninge> salgado: oh, I already said r=me, didn't I? :-P
<salgado> good point. ;)
<salgado> henninge, can you say so in https://code.launchpad.net/~salgado/launchpad/bug-433991/+merge/12170 ? :)
<henninge> salgado: done
<salgado> bac, I've got one for you: https://code.edge.launchpad.net/~salgado/launchpad/bug-433991/+merge/12170
<salgado> thanks henninge!
<henninge> abentley: I am moving out ...
* henninge changed the topic of #launchpad-reviews to: on call: abentley || reviewing: [-] || queue: [] (pqm is release-critical)
<noodles775> bac: regarding the ui-review for the copy-archives change - yes, thanks (I was in ui=rs mode from blueprints :) ).
<bac> salgado-lunch: looks
<noodles775> barry: sorry, would you mind doing a really easy ui-review - screenshots there as well as instructions for setting it up locally: https://code.edge.launchpad.net/~michael.nelson/launchpad/434000-copy-archive-ui/+merge/12164
<barry> noodles775: sure, just a few minutes
<noodles775> barry: no rush at all. Thanks!
<barry> abentley: i have a really easy one for ya
<abentley> barry: lay it on me.
<barry> abentley: mp'ing it now, thanks
<barry> abentley: sent.  i requested you so you should see it shortly
<abentley> barry: r=me
<abentley> barry: I assume you'll be getting an RC for this?
<barry> abentley: thanks!  and yep
<barry> bac: ^^
<bac> barry:  i'll look in a sec
<bac> barry: did you fix the lint issue?
<bac> or was it fake?
<barry> bac: i fixed it
<salgado> bac, should my branch go to devel or db-devel?
<bac> barry: cool.  i'll approve it.  on future ones please add me to the MP with 'release-critical' as the review type.
<bac> salgado, barry:  until tomorrow at 2200Z everything goes to devel
<bac> well, unless it is db stuff, which it isn't
<salgado> ok
<barry> abentley: i'll have a 150 line diff for ya in a few minutes if that's cool
<abentley> barry: Sure.
<barry> abentley: mp sent
<abentley> barry: r=me.
<noodles775> Hi barry, do you reckon you'll have time soon to do that quick ui-review? If not, no probs, but if you can I can send it off before I go to bed :)
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/434000-copy-archive-ui/+merge/12164
<barry> noodles775: gah, totally forgot sorry!  will do that now
<barry> abentley: thanks!
<noodles775> Thanks barry!
 * noodles775 should have requested it on the MP - always forget.
<bac> noodles775: did you get my email re: the blueprint conversion
<noodles775> bac: yep, just replied - that'll be great.
<bac> noodles775: thanks a ton!
<noodles775> np :)
<barry> noodles775: what does "copy archive generic-string18 for person-name16" even mean? :)
<bac> mrevell-dinner: ping
<barry> noodles775: so there's no archive description yet on that screenshot?
<noodles775> barry: sorry - yes, I used the factory to create and populate the copy-archive, hence all the generic names.
<noodles775> barry: and yes, I didn't add a description either.
<noodles775> barry: (I used the exact script in the test/demo section of the MP).
<barry> noodles775: cool.  ui=me* it looks great
<noodles775> barry: great, thanks!
<abentley> jtv: ping
<jtv> abentley: pong
<abentley> jtv: wrt your bug-146855, I think it would be a good idea to run the unit tests under the new database user.
<abentley> jtv: I often find I miss necessary permissions if I don't do that.
<jtv> abentley: sounds good... forgive me but it's late and hot here: self.layer.switchDbUser?
<jtv> abentley: I was worrying about the same thing.
<abentley> jtv: That's right.
 * jtv fiddles
<mrevell-dinner> bac: I'm more "mrevell-putting-son-to-bed" at the moment. Gimme twenty mins. Will you still be around?
<abentley> jtv: I think you also missed my question "Is FAILED really a terminal status? It seems like it's possibly not terminal, and that's why you're delaying 30 days"
<jtv> abentley: oh, sorry, I read through everything before starting to answer that one.  Yes, FAILED is a terminal statusâbut an entry can get out of any status by being "refreshed" with a new upload.
<jtv> So I'm not counting that.  :-)
<abentley> Okay, so r=me, modulo switching the db user.
<bac> barry: rc=bac.  remember land on devel
<jtv> abentley: working on that now.  Thanks!
<bac> mrevell-dinner: sure
<abentley> jtv: You'll probably need to commit the transaction before switching the db user.
<barry> bac: thanks
<bac> mrevell-dinner: https://launchpad.dev/+tour/images/community/3.png
<mrevell-dinner> bac: ah thanks, I'll fix that. Handsome devil..
<mrevell-dinner> right, I'm not here  for a few mins
<bac> mrevell-dinner: there's another too
<bac> mrevell-dinner: no worries
<bac> mrevell-dinner: and this one looks a little dated:  https://launchpad.dev/+tour/images/blueprints/3.png
<rockstar> abentley, howzabout a review?  It's something I'm going to get an RC on.
<abentley> rockstar: Sure thing.
<rockstar> abentley, coming up.
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: [-] || queue: [] (pqm is release-critical)
<rockstar> abentley, did my proposal arrive?
<abentley> rockstar: Yeah, about 3 minutes ago.
<abentley> rockstar: r=me.
<rockstar> abentley, hooray.
<rockstar> bac, did you see my rc review request?
<rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/sourcelist-template/+merge/12195
<bac> rockstar: no.  thanks for pinging me abt it
 * bac looks
<rockstar> bac, it's all mechanical, closes off the two templates you and I talked about this morning.
<bac> rockstar: will you be landing this via ec2?
<rockstar> bac, absolutely.  It seems stupid not to.
<bac> alrighty, then
<bac> rc=bac
<rockstar> bac, thanks.
<rockstar> bac, in the pqm regex, is rc= before r= ?
<bac> rockstar: [release-critical=bac][r=abentley]
<bac> rockstar: look at some of the landing messages from today
<mrevell> bac: I've just pushed a new version of the tour images branch. I've made extensive updates to the images and I've removed mentoring and sprints from the text. The first for obvious reasons, the second because it was always a bit of a struggle to justify its presence and even harder now there's not a pretty picture (silly, I know). All of this is pending a major and thorough re-do of the tour this comign cycle.
<mrevell> having said that, I shall be bowing out now. G'night
#launchpad-reviews 2009-09-22
* jtv changed the topic of #launchpad-reviews to: on call: â || reviewing: [] || queue: [jtv] (pqm is release-critical)
<noodles775> henninge: I've just reviewed jtv's work on the specificationtarget-assignments.pt, can you take a look and see if you agree?
<noodles775> https://code.edge.launchpad.net/~jtv/launchpad/mechanical-specificationtarget-assignments/+merge/12203
<noodles775> He'll be needing the same menu/portlet that you're currently doing, so it might be easiest for you just to merge his branch and do the small update to the template to ensure it's consistent.
<noodles775> Then you guys will only need one rc too.
<henninge> jtv: are you back?
<jtv> henninge: yes
<henninge> jtv: did you read noodles review?
<jtv> henninge: not yet, no
<henninge> jtv: I am working on the menus for hasspeci...
<jtv> henninge: then michael's suggestion makes sense
<henninge> jtv: he suggests I merge your branch
<henninge> noodles775: still, I cannot get the global actions to work ...
<jtv> henninge: shall I just fix up the things he remarked on, and then ping you so you can merge?
<noodles775> henninge: did you take a look at the mp I linked? If it's still not working push it and I'll take a look.
<henninge> jtv: yes, please
<henninge> noodles775: I did.
<jtv> henninge: I ran mine through EC2, so should be okay.  Feel free to merge now.
<henninge> jtv: ok
<jtv> noodles775: thanks for a understanding and constructive review.  :)
<noodles775> jtv: np!
<allenap> gmb: Are you OCR today? Cause I've got a nice RC candidate for you :)
<gmb> allenap: Yep.
<gmb> Goferit
<allenap> gmb: https://code.edge.launchpad.net/~allenap/launchpad/convert-blueprints-bug-434056/+merge/12210
<allenap> gmb: Thanks.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: [jtv] (pqm is release-critical)
<gmb> jtv: Do you have a branch in the queue or is that reference stale?
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: [] (pqm is release-critical)
<gmb> That answers that question then :)
<jtv> gmb: I have another one, but the reference was for an older one.
<noodles775> gmb: sorry, didn't see your question - just theh topic ;)
<jtv> Sorry, for another one
<gmb> allenap: Can you file a bug for the SpecificationGoalDecideView tests please? We should probably, like, write some.
<gmb> jtv: Understood.
<allenap> gmb: Heh, you professional you.
<gmb> Oh, is that what it is? Hmm.
<gmb> allenap: I hereby grant you Instant Hero status for getting rid of SpecificationFeedbackClearingView.process_form()
<allenap> gmb: Cool. It's so old I didn't know how it worked.
<gmb> allenap: Everything looks fine. r=me. Nice job!
<allenap> gmb: Thanks!
<allenap> gmb: Fwiw, bug 434555 is the SFDV-tests-pls bug.
<mup> Bug #434555: SpecificationGoalDecideView is not tested <Launchpad Blueprints:New> <https://launchpad.net/bugs/434555>
<gmb> Okiedoke.
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: [noodles] (pqm is release-critical)
<noodles775> gmb: I've got a small MP which should hit your inbox any minute, for when you've time.
<gmb> noodles775: Sure, I'll look shortly.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: noodles775 || queue: [] (pqm is release-critical)
<gmb> noodles775: Can you give me a link to the mp? It's not showing up in my inbox.
<noodles775> gmb: https://code.launchpad.net/~michael.nelson/launchpad/missing-distro-ppas-heading/+merge/12211
<gmb> Ta
<gmb> noodles775: Looks good. r=me
<noodles775> gmb: great, thanks.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] (pqm is release-critical)
<bac> noodles775: rc=bac
<noodles775> bac: great, thanks.
<intellectronica> gmb: i've got a branch for you. pretty simple stuff
<gmb> intellectronica: Sure, diff me.
<intellectronica> gmb: https://code.edge.launchpad.net/~intellectronica/launchpad/webkit-ajax-disables/+merge/12215 http://pastebin.ubuntu.com/275790/
<gmb> intellectronica: Looks good. r=me.
<intellectronica> gmb: thanks for a speedy review!
<gmb> intellectronica: I seem to be good at diffs today. I wonder if htat means I'll suck at Python...
<bac> gmb: please leave RC candidate MPs in 'Needs Review' and remind the dev to add a 'release-critical' request so the MP workflow works.
<intellectronica> bac: i just made such a request
<gmb> bac: Okay, duly noted.
<bac> intellectronica: cool
<bac> thanks graham
<gmb> noodles775, allenap ^^ please do the above if you haven't already.
<intellectronica> bac: i'm sure that as a safari user you'll be glad to approve it :)
 * bac looks!
<allenap> gmb: Thanks, done.
<bac> intellectronica: is it our ugly red error box?
<noodles775> gmb: yep, done. Thanks.
<intellectronica> bac: not quite. the ugly red error box is a fairly complicated bug to do with the interaction of webkit and lazr.restful, and i don't feel that i can cover it for r-c. for now i just made the controls usable in webkit by navigating to the boomerang form. i'll ask leonard for help fixing the real bug later on
<intellectronica> bac: there's also a fix to make editing duplicates in webkit work in that branch
<bac> intellectronica: got it
 * gmb -> lunch
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue: [] (pqm is release-critical)
* bigjools changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue: [bigjools] (pqm is release-critical)
<bac> intellectronica: why did you disable the ajax assignee for webkit?  i never had a problem with that
<intellectronica> bac: i thought it has the same problem as the product ajax control. if it doesn't then i can revert that. let me validate
<bac> intellectronica: also, on your branch i went to https://bugs.launchpad.dev/firefox/+bug/5 and tried to make it a duplicate of bug 1
<mup> Bug #1: Microsoft has a majority market share <ubuntu> <Clubdistro:Confirmed> <Computer Science Ubuntu:Confirmed for compscibuntu-bugs> <Ichthux:Confirmed for raphink> <JAK LINUX:Confirmed> <Linux Mint:Won't Fix> <OpenOffice:Confirmed for lh-maviya> <Launchpad Translations:Invalid> <Tabuntu:Confirmed for tinarussell> <Ubuntu:In Progress> <bum (Ubuntu):Invalid> <casper (Ubuntu):Invalid> <djplay (Ubuntu):Invalid> <firefox (Ubuntu):Invalid> <u
<bac> i got an error popup that had an embedded traceback.
<bac> intellectronica: i'm sure that isn't due to your fix but can you try to replicate that?
<intellectronica> bac: you're right about the assignee. i'll revert that change
<bac> intellectronica: cool
<intellectronica> bac: the error you're seeing is an error from the server that you can't mark the bug as a duplicate because it's a duplicate itself. that's a terrible way of showing that error, but fixing that is out of scope for an r-c
<bac> intellectronica: i guess the traceback will either only be shown on lp.dev or only to lp developers on lp.net
<intellectronica> bac: i've reverted the change to the assignee field. see http://pastebin.ubuntu.com/275807/
<bac> intellectronica: cool
<intellectronica> bac: yes, the error itself is just the stuff you see at the top
<bac> intellectronica: can you confirm for me that a normal user on lp.net doesn't get the embedded traceback?  i believe us but i'd like to see proof.
<bac> intellectronica: marking your MP rc=bac
<intellectronica> bac: thanks
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bigjools || queue: [] (pqm is release-critical)
<gmb> bigjools-lunch: You might want to check that you don't have some massive collisions with allenap's blueprint template conversions.
<allenap> gmb: We've had words :)
<gmb> Ah, okay.
<gmb> As long as it not go boom on merge.
<intellectronica> gmb: can you review another tiny branch?
<gmb> intellectronica: Sure, stick it on the queue and I'll take a look when I've finished with bigjools-lunch's branch
<intellectronica> let me see if anyone else can do it earlier. it's only 36 lines with context
<intellectronica> http://pastebin.ubuntu.com/275836/
<intellectronica> allenap: maybe you can take a look? ^^^
<allenap> intellectronica: Sure.
<intellectronica> thanks
<allenap> intellectronica: What does it do?
<intellectronica> allenap: https://code.edge.launchpad.net/~intellectronica/launchpad/tags-cloud-improvement/+merge/12220
<intellectronica> allenap: it makes it so that the tags portlet only shows official tags and the ten most popular unofficial ones, like it used to be
<intellectronica> allenap: so, what do you think?
<gmb> bigjools-lunch: Your branch looks good. Please file an rc request if this is an RC candidate. r=me.
<allenap> intellectronica: I was just finishing off an email, looking now.
<bigjools> gmb: ta very much
<bigjools> gmb: the only gotcha is that I know it will conflict with allenap's branch, but the changes are tiny, I don't think they need re-reviewing.
<intellectronica> allenap: any luck with my patch? it's tiny
<allenap> intellectronica: I know, but I have some questions about it. I'm writing a very quick review.
<intellectronica> why don't you just ask me the questions?
<intellectronica> wow, half hour for a review of one little display method
<intellectronica> allenap: i'm going to get something to eat. hopefully when i'm back you can let me know whether my patch is ok and if not what questions you have about it?
<bac> allenap: 1 rc, 1 not rc.  thanks for the work on both.
<allenap> bac: Thanks, cool
<gmb> bigjools: Righto; if it's just for conflict resolution purposes I'm not that concerned.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  || queue: [] (pqm is release-critical)
<bac> bigjools: rc approved with one minor fix
<bac> bigjools: and one optional fix
<bigjools> bac: ok - you are thorough :)
<bigjools> allenap: do you have a special commit message you want me to use when I land your branch?
<bigjools> you got it rc-ed right?
<allenap> bigjools: Yes. "I am allenap's bitch".
<bigjools> :)
<allenap> bigjools: That's a no :)
 * bigjools goes prone
<allenap> bigjools: Thanks!
<cprov> gmb: I have an RC candidate, can you review it ?
<intellectronica> allenap: thanks for the review. i'll change the name of the array of dicts to indicate what it contains. i'm not concerned about performance in this case and there's no point using a generator expression instead of a list if you're going to iterate over it immediately
* cprov changed the topic of #launchpad-reviews to: on call: gmb || reviewing:  || queue: [cprov] (pqm is release-critical)
<allenap> intellectronica: Okay, cool.
<gmb> cprov: Sure
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: cprov || queue: [] (pqm is release-critical)
<gmb> cprov: Is there an MP?
<cprov> gmb: thx, https://code.edge.launchpad.net/~cprov/launchpad/soyuz-missing-heading/+merge/12226
 * gmb looks
<cprov> gmb: I have to go afk for a bit, please add your comments to the MP I will address then when I'm back
<gmb> cprov: Ok.
<cprov> gmb: the branch is already playing on ec2,  just in case it breaks other tests.
<bac> cprov: please add your r-c request to the MP before you go
<cprov> bac: ehe, already done ;)
<bac> sweet
<intellectronica> bac: hi, i've asked for https://code.edge.launchpad.net/~intellectronica/launchpad/tags-cloud-improvement/+merge/12220 to be considered for release-critical. it's not really a blocker but it's a very simple fix and we'd really like to have it as part of 3.0
<cprov> bac: it's a little big for a RC, but let me know what you think
<bac> intellectronica: ok, i'll look
<bac> deryck: in your new milestone portlet the counts are not right-aligned.   looking at https://bugs.launchpad.dev/debian/+bugs the counts for the three different portlets are all over the place, though internally consistent.  ideas?
<deryck> bac, on phone, just a moment
<deryck> bac, sorry
<bac> np
<bac> intellectronica: rc=bac, with mods
<intellectronica> bac: thanks
<bac> intellectronica: that's some action-packed code in a few lines!
<intellectronica> heh, yes, it's easy to do long expressions with list comprehensions
<gmb> cprov-afk: Your branch looks good to land. r=me.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] (pqm is release-critical)
<bac> henninge: i am going to disapprove the r-c for jtv's MP since you will be doing a combined landing.  i like his change but an RC is not appropriate.
<henninge> bac: ok
<henninge> bac: I am currently struggling with some stray changes in my branch.
<henninge> dunno were they came from. they are in answers.
<bac> noodles775: will you mark your review of https://code.edge.launchpad.net/~jtv/launchpad/mechanical-specificationtarget-assignments/+merge/12203 as approved per jtv's changes, if you agree.
<noodles775> bac: Done.
<bac> noodles775: thanks
<bac> thanks cprov-afk
<deryck> bac, concerning the alignment issue, the problem is because the portlets use table cells, and the filters have long word to push the number cell far to the right.
<deryck> bac, the fix requires either making the number cell a fixed width, which I don't particularly like, or converting to not use tables and float the number, which is a lot of templates to touch.
<deryck> bac, but this is a problem regardless of my fix to add a milestone links portlet.
<bac> deryck: thanks for the explanation.  we can fix them together later
<deryck> bac, ok.  so rc on this branch is ok?  Or wait for it to land?
<bac> deryck: rc=bac
<deryck> bac, awesome,thanks!
<mrevell> gmb: Fancy a review? https://code.edge.launchpad.net/~matthew.revell/launchpad/3.0-tour-images/+merge/12174
<gmb> mrevell: Sure, I'll take a look in five minutes or so.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: mrevell || queue: [] (pqm is release-critical)
<gmb> mrevell: Looks fine to me... I see that bac has already r-c'd this, so go ahead and land it :)
<mrevell> thanks gmb
<cprov-afk> gmb, bac:  thanks for your review
<bac> cprov-afk: np, thanks for the branch
<allenap> gmb: Are you accepting new reviews? Specifically, reviews for external bug tracker fixes? Like https://code.edge.launchpad.net/~allenap/launchpad/external-bugzilla-3.4-api-bug-434580/+merge/12230
<gmb> allenap: I think I might :0
<gmb> :) even.
 * gmb waits for the diff to generate.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: [] (pqm is release-critical)
* henninge changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: [henninge] (pqm is release-critical)
<henninge> gmb: I appended my branch, too, but maybe I can get noodles775 to do it.
<henninge> noodles775: do you have time to review my branch, seeing that you were involved earlier?
<henninge> bac: my branch is up for review now. Sorry, that took much longer.
<henninge> bac: I had to clean up some mess in the branch. I will eventually submit through ec2 because now I am not sure *what* I was testing there ... :(
<bac> henninge: ok.  i'll look now
<intellectronica> hey henninge, want to review a tiny fix i want to r-c while you wait?
<henninge> intellectronica: like a snack? sure!
<bac> henninge: you need to get a code review first
<gmb> allenap: that looks perfic. r=me. Thanks!
<bac> henninge: ah, i see you're in the queue
<henninge> bac: I know, sorry to have been unclear ...
<allenap> gmb: Splendid, thanks!
<gmb> allenap: Please to be doing the rc dance :)
<henninge> gmb: that was quick ;) looks like noodles775 is gone ...
<intellectronica> henninge: lovely. http://pastebin.ubuntu.com/275928/ fixes https://bugs.edge.launchpad.net/malone/+bug/434115 i'll create an mp for you now
<mup> Bug #434115: ubuntu/series/+source/package/+filebug?no-redirect redirects any way <Launchpad Bugs:In Progress by intellectronica> <https://launchpad.net/bugs/434115>
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: henninge || queue: [] (pqm is release-critical)
<gmb> henninge: Looking at yours now.
<henninge> gmb: thanks a lot!
<gmb> henninge: Or I would... can you give me the URL of the MP please? (Yes, that's lazy of me)
<henninge> gmb: https://code.edge.launchpad.net/~henninge/launchpad/bug-434055-combined/+merge/12229
<gmb> Ta
<gmb> henninge: Don't the methods of HasSpecificationsMenuMixin need to be properties? (That's usually the case for no-arg methods called by templates).
<gmb> henninge: Also, how much effort would it be to PEP8ify their names, e.g.: list_all, list_accepted, etc?
<intellectronica> henninge: and here's the mp: https://code.edge.launchpad.net/~intellectronica/launchpad/distroseries-source-package-filebug-no-redirect/+merge/12232
<intellectronica> henninge: is the patch ok?
<gmb> henninge: Otherwise the branch looks good. I'm not terribly worried about the PEP8 thing but I think that those methods should be properties.
<henninge> gmb: let me look for those properties ...
<henninge> gmb: no, not properties, they are methods (at least on translation NavigationMenus)
<henninge> gmb: changing the names now might break more tests. I have seen it when I changed 'addspec' to 'new' ... So I'd rather not.
<gmb> henninge: Okay.
<henninge> intellectronica: r=me
<henninge> gmb: thanks
<gmb> henninge: They are properties on other NavigationMenus
<gmb> I think for consistency they should be here, too.
<gmb> (Since anything that the template uses should be a property or an attribute, not a no-arg method)
<intellectronica> henninge: thanks a bunch
<gmb> henninge: So please add the @properties. Leave the renaming though.
<gmb> henninge: r=me with that change.
<intellectronica> bac: i've requested your consideration of https://code.edge.launchpad.net/~intellectronica/launchpad/distroseries-source-package-filebug-no-redirect/+merge/12232 for release-critical. it's a blocker for us
<bac> intellectronica: no test?
<intellectronica> bac: i guess i can try and add a test, but i'm not really sure how. i don't think it will work with the test browser, so i have to find out how to test it
<henninge> gmb: I am sorry, but I have been doing some greping here and I cannot verify that statement. There are *some* methods marked as property but the bulk of them is not. or so it seems.
<henninge> gmb: so are they all wrong? Just say it, I'll believe you .. ;-)
<gmb> henninge: Hmm. Actually, you're right. I've mistaken a @property for a link in a NavigationMenu.
<bac> intellectronica: can you puzzle over it for a timeboxed period and let me know?
<gmb> henninge: So leave them as they are.. we can take this to the reviewers list. It's not relevant for RC>
<intellectronica> bac: sure
<henninge> gmb: ok, thanks. Sorry for being so stubborn ... ;)
<gmb> henninge: Stubborn is good. It's how we make everyone else learn :).
<henninge> ;-)
 * gmb goes off-call
* gmb changed the topic of #launchpad-reviews to: on call: - || reviewing:  || queue: [] (pqm is release-critical)
<henninge> bac: code-review is done ;-)
<henninge> bac: https://code.edge.launchpad.net/~henninge/launchpad/bug-434055-combined/+merge/12229
<bac> ok
<mrevell> Anyone care to review a help update? https://code.edge.launchpad.net/~matthew.revell/launchpad/home-page-staging-popup-help/+merge/12233
<henninge> mrevell: me! me! me!
 * henninge waits for diff to see how much it is ...
<henninge> ;)
<mrevell> :)
<intellectronica> bac: turns out there's no problem whatsoever doing the test you requested with the test browser. see http://pastebin.ubuntu.com/275955/
<mrevell> henninge: the diff's there. Want to do it? :)
<bac> intellectronica: excellent
<intellectronica> bac: are you happy for this branch to land for r-c with the addition of the patch?
<intellectronica> of the test, that is
<henninge> mrevell: sorry, distracted. looking now.
<mrevell> danke
<henninge> mrevell: is href="+help..." ok? or shouldn't it be href="/+help..." ?
<mrevell> henninge: Yes, probably. I'll fix that.
<henninge> mrevell: you are saying "What is staging?" but never mention the term before that.
<henninge> Maybe it should be  "... in our staging environment. (What is staging?)" ?
<mrevell> henninge: Hmm, yes. I misread "sandbox" for staging. Will fix that too.
<allenap> beuno: Can you review a download icon fix? You've done some work on this previously so you've got context. https://code.edge.launchpad.net/~allenap/launchpad/remove-too-many-download-icons-bug-423105/+merge/12237
<henninge> mrevell: don't we do tests for help pop-ups?
<mrevell> henninge: Not AFAIK
<henninge> mrevell: ok ... ;)
<henninge> mrevell: r=me with the discussed changes  ;-)
<bac> intellectronica: yes.  i'll update the MP
<mrevell> henninge: Does that mean you'd like to see a test? :)
<beuno> allenap, yes
<henninge> mrevell: having a test is always better but if we don't have examples of how to test that, I think it's too much work atm.
<mrevell> henninge: Okay, thanks.
<henninge> mrevell: maybe file a bug like "Help pop-ups should have tests." ?
<mrevell> henninge: Okay, will do
<beuno> allenap, approved. I know I can't *really* approve code, but, well, you know.
<allenap> beuno: Oh, go on :) Thanks!
<jml> allenap, I've done a code review.
<allenap> Thanks jml!
<mrevell> bac: I've requested your review but it wouldn't let me add "release-critical" ... am gonna see if it's a bug. It's for the "sandbox" help change on the front page. https://code.edge.launchpad.net/~matthew.revell/launchpad/home-page-staging-popup-help/+merge/12233
<bac> mrevell: thanks
<salgado> https://code.launchpad.net/~salgado/launchpad/bug-434349/+merge/12239
<salgado> sinzui, do you have time to review ^ or should I try to find somebody else?  it's rather trivial
 * sinzui looks
<sinzui> salgado: r=me, no changes
<salgado> thanks sinzui 
<bac> henninge: rc=bac
<henninge> bac: cheers
<henninge> just in time for me ... ;-)
<bac> henninge: thanks for hanging around
<henninge> bac: np, you have your hands full, I can see that. ;)
<bac> hi mrevell-dinner
<bac> mrevell-dinner: rc=bac
<bac> allenap: thanks for waiting around.  rc=bac * 2
 * bac grabbing some lunch.  RCs to flacoste for the next bit
<gary_poster> bac, I have a branch that does not fix blueprints but fixes the other bits AFAIK: answers, bugs, code.  tests matching the pattern '[bB]read[cC]rumb' pass :-) .  ec2 is running tests now.  Can you review, or should you/I try to rustle up someone else?  https://code.edge.launchpad.net/~gary/launchpad/breadcrumbs/+merge/12246
<bac> gary_poster: i'll do it
<gary_poster> bac, thank you.  the diff is a bit longer than it needs to be because my editor decided to do some whitespace cleanup in the zcml
<bac> gary_poster: good for your editor
<gary_poster> :-)
<bac> gary_poster: it's a very pretty diff
<gary_poster> bac: a bit early for christmas, but red and green stripes are nicely complementary
<bac> gary_poster: i understand why they had HasBranchesBreadcrumb.  you think standardizing on the rootsite name is better?
<gary_poster> bac: I didn't understand and thought it was simply irregular.  Having regular names certainly helps with mass changes like this.  Why did they have the old name?  And/or, if you simply want me to change it back, just say so. 
<bac> gary_poster: no, the fix is more standard.  they could argue it is for IHasBranches so the old name is better
<bac> gary_poster: approved as is.  if you're going through ec2 you'd best target it to db-devel
<gary_poster> bac: thank you.  oh, good point.  /me CTRL-C's the current run...
<bac> gary_poster: check to see that instance is killed!  ctl-c left me some zombies last week that were eating money
<rockstar> Can I get a review from someone?
<gary_poster> bac ack, thanks
<bac> rockstar: yes
<rockstar> bac, on its way.
<bac> rockstar: rc candidate?
<rockstar> bac, yea, it's the one you and thumper talked about last night.
<bac> ok
<gary_poster> bac: are you a ui reviewer too ([ui=bac]) or should I do a [ui=rs] or do something else?
<bac> gary_poster: i am not a ui.  i'd do ui=rs
<gary_poster> ok thanks
<barry> gary_poster: rockstar flacoste abentley salgado anybody available a review of changes for bug 434761?  this is a redesign of the home page requested by beuno
<mup> Bug #434761: Make the home page pretty <story-ui-3> <Launchpad Registry:In Progress by barry> <https://launchpad.net/bugs/434761>
<rockstar> barry, sure.
<barry> rockstar: cool, thanks.  i'll push up an mp for ya straight away
<rockstar> barry, great.
<barry> rockstar: i have a couple of test failures i need to investigate first...
<bac> barry: do let me have a look after the code review.
<barry> bac: oh, definitely
<bac> barry: i was a bit ambigous earlier
<barry> bac: do you want me to just request a release-critical review of the branch from you?
<barry> rockstar: mp sent
<rockstar> barry, ack
<barry> rockstar: gah!  i think i spelled the reviewer request incorrectly
<barry> https://code.edge.launchpad.net/~barry/launchpad/434761-homepage/+merge/12252
<rockstar> barry, yeah, was just about to say I hadn't seen it.
<barry> rockstar: i'm requesting them old skool
<barry> bac: beuno just gave me ui approval
<bac> barry: great.  you know he's been drinking, though.
<barry> :-D
 * rockstar can also UI review
<rockstar> :)
<barry> rockstar: have you been drinking?
<bac> grape nehi
<rockstar> barry, only water, although I'm wondering if booze would give me my voice back.  :)
<bac> rockstar: it's too bad you don't drink beer.  i like that Fat Tire you've got up there
<rockstar> bac, everyone does.  :)  You need a sprint here if you really like beer.  #2 in the nation for microbrews next to Portland.
<bac> nice
<barry> rockstar: unfortunately i only request ui reviews from intoxicateds
<bac> new belgium has *taken over*.  in a year they ramped up massive distribution
<rockstar> Really?  I guess I never noticed since they are so big here.
<rockstar> barry, r=me
<barry> rockstar: thanks.  bac it's up to you now
<bac> barry: land it, dano
<bac> barry: i'd be remiss if i didn't remind you to point ec2 to db-devel
<barry> bac: yes, i will land it through ec2.  do you remember the magic incantation to send it through db-devel?
<bac> barry: check my email to lp-dev
<barry> bac: +1 (been under an email blizzard)
<barry> bac: got it
<bac> barry: props to gary for the magic
<gary_poster> heh
#launchpad-reviews 2009-09-23
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: sinzui || queue: [] (pqm is release-critical)
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: â || queue: [] (pqm is release-critical)
<jtv> hi sinzui, r=me
<sinzui> jtv: thanks.
<jtv> Very nice change.
<jtv> BTW you also had a branch with two db reviews... does it need a code review as well?
<sinzui> jtv: no code
<jtv> ok
<sinzui> I got jml's approval so that edge uses (which most of the UDS participants are) get access to the feature so that Jorge can plan the logistics
<sinzui> So I will add the code next week so that we can have a successful UDS
<noodles775> Good morning jtv. Are you ready for another?
<noodles775> https://code.launchpad.net/~michael.nelson/launchpad/435023-getPubXX-oops-db-devel/+merge/12266
<jtv> noodles775: coming up
<noodles775> Thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: jtv || reviewing: â || queue: [noodles] (pqm is release-critical)
<jtv> noodles775: was answering a question.  Back now.
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: noodles || queue: [] (pqm is release-critical)
<jtv> man, it's cold here
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: noodles, - || queue: [] (pqm is release-critical)
<jtv> I can hear the wind howling and I _know_ it's calm outside.  It's the aircon.
<allenap> Morning jtv :)
<jtv> hi allenap!
<jtv> noodles775: in this error condition you're fixing, what happens currently?
<noodles775> jtv: the webservice 500's with a trace - as shown in the bug.
<noodles775> jtv: (and generates an oops of course)
<jtv> noodles775: are we getting that a lot?  RC for this seems a bit radical
<noodles775> jtv: I wasn't sure - but checked with bigjools - apparently anything that generates an oops is worthy?
<jtv> wow
<bigjools> it's a simple fix and it should go in if it ticks off an oops
<jtv> It certainly is a simple fix, and properly tested.
<jtv> The new exception type is _very_ specific...  Was there no "no package specified" exception anywhere that would suit this case?
<jtv> Also, maybe it should be derived from InvalidArguments?
<bigjools> yes, there is an unknown package name one somewhere, but InvalidArgs is better and is something that should be in Foundations if it's not already?
<jtv> Well, "unknown" package would be a very different error from a "none specified"
<bigjools> that's fine.  Considering that this error is a programmer error, they will only be reading the exception text
<bigjools> but anyway, InvalidArgs FTW
<noodles775> jtv: as I mentioned in the MP, I did search for InvalidArgs and other general exceptions for api usage - and didn't find any. I can create one if it's worthwhile as part of the rc branch?
<noodles775> OK, will do.
<jtv> noodles775: ah, I read your MP as meaning that you'd found one called InvalidArguments but decided against using it.
<noodles775> jtv: ah, no. I checked for one expecting to find one.
<jtv> that changes everything
<bigjools> I say add one and mail the list
<jtv> There ought to be one in the Python standard lib.
<noodles775> jtv: it needs to be decorated as a web service exception.
<jtv> Right, I'm only saying there must be a more specific base class than Exception to base yours on.
 * jtv digs through library docs
<jtv> Amazingly, there isn't.
<jtv> Then the current arrangement is fine; adding an InvalidArguments would be a good thing, but not in an RC candidate.
<noodles775> Great... I'll start straight on this release blocker for beuno then.
<jtv> Sure.  I could go bikeshed the wording of everything but not worth the time in this case.  r=me
<noodles775> Taa.
<jtv> np
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â || queue: [] (pqm is release-critical)
<jtv> allenap: I'm relocating to a different office, so will be afk for a bit.
<allenap> jtv: Cool.
<jtv> allenap: actually, that's exactly what I'm hoping to get away from.  Freezing in here!
<allenap> jtv: :)
<bac> noodles775: rc=bac.  please make sure that bug is tracked on CurrentRolloutBlockers
<noodles775> bac: ok, thanks.
<intellectronica> bac: can i still get r-c for http://pastebin.ubuntu.com/276303/ ? it's a pretty simple change (r=gmb) to fix https://bugs.edge.launchpad.net/malone/+bug/432088 which is quite important to the ubuntu developers
<mup> Bug #432088: Disable +filebug redirection for ~ubuntu-bugcontrol <Launchpad Bugs:In Progress by intellectronica> <https://launchpad.net/bugs/432088>
<bac> intellectronica: yes.  i'd really like a MP for tracking, even though it is a small fix
<intellectronica> bac: https://code.edge.launchpad.net/~intellectronica/launchpad/bug-supervisor-no-redirect/+merge/12269
<intellectronica> gmb: would you mind stamping https://code.edge.launchpad.net/~intellectronica/launchpad/bug-supervisor-no-redirect/+merge/12269 since you code-reviewed it?
<bac> intellectronica: thanks tom
<bac> gmb: please add your mark and set the whole MP to approved when done.
<bigjools> jtv, allenap: I have a short r-c branch if one of you can take it please?
<allenap> bigjools: Sure.
<bigjools> allenap: fab thanks, the MP will appear soon when the email gets through the tubes
<allenap> bigjools: Cool.
<jtv> allenap: I'm back.
<allenap> jtv: Hi!
<jtv> no sudden rushes while I was away?
<bigjools> allenap: grrrrrrrrr it seems that using bzr send against db-devel doesn't work
<bigjools> allenap: https://code.edge.launchpad.net/~julian-edwards/launchpad/no-udev-diff/+merge/12270
<allenap> bigjools: On it now :)
<bigjools> posting diff now
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, bigjools || queue: [] (pqm is release-critical)
<gmb> bac, intellectronica: Done.
<intellectronica> gmb: cheers
<allenap> bigjools: Approved. Nasty, nasty udev :)
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, - || queue: [] (pqm is release-critical)
<bigjools> allenap: nasty diffutils more like!
<bigjools> thanks!
<bigjools> bac: can you r-c that one as well, you're on the MP
<allenap> bigjools: Oh yes. That is a bit of a pain.
<jtv> allenap: you don't like emdashes?  :-)
<allenap> jtv: I don't have an emdash key on my keyboard :)
<allenap> jtv: And I'm too lazy to copy and paste.
<jtv> allenap: <compose> and three dashes  :)
<allenap> jtv: I don't know what a <compose> key is.
<jtv> allenap: it's like a doorway into a bigger world.  In my case it's a key with a weird little kind of swastika flag on it, but it's configurable.
<allenap> jtv: Ah, okay, I'll look it up. Still, that's four key strokes versus my one.
<jtv> allenap: but you _get_ so much more for it!
<bigjools> jtv: like RSI?
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, lunch || queue: [] (pqm is release-critical)
<allenap> jtv: I'm off for ~3 hours, but I'll be back later.
<bac> bigjools: looking
<bac> noodles775: done
<noodles775> Ta.
<bac> bigjools: rc=bac with minor mods
<bigjools> bac: got 'em, ta
<bac> intellectronica: are you going to have allenap look at the MP you submitted?  i can't run it now but i'd like someone to.
<deryck> jtv -- could you review a branch for me?  The last of the template conversions! :)
<jtv> deryck: if it's small!
<deryck> jtv, 179 lines ok?
<jtv> deryck: ok
<deryck> jtv, https://code.edge.launchpad.net/~deryck/launchpad/person-bug-page-ui-update-434794/+merge/12275
* jtv changed the topic of #launchpad-reviews to: on call: allenap || reviewing: deryck (by jtv, who is quiescing) || queue: [] (pqm is release-critical)
<jtv> deryck: you filled the heading slot.
<deryck> jtv, indeed I did.
<jtv> Bad, bad deryck.
<jtv> Also, you're using getSearchPageHeading as page_titles, but they're basically labels AFAICS
<jtv> A label should give broad context; today's page_titles should only identify the page within the context of the preceding breadcrumb.
<deryck> jtv, ah
<jtv> You don't want the breadcrumbs to go "Deryck H Â» Bugs Â» Bugs commented on by Deryck H"
<deryck> jtv, so that's why page_title didn't show up and I had to fill heading slot.
<jtv> You want them to go "Deryck H Â» Bugs Â» Commented" or somesuch
<jtv> The page_title doesn't go in the heading.  It doesn't go into the h1.  It goes into the <title>, followed by the rest of the breadcrumbs.
<jtv> (Because it also goes at the end of the breadcrumbs)
<jtv> This was changed in week 3
<jtv> So: label becomes h1, page_title becomes a breadcrumb, breadcrumbs reversed become title.
<deryck> right, I follow now.  And I knew it changed.  I'll plead laziness for not going to read the rules again.
<jtv> That's why your pagetest at the end has these nasty repetitive titles.
<jtv> deryck: Re-reading rules is generally futile.
<deryck> and people say defining a page title manually is more work. ;)
 * deryck ducks
<deryck> jtv, I'll do a quick update now
<jtv> thanks
<bac> gmb: could you get a code review on your branch before requesting the r-c?
<jtv> deryck: doing it right can be surprisingly hard, and this kind of fix generally pays off for multiple pages.
<jtv> deryck: and once you get this implemented consistently, you may be amazed at how clear it is.
<gmb> bac: I've already requested one; sorry for jumping the gun on the rc.
<bac> gmb:  np.
<bac> intellectronica has done the same.  i'm trying to avoid a trend.  :)
<gmb> Heh.
<gmb> bac: It's the first time we've tracked RCs this way; maybe we need to write down some guidelines.
<deryck> jtv, it feels like editor wars to me.  some of us like our page title code to be an operating system. ;)
<jtv> deryck: you know the rule: whoever mentions emacs first...  :)
<deryck> heh
 * deryck mubles somthing about being able to read email and rss and do irc from our page title code
<jtv> deryck: I have to go; I fear allenap may have to take up the rest of this review.  Another point is that the actions menu really doesn't belong in the sidebar in the new design afaics.
<jtv> It ought to be a horizontal <ul> of links, each with an info icon, at the top or bottom.
<deryck> jtv, I followed registry template examples there, or so I thought.  but I can follow up with allenap 
<jtv> I suspect there's no time to fix that anyway.
<jtv> So at best, it's a bug waiting to be filed.
<jtv> deryck: I really have to go.  I'll vote Needs Fixing and let allenap work on this, OK?
<bac> gmb: yeah, we're just making it up as we go along.  i think r-c's were done via MPs at least once before, though maybe not exclusively
<deryck> jtv, that's good, thanks
<jtv> OK, g'night!
<gmb> bac: I only ever remember using the pester-kiko-until-he-says-yes approach. Actually having a procedure for it is weird.
<bac> gmb: :) -- i do remember kiko sending me back to the MP before.
* jtv changed the topic of #launchpad-reviews to: on call: allenap || reviewing: â || queue: [deryck: The Lasts Template Conversion, in Needs Fixing now] (pqm is release-critical)
<bac> gmb:  we need to figure out how to get rid of the dumb CRB wiki
<gmb> Yeah.
<gmb> allenap: When you're back from lunch, can you take a look at  https://code.edge.launchpad.net/~gmb/launchpad/enable-bz-3.4-bug-415779/+merge/12276 for me? It's the last of the Bugzilla work.
<gmb> allenap: I need to step out for a bit myself, so if I'm not back please just comment on the MP.
<gmb> deryck: FTR, I'll take over your review from jtv. Not that I actually have anything to add at this point... looking nwo.
<deryck> gmb, thanks.  Almost have an updated diff now.
<gmb> deryck: Cool. I don't have anything to add to jtv's comments.
<bac> intellectronica: i'm looking at your branch with the search box spacing
<bac> intellectronica: it looks like it is worse to me
<bac> deryck: heads up that PQM is closing in 2:45.  you've not got much time to land via ec2
<deryck> bac, I already ran through ec2test last night.  I know which tests to watch as I update this.
<bac> deryck: great!
<bac> deryck: please bin/test -vvm lp.bugs too
<deryck> bac, ok
<gmb> bac: Will you be able to do my code review too or are you too loaded at the moment?
<bac> gmb: i'm looking now
<gmb> bac: Awesome, thanks. It's already playing in ec2, FTR.
<deryck> gmb, can you look at my updated diff, please?
<gmb> deryck: Sure. Is it on the MP or can you paste it for me?
<deryck> gmb, I added it to the MP
<gmb> Hmm, I don't see it.
<gmb> deryck: It's not showing up for me. Can you paste it for the sake of speed?
<deryck> gmb, sure.
<intellectronica> bac: why? i prefer the details to wrap if necessary. how do you suggest fixing this?
<deryck> gmb, http://pastebin.ubuntu.com/276403/
 * gmb looks
<gmb> deryck: Looks good to me.
<deryck> gmb, awesome, thanks.
<deryck> gmb, one more test fix, too.
<gmb> Ok...
<deryck> gmb, http://pastebin.ubuntu.com/276406/
<gmb> deryck: Looks good too.
<deryck> gmb, thanks
<deryck> bac, can you look at this for rc approval?  https://code.edge.launchpad.net/~deryck/launchpad/person-bug-page-ui-update-434794/+merge/12275
<bac> deryck: i will
<deryck> bac, thanks!
 * deryck is running tests now
<bac> gmb: land it
<gmb> bac: Awesome, thanks.
<bac> intellectronica: i ran your new code and what is on trunk.  both wrap as i make the browser smaller but the changed version has the details section wider and thus wraps sooner.  perhaps i'm missing the point.
<bac> intellectronica: what is the status of the fix for bug 432088 ?
<mup> Bug #432088: Disable +filebug redirection for ~ubuntu-bugcontrol <Launchpad Bugs:In Progress by intellectronica> <https://launchpad.net/bugs/432088>
<intellectronica> bac: just finished ec2 and is on the patch queue
<bac> intellectronica: great!
<bac> intellectronica: as to the search box, i see the details portlet shifted to the left, creating extra space on the right.  this causes the search box to be pushed down on browser widths for which it doesn't happen on trunk
<bac> intellectronica: so what i see is the details portlet always increasing in width.
<intellectronica> bac: oh. is that using safari? that's not what i see in firefox
<bac> no, in FF
<bac> but, admittedly, FF in os x
 * bac looks at FF in jaunty
<intellectronica> bac: it wraps as i expect in chromium too for me, but it does eventually still cause the search box to move down if you resize smaller
<bac> intellectronica: right.  but my point is your branch actually causes it to push down sooner as the browser gets narrower
<bac> intellectronica: and i confirmed the same in FF on jaunty
<intellectronica> bac: let me think. i might just move the details back to the side bar for now
<bac> intellectronica: tom TBH if you have other work to do i'd recommend it.  i'm not inclined to think this is an r-c issue regardless of the solution.
<bac> deryck:  looking at your branch now
<intellectronica> bac: i would really like to solve this before the release, though i realise it's not a real blocker
<deryck> bac, cool, thanks
<bac> intellectronica: i understand.  but at this late hour i don't think it is worth the risk of another branch in the mix.
<bac> intellectronica: there will be re-roll opporunities, i'm quite sure.
<bac> deryck: i don't see any breadcrumbs
<deryck> bac, yeah, the first page doesn't have them.  I don't get what's required to enable them, and meant to ask, but forgot.
<bac> deryck: now i see breadcrumbs on all pages but 'list all related pages'
<bac> hey barry!
<deryck> bac, right.  I poked at it a bit last night, but never could work out what I needed to do for that one page, so I just forgot to come back to it.
<bac> deryck: hound barry or salgado.  there is a very specific fix for this type page
<deryck> barry, ping.  Can you help me work out why one page doesn't have breadcrumbs?
<bac> deryck: the 'action' portlet looks very LP 1.0.  we can land as is but please immediately start work to have it redone for the first re-roll
<salgado> deryck, which page is that?
<deryck> hi salgado, thanks.  let me get an MP link if you need.  It's the person bug page.  bugs.lp.dev/~name16 for example
<deryck> salgado, https://code.edge.launchpad.net/~deryck/launchpad/person-bug-page-ui-update-434794/+merge/12275
<deryck> bac, so should the links not be in the side slot?  They should be in the main page?  Or just the look of the sidebar is wrong to you?
<bac> deryck: look is wrong.  perhaps they shouldn't be there.  no icons
<salgado> deryck, can you tell me what you'd expect for the breadcrumbs and what you see instead?
<deryck> bac, right, I thought they needed icons, but that position was ok.
<bac> deryck: it smells funny but i'd defer to beuno for the corrective action
<bac> deryck: i'm thrilled we've gotten this page where it is, don't get me wrong
<deryck> salgado, there is no breadcrumbs at all.  I would expect "Foo Bar: Related bugs" or something similar.
<deryck> bac, right, me too.  We need to get what needs to be fixed documented, too.  If I come off frustrated, it's only because I feel like I have to relearn the rules everytime I do a conversion.
<bac> deryck: understood
<beuno> bac, deryck, what's up?
<deryck> I jump from conversions to other things a lot lately, too.  So maybe it's just me.
<salgado> deryck, you need a Breadcrumb adapter similar to BugTargetOnBugsVHostBreadcrumb (in lib/lp/bugs/browser/bugtarget.py), but for IPerson
<bac> beuno: here's the current look of the person bug page: http://people.canonical.com/~bac/bug-page.png
<beuno> right
<beuno> wrong
<beuno> how about a drop down as a filter
<deryck> salgado, but the other views in browser/person.py for these bug views have breadcrumbs, so shouldn't this already exist?
<beuno> which is kind of what it is?
<beuno> on top of the listing
<bac> beuno: it has issues with breadcrumbs on that page and the actions look incorrect.  it just got converted so i'm inclined to have it land and fix it first opportunity
<beuno> View: [INPUT BOX THAT NAVIGATES ON CHANGE]
<salgado> deryck, the other views have because they're not the default view for IPerson on that layer
<bac> beuno:  we have little time for any but the most simple changes
<beuno> bac, it's a pretty important page to land this way, no?
<beuno> also
<beuno> the format of the listing is different than other places, I think
<salgado> deryck, and they lack an intermediate breadcrumb pointing to bugs.lp.dev/~person
<deryck> salgado, ok, I'll take another stab at it.  I tried a breadcrumb adapter last night and ended up with zcml errors.
<salgado> deryck, it's got to be a named adapter, like in the zcml declaration of BugTargetOnBugsVHostBreadcrumb
<deryck> beuno, bac -- so how much time do I left to work on this?
<deryck> salgado, ok
<bac> beuno: if we want this page fixed for our deadline we'll need some prescriptive help from you ASAP.
<beuno> bac, drop down
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: â || queue: [deryck: The Lasts Template Conversion, in Needs Fixing now, noodles] (pqm is release-critical)
<beuno> and use the same formatting for the list as everywhere else
<beuno> that's it
<beuno> or is there something I'm missing
<beuno> those option s are a filter
<beuno> lets show them as such
<noodles775> Hi allenap, if/when you have time, another RC: https://code.launchpad.net/~michael.nelson/launchpad/sprint-index-should-hide-heading-db-devel/+merge/12284
<bac> deryck: PQM closes at 16:00 UTC -- less than two hours from now
<beuno> bac, deryck, I'm 100% available for you guys
<bac> beuno: thanks
<bac> beuno:  i don't know what you mean by 'dropdown'
<deryck> bac, beuno -- I worry that adding a drop down menu and changing the fields will take a bit of time, and also cause test failures which will take a bit of time.
<deryck> well the fields change isn't hard, but the test failures are real if the fields are tested
<bac> beuno: should "List assigned bugs" become (i) Assigned bugs?
<deryck> plus I have to now get the breadcrumbs right for the page.
<beuno> bac, yes
<beuno> deryck, the listing should look like this: https://bugs.edge.launchpad.net/bzr/
<bac> beuno: and should the be <ul horizontal> ?
<beuno> well, if a drop down is out of the question
<beuno> then lets move it from the side bar
<beuno> use the whole page
<beuno> and have something like:   "See all bugs: assigned to me, I've commented on, reported..."
<deryck> beuno, can you do me a sketch or mockup so I can picture how you want it?  Is that possible?
<beuno> above the listing
<beuno> deryck, for which of the two options?  :)
<beuno> I'll do whatever you need
<deryck> beuno, so just a paragraph  of links basically?  Or a list?
<deryck> beuno, the related pages links I'm asking about
<bac> beuno: are you suggesting the list side bar have those counts?
<beuno> deryck, I'd have links in a phrase, one line, above the table
<beuno> well
<bac> beuno: i think your example page is misleading then
<beuno> if you want to make it inconsistent with launchpad, but consistent with the bug page
<beuno> add the counts
<bac> beuno: something more like https://edge.launchpad.net/~bac
<beuno> then yeah
<beuno> bac, deryck, like: https://code.edge.launchpad.net/~beuno
<beuno>  93  owned branches, 101  registered branches, 103  subscribed branches  
<deryck> beuno, ok.
<beuno> if you can't do the counts, no counts
<deryck> beuno, bac -- is there an easy way to get the elements in context/@@+global-actions or do I have to build this by hand?
<beuno> deryck, and making the listing look the same format as the rest is the most important
<deryck> I can always look at the branches code, too.
<beuno> have bug icons, #, etc
<beuno> like the listing in: https://bugs.edge.launchpad.net/bzr/
 * beuno doesn't know about contexteses
<deryck> beuno, you don't want the "In" column in these listings?
<beuno> well...
<beuno> damn
<beuno> yes
<beuno> so just displau the bug # the same way  :)
<deryck> beuno, ok :)
<bac> beuno: ok
<beuno> \o/
<beuno> group hug
<deryck> beuno, bac -- so breadcrumbs on the first page, links above bug list, and bug number formatted the same in bug list.  yes?
<beuno> deryck, yes
<intellectronica> beuno: as for the bugtarget homepage, would you be ok with me moving the details portlet back into the sidebar? there are various problems with its current location which i can't seem to solve, and i think this will be a reasonable compromise
<deryck> beuno, bac -- I'll work on this, but I think it's going to be quite close for an hour and a half work.
<beuno> deryck, ignore IRC, and may the force be with you
<beuno> intellectronica, let me look...
<noodles775> bac: you should have just received an rc-request for a branch of mine (not yet reviewed), but from what you've said, it won't make pqm (by the time I get it through ec2test etc.). So should I remove the rc-request? Or is it worth going ahead in case things are delayed?
<bac> noodles775: i'll look at it.
<beuno> intellectronica, it's not my favorite thing, but if it has to be, it has to be. If you do, make sure that label and value are on 2 different lines, with enough vertical spacing between the next st of label:value
<noodles775> bac: ok, thanks.
<intellectronica> beuno: yes
<barry> deryck: still need help?
<deryck> barry, I think I'm ok now.  thanks
<bac> intellectronica: is the testing for the change guaranteed to be isolated to lp.bugs?
<intellectronica> bac: it's a template of the bugs application, but i can't guarantee that it's not used in the test suite of other applications
<noodles775> bac: I forgot to mention two things on that mp: (1) the second h1 heading that is displayed is actually incorrect - it refers to blueprints, (2) I also fixed the icon issues for attendees as you'll see in the screenshots.
 * noodles775 adds that to the mp
<barry> deryck: cool
<bac> intellectronica: PQM closes in 90 minutes.  i cannot accept the risk of test failures.  make the change now if you, as noodles775 has done, but it probably won't get included.
<intellectronica> bac: ok, yes, this is not enough time
<bac> intellectronica: i appreciate your desire to get this fixed!
<bac> noodles775: your fix is good too.  i wish we could squeeze it in.  luckily it is a rarely visited page.
<noodles775> bac: thanks. OK, I'll send it off to ec2test just in case something happens to delay things :) 
<bac> noodles775: do we know if other pages that inherit from HasSpecificationView are affected by the <h1> problem
<noodles775> bac: as far as I know, there are not any other IMajorHeadingView views that inherit from HasSpecificationView - the sprint is quite unique in that way. But I'll check.
<bac> noodles775: good to know.  thanks for looking.
<allenap> deryck: Has your branch already been reviewed?
<deryck> allenap, yes, thanks
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: noodles || queue: [] (pqm is release-critical)
<bac> allenap: what is the status for bug 423105
<mup> Bug #423105: Duplicate download icons in many places <Launchpad Foundations:Fix Committed by beuno> <Launchpad Bugs:In Progress by allenap> <https://launchpad.net/bugs/423105>
<allenap> bac: Landed in db-devel.
<allenap> bac: I'll update CRB
<bac> thanks
<bac> gary_poster: rc=bac.  land on db-devel at your leisure
<bac> deryck: how's it coming?
<gary_poster> bac thank you
<deryck> bac, painfully ;)
<deryck> bac, links are done, working on list format now
<bac> noodles775, intellectronica, gmb, gary_poster: can you update CRB as required to reflect current statuses
<gary_poster> bac yes
<deryck> bac, there will most certainly be tests broken with these changes.  I don't see with running tests to find spots and fixing how I can make it in an hour from now.
<bac> noodles775, intellectronica: please move your branches that won't make it to 'not blockers'
<bac> deryck: push your branch as soon as you can
<deryck> bac, ok
<bac> we can manually parallelize the test suite
<bac> you can run lp.bugs, i can run lp.registry, etc
<deryck> bac, gotcha
<bac> barry and his massive new machine can run the rest
<kiko> fe fi fo fum
<beuno> look!  a kiko!
 * gary_poster doesn't have blood of englishman.
<gary_poster> well, maybe just a bit.
<kiko> just a wee bit
<kiko> as they say
<noodles775> allenap: thanks.
<allenap> noodles775: Welcome.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] (pqm is release-critical)
<bac> noodles775: i like your test to ensure no <h1> is in the main content.  is that something that should be applicable to all pages?
<bac> noodles775: would've come in handy to spread that around a few weeks ago.
<jml> https://code.edge.launchpad.net/~jml/launchpad/code-to-branches/+merge/12288
<noodles775> bac: hmmm... I'd just added that as a regression for that bug, as it doesn't apply generally (most pages *should* have an h1 in the main content above the breadcrumbs)
<noodles775> bac: it's only views that implement IHasMajorHeading that should not have an h1 in the main content.
<bac> noodles775: ah, right.  i was thinking of a restriction below the line.
<bac> jml: are you proposing that branch for inclusion in the release?
<noodles775> bac: I've got my branch on ec2test to land on db-devel just in case it's still open (unlikely), but in the off-chance, could you approve the rc if you agree?
<jml> bac, yes.
<bac> noodles775: we have no intention of delaying the closing atm.  i've already looked at your changes and can give you an RC quickly should the circumstance arise.
<bac> jml:  has it been through ec2?
<noodles775> bac: k. great.
<jml> bac, it has, but there were failures. I fixed all of the failures, but there is a chance that I missed some.
<jml> note also that there were unrelated failures from previous branches, which I mentioned on #launchpad-dev
<bac> jml:  PQM closes in 31 minutes.
<jml> bac, shall I propose it for the next rollout then? :)
<bac> jml:  there is no time.  yes, please queue up for a re-roll in the very unlikely event that should happen.  :)
<jml> bac, will do.
<bac> jml:  would you add it to the non-blocker section of https://dev.launchpad.net/CurrentRolloutBlockers
<jml> bac, added, but left disposition blank
* bac changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] (pqm is release-critical) This channel is logged: http://irclogs.ubuntu.com/ ||
* bac changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
#launchpad-reviews 2009-09-24
<jtv> allenap: I hope you're not still ocr'ing...  :)
* jtv changed the topic of #launchpad-reviews to: on call: â || reviewing: â || queue: [jtv] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> oh, it's thursday...
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: â || queue: [jtv] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<jtv> o/
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: jtv || queue: [-] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> jtv: which of your two do you want me to start with?
<jtv> noodles775: db-bug-435655, but turns out my last push (minor test fixup) failed.
<jtv> Let me check that the diff is at least reasonably representative.
<jtv> Oh well, the meat of the branch is there, just not a few removed lines.
<jtv> noodles775: pretend the import at the very top of the diff isn't there (I ended up not needing it), and that one line of output has disappeared from the doctest.  :)
<noodles775> :) I'll merge your branch locally anyway - so just let me know when you've pushed the latest change.
 * jtv tries to break-lock...
<jtv> noodles775: pushed.  Review at your convenience!
<noodles775> ta
<allenap> jtv: Ah, no. Thanks :)
<jtv> allenap: did deryck get his template conversion through yesterday?
<noodles775> jtv: r=me, sent.
<jtv> noodles775: thanks!  Got another one coming up.
<jtv> (Much more important than the one I had on the queue from last week)
<noodles775> jtv: ok, does that mean there will be a re-roll?
<jtv> noodles775: dunno, but if not, I'm going for 2 CPs.
<noodles775> k.
<jtv> Frankly, for this particular one, it'd be pretty impressive if we didn't need a re-roll...
<jtv> noodles775: great suggestion to test for the Committed notice, thanks.  I'm using regex search to find it.
<noodles775> jtv: why couldn't you just use assertIn() as you've done for the others?
<jtv> noodles775: mostly because of the spacing between INFO and Committing.
<jtv> If that changes, this test won't breakâbut it will no longer have any value as proof that the commit doesn't happen.
<noodles775> k.
<jtv> I could search for just Commit, but that's much too brittle for my taste.  I like tests that break when something important changes, not as superficialities change.
<noodles775> Yup.
<noodles775> jtv: Sorry, I still don't see why self.assertIn('Committed 0 file(s)', stderr) isn't what you want... what did I miss there?
<jtv> Has no value as proof if the number isn't 0.
<noodles775> jtv: but in that specific case, you want to ensure that the number is zero right?
<jtv> I want to assure that the message isn't there.
<noodles775> of course... I was thinking of checking that it was.
<jtv> Or a message that would be in its place if things went wrong.
<jtv> OIC... for that check I'm much less picky!
<noodles775> Yep.
<jtv> noodles775: my other critical MP is: https://code.edge.launchpad.net/~jtv/launchpad/bug-435699/+merge/12338
<noodles775> jtv: yep, already started.
<jtv> oh, sorry
<noodles775> np!
<noodles775> jtv: in _setUpDistro, you've replaced some lines with calls to _makeTemplateForDistroSeries... is it intended that this will not change the test at all (ie. just refactoring)?
<noodles775> jtv: or, the other way around, is it relevant that now a POTemplateSubset is created for each template, where as before the same one was used twice?
<jtv> noodles775: it's purely a refactoring, yes.  POTemplateSubset is stateless.
<noodles775> jtv: great.
<noodles775> jtv: r=me for the second one :)
<jtv> noodles775: thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [-] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<bac> jtv: rc=bac on bug 435655.  please add to CRB.  has it gone through ec2 yet?
<mup> Bug #435655: Export to branch fails: unsupported operand type(s) for -: 'float' and 'datetime.timedelta' <Launchpad Translations:In Progress by jtv> <https://launchpad.net/bugs/435655>
<jtv> bac: no, still in there, but has been for a while
<bac> jtv:  thanks.  no decision has been made about a re-roll but this should be a candidate
<bac> hi noodles775
<jtv> bac: thanks for that.  Pretty urgent stuff.
<bac> BjornT: ping
<BjornT> hi bac 
<bac> jtv: how long will you be around
<jtv> bac: a while, I can check in at night.
<bac> hi BjornT -- i have to leave in just a few minutes for a dental appt.  can i ask you to be RM until i return in a few hours?
<BjornT> bac: sure
<bac> BjornT: there are 4 MPs with r-c requests.  thanks!
<BjornT> bac: ok. what's the status of bug 434519? it's fixed released, but is still on CRB
<mup> Bug #434519: Text in overlay is now centered <Launchpad Foundations:Fix Released by michael.nelson> <LAZR Javascript Library:Fix Released by michael.nelson> <https://launchpad.net/bugs/434519>
<leonardr> noodles775, care to review a javascript branch? (not release critical) https://code.edge.launchpad.net/~leonardr/launchpad/get-field-uri/+merge/12344
<bac> BjornT: could you have noodles775 update CRB for that bug?
<BjornT> noodles775: ^^^
<BjornT> noodles775, bac: nm, i was looking at the wrong wiki
<noodles775> leonardr: sure!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: leonardr || queue: [-] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> leonardr: actually, I'll just do cprov's first if that's ok... looks urgent.
<leonardr> noodles775, sure
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: cprov || queue: [leonardr] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<cprov> thx, noodles775
<noodles775> np
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: jml || queue: [leonardr] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> noodles775: Hi!
<noodles775> Hi henninge :)
<henninge> noodles775: Can you please do a code and ui review here? :
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-435712/+merge/12347
<noodles775> henninge: sure - is it an rc? if so, I'll do it before leonardr's otherwise after?
 * noodles775 checks
<henninge> noodles775: yes, I plan to r-c it.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: jml || queue: [henninge, leonardr] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> henninge: usually I'd leave it unclaimed until I start, just in case others join reviewing soon.
<henninge> noodles775: ah, sorry. I just requested the ui part from you personally. It's really easy, too.
<noodles775> Great!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: henninge || queue: [leonardr] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> jml: r=me - it might be worth adding an r-c request to your MP now.
<noodles775> Hi henninge - there's no leaf breadcrumb on that question page - is that intentional or it just hasn't been done yet?
<noodles775> (ie. outside of the scope of this branch)
<henninge> noodles775: the latter, I filed a seperate bug about it. FAQ pages seem to suffer from that, too.
<noodles775> aha
<henninge> noodles775: bug 435743, just fyi ... ;)
<mup> Bug #435743: Bread crumb missing for question and FAQ <Launchpad Answers:New> <https://launchpad.net/bugs/435743>
<noodles775> Great.
<jml> noodles775, which one?
<noodles775> jml: Code->Branches?
<jml> noodles775, ahh, thanks.
<noodles775> jml: just see the note I added about other branches introducing print_location...
<jml> noodles775, yeah, I just saw that.
<jml> noodles775, really the best thing to do with that is land it asap
<noodles775> jml: yes, but even then there are risks (like my branch which I updated).
<jml> noodles775, well I guess I mean land via ec2test
<noodles775> jml: but with a re-roll - even if we both sent off our branches again to ec2test - to land, they'd both pass (as they wouldn't have each others changes right).
<noodles775> it would only be the integration that would fail.
<noodles775> jml: I guess what I'm trying to say (but doing so poorly) is that the issue here is us all landing these branches for the re-roll at the same time.
 * jml misses pqm
<noodles775> henninge: code=me, but I'd like someone else to look at the ui as I had a few questions.
<noodles775> jml: heh, yeah, that'd solve it :)
<jml> noodles775, sure. I can take a reasonable amount of care to check for that.
<noodles775> Great.
<jml> noodles775, but in those cases, our present build system basically says "be ready to apply a [testfix]"
<henninge> noodles775: so do I find somebody else for ui?
<henninge> or do you?
<jml> noodles775, which I'm happy to do, as long as it doesn't screw the release.
<henninge> noodles775: thanks anyway ... ;-)
<noodles775> jml: yep, I guess we could just try to avoid an extra 4hrs.
<jml> yeah.
<noodles775> henninge: np. And yes, I was about to ping someone... rockstar could you give a second opinion on the ui for henninge's branch:
<noodles775> https://code.edge.launchpad.net/~henninge/launchpad/bug-435712/+merge/12347
<rockstar> noodles775, sure.
<noodles775> Thanks!
<rockstar> henninge, ah!  Thanks for doing this!  When I fixed this page, that slot didn't exist.
<rockstar> henninge, a big +1 from me.
<henninge> rockstar: pleasure
<noodles775> henninge: did my question about the status make sense? I wasn't sure if it should be included there in the rego info, but maybe it's something you discussed with beuno et al?
<rockstar> noodles775, it was discussed with beuno.  It allowed us to remove on entire superfluous portlet.
<noodles775> OK, great!
<henninge> noodles775: yes, makes sense.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: henninge || queue: [leonardr,mrevell/bac] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
* rockstar changed the topic of #launchpad-reviews to: on call: noodles775, rockstar || reviewing: henninge, leonardr || queue: [mrevell/bac] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<rockstar> leonardr, I presume your branch is a lazr-restful branch, correct?
<rockstar> Can you point me to it?
<leonardr> rockstar: no, it's a launchpad branch
<noodles775> ah great - I was just about to apologise to leonard for not getting to it!
<noodles775> https://code.edge.launchpad.net/~leonardr/launchpad/get-field-uri/+merge/12344
<leonardr> though i do have a lazr.restful branch coming up in just a minute--writing the mp now
* noodles775 changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: henninge, leonardr || queue: [mrevell/bac] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<rockstar> noodles775, I got your back.  :)
<noodles775> Thanks rockstar - about to EOD.
<bac> rockstar: are the reviews you are working on RC re-roll candidates?
<rockstar> bac, not sure.
<bac> rockstar: please ask.  jump mrevell and any others to the top, please.
<mrevell> thanks bac
<rockstar> bac, I assumed since there was no one on duty when I started, no one needed theirs ASAP.
<bac> rockstar: mrevell's will take 2 seconds
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: mrevell/bac || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<bac> rockstar: sure, that's reasonable.
<rockstar> I don't see one in here from mrevell
<mrevell> rockstar: it may be under bac
<rockstar> Ah, I see it.  It's yours bac.  :)
<mrevell> rockstar: https://code.edge.launchpad.net/~bac/launchpad/lp-blog/+merge/12351
<rockstar> bac, r=me
<mrevell> thanks rockstar
<bac> tahnks rockstar.  sorry for the disruption.
<rockstar> I'm a little confused on why we're copying the blog instead of aggregating it though.
<rockstar> bac, I'm done now anyway.  :)
<leonardr> rockstar, here's the lazr.restful branch: https://code.edge.launchpad.net/~leonardr/lazr.restful/431986-field-resource-link/+merge/12357
<bac> rockstar: that's coming soon
<mrevell> rockstar: we'll be aggreagting soon
<rockstar> bac, okay.
<mrevell> or even aggregating
<mrevell> bac I'll send it to pqm
<mrevell> if I get rc from you bac
<bac> mrevell: thanks
<mrevell> heh
<bac> mrevell: using your new dbsubmit alias
<mrevell> thanks abc
<mrevell> bac
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: leonardr || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> noodles775: oops, I was so preocupied with the ui issue that I didn't see your suggestions in the code. Seeing that this is r-c, I will not do too much here, though.
<noodles775> henninge: yep - np!
<bac> henninge: re: your redirect patch.  can you add a comment better nailing down what "old stable release" means?  how long do we need to support it?  hardy + 5 years?
<rockstar> leonardr, r=me
<henninge> danilos: ^^
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> bac: AFAIUI it just changed in Karmic, so hardy +5 years would seem right.
<danilos> bac: in general, we are not exactly sure what was the first release with these links, and how long do we want to support them
<bac> danilos: normal releases only 2 years, as i understand.  LTS for 5.  that's why hardy+5 was my suggestion.
<henninge> bac: but Karmic is mentioned in the test as the release where it changed, so anything before Karmic would be considered "old" then.
<danilos> bac: yeah, I am not sure if we need to support dapper still, but hardy+5 is probably sane
<danilos> of course, hardy+5 covers dapper+5
<bac> well dapper+5 < hardy+5
<bac> :)
<danilos> also, 5 years is server only, but I'd definitely not try to determine when we can drop the support for these links now :)
<bac> best would be to just leave the redirect in there for a good long while
<danilos> bac: yep, but since it's an otherwise dead page, I wanted a comment in that would help us in the future determine if we can drop it
<bac> ok
<bac> henninge: so how about something to the effect of "This redirect must be supported for at least five years after the release of Hardy, which is 2013-04.  Please consult with the XYZ team before removing."
<henninge> bac: sounds great!
<stub> Open a bug to remove the link, create a milestone for LP 10.0 and target it.
<stub> (not serious)
<bac> henninge: rc=bac conditional on submission via ec2 to db-devel
<henninge> bac: thanks, it is already running through ec2.
<bac> henninge: super!
<henninge> bac: I have another one as you may have noticed ... 
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-435712/+merge/12347
<bac> henninge: ok
<bac> will look soon
<henninge> bac: thanks a lot
<cprov> sinzui: can you please take a quick look at https://code.edge.launchpad.net/~cprov/launchpad/bug-408528-ensurePerson-again/+merge/12356 ?
<cprov> bac: the death-row fix is a 'go', worked fine in production.
<bac> cprov:  will look now
<cprov> bac: https://code.edge.launchpad.net/~cprov/+activereviews
<cprov> err
<cprov> https://code.edge.launchpad.net/~cprov/launchpad/bug-430552-unblock-death-row/+merge/12346
<bac> yep
<cprov> bac: it's on pqm, thanks.
<bac> cprov: thanks.  please update CRB when it lands.  put it under the re-roll section
<cprov> I wonder what PQM is doing during the 20 minutes it takes to process a request.
<deryck> rockstar, ping
<rockstar> deryck, hi
<deryck> rockstar, I've got something to fix an oops that needs to go in.  Can you look at it?  One thing, though -- I have to leave in 5 minutes or so to pickup kids at school.
<deryck> rockstar, so I'm asking for offline, while I'm away.
<rockstar> deryck, cool.
<deryck> rockstar, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/filebug-redirect-package-oops-435628/+merge/12366
<rockstar> deryck, that's actually really straightforward.
<rockstar> deryck, r=me
<deryck> yeah, I think so, too.  
<deryck> excellent, thanks!
<rockstar> deryck, wait, why is it going to the tour?
<rockstar> Is this the change to the bug filing policy I saw recently for Ubuntu?
<deryck> rockstar, that's the test.  earlier in the test, it sets the redirect url to the tour.  I assume to make the test not hit network for a live URL.
<rockstar> deryck, alright, that's fine.
<deryck> cool
<henninge> bac: the +translations redirect patch is about to land!
<bac> excellent henninge.  when it does will you move it to the re-roll section of CRB and note the revno?  thanks.
<henninge> bac: I will
<kiko> bac, I'm seeing an issue with the homepage, where it misrendered /and/ the beta testers redirect is being displayed as a box instead of the inline ajax thing?
 * bac looks
<bac> kiko: what kind of bad rendering?
<bac> i don't see anything that looks off
<kiko> the search box to the right hand side
<kiko> blog posts to the left, search to the right
<kiko> instead of search in the middle
<kiko> bac, at any rate, there should be no beta testers redirect box up there -- it's in the footer everywhere
<bac> kiko: i know it is in the footer for other pages.  i don't know what the design intent for the front page was.  barry can you explain?
<bac> kiko: i'll agree its fugly
<bac> beuno: you around?
 * barry reads back
<kiko> beuno, please help 
<bac> beuno is probably on the floor at morpeth arms about now
<barry> kiko, bac that's the design i was given by beuno
<barry> kiko, bac as for the beta-box, i didn't touch that (wasn't part of the design change)
<bac> barry: so the search box is placed as intended
<barry> bac: as beuno intended, yes
<kiko> hmm, okay then
<bac> i know noodles moved the beta message and disable to the footer for internal pages but don't know about killing off the beta box
<bac> (sounds like sushi)
<kiko> needs to happen :-)
<barry> bac: i reviewed noodles' branch.  iirc, it did not touch the beta-box on the home page, just added to the footer of internal pages
<bac> barry: my recollection too
<bac> shall i open a bug 'kiko hates the beta box'?
<barry> :)
<kiko> the bug is "MARK hates the beta box"
<kiko> and asked a month ago to get it gone
<barry> kiko: mark hates > beuno loves
<bac> oh, that's even better
<bac> ok, will do
<bac> kiko: bug 436124
<mup> Bug #436124: Beta user warning on Launchpad homepage needs to go away <Launchpad itself:New> <https://launchpad.net/bugs/436124>
<kiko> thanks bac
<bac> np
<beuno> kiko-afk, bac, hi
<beuno> it does need to go away
<flacoste> cprov-afk: it's running make build
<barry> rockstar: ping
<rockstar> barry, hi
<barry> rockstar: do you have time to review a critical fix?
<barry> rockstar: it's slightly over the 800 line limit but mostly because there's a big delete in there
<rockstar> barry, only for you.
<barry> rockstar: you have once again earned your nick
<rockstar> :)
<barry> sending the mp now :)
<rockstar> barry, my new band is playing our first show on Halloween.
<barry> very cool!  and what are you going as? :)
<rockstar> Well, the dressing up thing happens AFTER the set. :)
<rockstar> Otherwise, I'd be going as "it's too hot from the lights to wear much of anything..."
<barry> rockstar: oh, so you're going as the red hot chili peppers
<rockstar> :)
 * rockstar is glad his mental firewall silently dropped that last few proposed responses
<barry> rockstar: mp is sent.  ping me if you have any questions about the test specifics.  running the MailmanLayer tests are um, "fun"
<rockstar> Oh man.  You didn't tell me it was a Mailman fix.
<barry> rockstar: gotta send me a link to the music when you have something.  i'd love to hear it
<rockstar> Deal is off, I can't review it.
<rockstar> :)
<barry> rockstar: not much mailman in there :)
<barry> rockstar: mostly integration issues, but i'm here to answer any questions
<barry> rockstar: those lint warnings are bogus.  ignore 'em
<rockstar> barry, so this was cowboyed into production already?
<barry> rockstar: not that exact patch, but something very similar.  this is cleaner
<rockstar> barry, okay.
<rockstar> barry, why the change from "Configure a mailing list" to "Create a mailing list"
<barry> rockstar: the link text on the team index page has changed
<rockstar> barry, was that part of the issue, or a "while I'm in here" change.
<barry> rockstar: it was a 'while i'm here' change ;)
<rockstar> barry, okay.
<barry> rockstar: these tests are not run automatically (there's an open bug on that) so whenever i mess around in them i first try to get them running again
<rockstar> barry, I don't have a problem with that, but I would think usually RC patches are the minimal to fix the "holy shits"
<barry> rockstar: you might be right; i'll fight that out with bac :)
<rockstar> I guess I've never thought about how much of our test suito is Mailman integration tests.
<rockstar> +    launchpad_top = os.path.dirname(
<rockstar> +        os.path.dirname(os.path.dirname(canonical.__file__)))
<rockstar> barry, ^^ that's sub-optimal.
<barry> rockstar: this is warsaw's extension to zawinksi's law
<rockstar> Which is?
<barry> rockstar: yeah.  we need to get to the top of the tree
<barry> rockstar: http://catb.org/jargon/html/Z/Zawinskis-Law.html
<barry> âEvery program attempts to expand until it can read mail. Those programs which cannot so expand are replaced by ones which can.â 
<rockstar> And your extension is?
<rockstar> barry, have you run this through ec2?
<barry> rockstar, essentially s/mail/mailman/ :)
<rockstar> :)
<barry> rockstar: no. it wouldn't help anyway because none of the MailmanLayer tests are run automatically
<rockstar> barry, oh snap.
<barry> rockstar: yeah.  there's an open bug about adding them to a separate buildbot, but it hasn't been done yet
<rockstar> barry, how confident are you that this patch isn't going to explode everything?
<barry> rockstar: i'm pretty confident.  the essential change is to add an import of _pythonpath to make sure it can find the eggs.  everything else is inconsequential to production
<rockstar> barry, okay, r=me
<barry> rockstar: but don't worry, i will work closely with the losas to make sure its application goes smoothly
<barry> rockstar: thanks!
<rockstar> barry, you are welcome.
<barry> bac: rc needed for https://code.edge.launchpad.net/~barry/launchpad/435604-mailman/+merge/12380
<bac> hi barry -- good timing.  i just got back.
<barry> bac: cool.  rockstar just reviewed and approved it
<bac> barry: are all of those lint message fake?
<barry> bac:  they are
<barry> bac: they're doctest globals for convenience
<bac> barry: i agree with rockstar that this fix should've been limited to a minimal change.  that said i know you can't see a test without convert it to ReST
<bac> but it does make sussing out the good parts hard
<barry> bac: yeah
<bac> but let's land this thing
<barry> bac: against devel, db-devel, or ...?
<bac> db-devel
<barry> bac: okay.  i'm submitting this straight to pqm since none of these tests are run by ec2 automatically
<bac> barry: but you've run them locally, right?
<barry> bac: lots :)
<bac> great
<bac> hey barry when you get a landing message would you update CRB and move it down to the 're-roll' section with the revno?
<barry> bac: yepper!
<rockstar> mwhudson, could you do a small review for me?
<mwhudson> rockstar: sure!
<rockstar> mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/fix-bmp-links/+merge/12384
<mwhudson> rockstar: after the standup i guess
<mwhudson> rockstar: reviewed, that was easy :)
* rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
#launchpad-reviews 2009-09-25
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/bmp-notification-recipients/+merge/12399
<mwhudson> thumper: replied
<thumper> mwhudson: ta
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/log-diffstat-error-and-continue/+merge/12400
<mwhudson> thumper: replied
<thumper> mwhudson: thanks, updated
<mwhudson> hm, not showing yet
<mwhudson> mumble mumble message queues
<thumper> :)
<allenap> BjornT: Are you CHR today? If so, do you fancy reviewing a branch that'll make your life better? :) https://code.edge.launchpad.net/~allenap/launchpad/just-comment-on-question-bug-114710/+merge/12403
<BjornT> allenap: indeed i am. i doubt that branch will be useful for me today, though, unless you plan to cowboy it to production :)
<allenap> BjornT: I hoped you wouldn't realise that though ;)
<BjornT> allenap: i'd rather not review such a big branch now, i want to get started with my CHR duties first. but maybe later
<allenap> BjornT: Sure. I'll see if anyone else can be roped in then.
<allenap> Actually, I'll wait for OCR later, that's probably less disruptive.
<BjornT> allenap: btw, why did you change the case of all the buttons? i though those should be Headline Case? (https://dev.launchpad.net/UserInterfaceWording)
<allenap> BjornT: Oh blast. There is a bit of a mix in LP at the moment, but that (headline) does seem to be the dominant form. Rats. Okay, I'll remove all that then!
<allenap> BjornT: Thankfully that part I did with a sed script, so I only spent about 5 minutes doing it.
<james_w> is requesting a review from ~lazr-developers not the thing to do?
<james_w> https://code.edge.launchpad.net/~james-w/lazr.restful/lazr.restful.fix_test_suite/+merge/8559
<james_w> https://code.edge.launchpad.net/~james-w/wadllib/fix-doctests/+merge/8476
<allenap> james_w: I'll review those.
<allenap> james_w: Both approved, thanks. Are you able to land those?
<james_w> thanks allenap, I am not
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
* barry changed the topic of #launchpad-reviews to: on call: salgado,barry || reviewing: -,- || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<allenap> salgado, barry: Fancy reviewing an answers branch? https://code.edge.launchpad.net/~allenap/launchpad/just-comment-on-question-bug-114710/+merge/12405
<allenap> salgado, barry: Also, either of you up for a lazr.restful branch? https://code.edge.launchpad.net/~allenap/lazr.restful/james_w/+merge/12414
<salgado> allenap, I can take the latter
<salgado> barry, can you take the former?  if not I'll do it afterwards
<allenap> salgado: Thanks.
<barry> salgado, allenap i can do it
<allenap> barry: Thanks.
* barry changed the topic of #launchpad-reviews to: on call: salgado,barry || reviewing: -,allenap || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<barry> allenap: is this a release-critical branch?
<allenap> barry: No, so put it aside if there's something more pressing to do.
<barry> allenap: cool.  nothing yet, so i should be able to do it
<allenap> barry: I think it'll be a quick one; very repetitive, nothing fancy.
<allenap> james_w: I'm having trouble running the tests in wadllib. Is there any trick to it? They work fine in lazr.restful which appears to be similarly set up.
<allenap> james_w: Hold that; my trunk seemed to have been pointing at the wrong thing.
<james_w> I just ran setup.py test I think
<allenap> james_w: I just commented on the mp for your fix-doctests branch. I think tests seem to run okay without it.
<allenap> salgado: Thanks for the review.
<james_w> I have a suspicion there may have been an alternate fix in the meantime now you mention it
<james_w> it was definitely needed back when I proposed it
<allenap> james_w: Yeah, I assumed as much. Cool. Thanks for the fix anyway :)
<allenap> james_w: BTW, I tried to figure out how we missed the mp for so long. I think most of us use https://code.edge.launchpad.net/launchpad/+activereviews to discover reviews, but we should also look at https://code.edge.launchpad.net/lazr/+activereviews. I'll write to the list about it.
<james_w> thanks
<barry> allenap: any chance you have a screenshot of the changed answer page?
<allenap> barry: I'll get one for you.
<barry> allenap: great!  might as well get you a ui review while i'm at it :)
<allenap> barry: http://people.canonical.com/~gavin/answers.png
<allenap> barry: The only change is the addition of the Add Comment button I think, so not much to see :)
<barry> allenap: right.  question: which do you think will be the most common action?
<allenap> barry: Hopefully Add Answer, then Add Information.
<allenap> s/Add Information/Add Information Request/
<barry> allenap: do you think the buttons should be ordered from most common to least common action?
<allenap> barry: I think they should be ordered from most "disruptive" to least. If someone adds a comment, they can always go and change the status, and the result is similar to pressing one of the other buttons...
<allenap> barry: The same argument could be used to say that we should encourage the workflow to move forwards, so make "Add Answer" or "Add Information Request" more prominent, because the user can always go an set the status back.
<barry> allenap: that's what i'm thinking.  we want to encourage people to answer the question, and putting 'Add Answer' first subtly sends that message
<barry> allenap: so i think: Add Answer, Add Information Request, Add Comment
<allenap> barry: However, the reason I want the "Add Comment" button is to prevent that flip-flopping.
<barry> allenap: that would be my thought anyway
<barry> allenap: the middle one will probably always be the least chosen button
<allenap> barry: That's a good reason. I'm happy to change it.
<barry> allenap: cool.  final question...
<barry> allenap: do you think people will understand the difference between 'add answer' and 'add information request'?
<barry> allenap: iow, do we need a legend or a (?) help popup?
<allenap> barry: Yeah, it could do with a reference. I'm reluctant to actually do it though, because I've already spent far more time on this branch that I ought to have done :)
<barry> allenap: :)  cool.  i'll mention it in my review but let's let users file a bug if they find it confusing :)
<allenap> barry: I like the cut of your jib :)
<barry> my jib likes it too! :)
<barry> allenap: r=me
* barry changed the topic of #launchpad-reviews to: on call: salgado,barry || reviewing: -,- || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<allenap> barry: Thanks :)
<barry> allenap: my future chr'ing self thanks you!
<bac> hi salgadoor barry:  https://code.edge.launchpad.net/~bac/launchpad/retest/+merge/12421 for review at your convenience
<bac> uh, salgado -or- barry
<bac> hi allenap
<barry> bac: i can do it
* barry changed the topic of #launchpad-reviews to: on call: salgado,barry || reviewing: -,bac || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<bac> barry: thanks
<barry> bac: omg, you are my best friend :)
<bac> it's pretty simple but someone had to do it
<barry> bac: review sent
<bac> thanks
* barry changed the topic of #launchpad-reviews to: on call: salgado,barry || reviewing: -,- || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<bac> nice comments barry
<bac> it's hard converting a hacked up script into something others will see without missing a few things
<barry> bac: no worries!
<kiko> james_w, have I missed you?
<kiko> I wanted to ask you why the bzr-nightly-ppa doesn't have karmic uploads -- any reason?
<james_w> kiko: stupidity
<kiko> james_w, heh, mark was asking me about it today and I thought it was just an oversight
<james_w> yeah
<james_w> I'd written the recipes, just not actually included them
<kiko> james_w, heh, okay then :)
<james_w> fixed and triggered
<james_w> thanks to you and Mark
<kiko> james_w, awesome man
<kiko> :-)
<EdwinGrubbs2> salgado, barry: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-436229-distroseries-page-titles/+merge/12439
<salgado> barry, can you take this one?  I'm helping Danny with a shipit branch right now...
<barry> i can
* barry changed the topic of #launchpad-reviews to: on call: salgado,barry || reviewing: -,edwin || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
* salgado-afk changed the topic of #launchpad-reviews to: on call: barry || reviewing: -,edwin || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<barry> EdwinGrubbs: r=me
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: -,- || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<EdwinGrubbs> barry: thanks
<EdwinGrubbs> barry: oh, I assume that I don't get an easy ui=rs now that the rollout has passed and this isn't really an RC candidate.
<barry> EdwinGrubbs: i think that's right.  i gave you a ui=me* and afaik the policy is that you need two ui reviews again.  i'm actually not sure though, so we should talk to beuno about
<EdwinGrubbs> barry: ok, I'll track someone down to do the secondary review if I can't find beuno.
<EdwinGrubbs> beuno: ping
<EdwinGrubbs> rockstar: ping
<rockstar> EdwinGrubbs, hi.
<rockstar> EdwinGrubbs, it looks like you need a ui-review.  Can I do that for you? </paperclip>
<EdwinGrubbs> rockstar: can you do an easy secondary ui review, yes, that's the one.
<EdwinGrubbs> rockstar: are you trying to be clippy?
<rockstar> EdwinGrubbs, something like that.  :)
<rockstar> EdwinGrubbs, link to screenshot and MP?
<EdwinGrubbs> rockstar: I think you need to reduce your helpfullness to do it right.
<EdwinGrubbs> rockstar: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-436229-distroseries-page-titles/+merge/12439
<rockstar> :)
<EdwinGrubbs> rockstar: sorry, I don't have screenshots, but I have direct links to the four pages whose <title> and <h1> have slight changes.
<rockstar> EdwinGrubbs, okay.  It's usually faster when I have screenshots, as I don't  actually have to run your branch.  Just a note.
<EdwinGrubbs> abentley: Here is the branch with a working return value in javascript: lp:~edwin-grubbs/launchpad/bmp-inline
<EdwinGrubbs> abentley: I don't know if you need the date_created off of the IMessage, but that is also not exported. ugh.
<abentley> EdwinGrubbs: Thanks!
<EdwinGrubbs> rockstar: I have to go offline for about ten minutes to drive home.
<EdwinGrubbs2> rockstar: I'm back. Any questions for me?
<rockstar> EdwinGrubbs, nope, r=me
#launchpad-reviews 2009-09-26
* barry changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<jtatum> morning :)
<jtatum> anyone available for reviews of a trivial bug? it's my first time :)
#launchpad-reviews 2010-09-27
<wallyworld> StevenK: ping?
<lifeless> mwhudson: so, will you be up for reviewing my branch?[no panic, just wanting to know if you're 'tagged' or not]
<mwhudson> lifeless: looking at it now, yes
<lifeless> cool, thanks
<mwhudson> lifeless: um TacTestSetup calls Fixture.setUp() twice
<lifeless> heh, fortunately thats idempotent.
<lifeless> fixing
<mwhudson> lifeless: why is that lib/lp/testing/fakelibrarian.py shim still needed?
<mwhudson> lifeless: couple more questions/comments on the proposal, but reviewed
<lifeless> mwhudson: because db-devel has more uses, I suspect
<mwhudson> lifeless: ah ok
<lifeless> I seriously doubt that there is just one use in the entire codebase
<lifeless> IMBW
<lifeless> the docstring in __init__ is a pep8 variable docstring
<lifeless> updated the audit bit
<lifeless> sorry, pep257
<lifeless> "String literals occurring immediately after a simple assignment at the top level of a module, class, or __init__ method are called "attribute docstrings"."
<mwhudson> lifeless: oh right
<mwhudson> i'm not sure that idea ever really took off...
<lifeless> I shall carry the flag!
<lifeless> Until someone (like the friendly pydoctor maintainer) makes it work.
<mwhudson> heh heh
<lifeless> seriously, I can use a comment if you'd prefer.
<lifeless> I do quite like the idea of that being extractable.
<lifeless> particularly as I don't like documenting _ variables as :ivar:
<mwhudson> i think perhaps a comment is more appropriate
<mwhudson> eventually i might get around to the thing i keep meaning to do in pydoctor -- a 'maintainer mode' where _things are not shown by default
<mwhudson> but you can click something and they appear
<wallyworld> StevenK: you around today?
<StevenK> wallyworld: Indeed, what's up?
<wallyworld> hi, i just need you to do a review as per tim's request. i'll paste the link
<wallyworld> https://code.edge.launchpad.net/~wallyworld/launchpad/reassign-reviewer-cancel-option/+merge/36671
<wallyworld> should be a simple one hopefully :-)
<wallyworld> thanks in advance
<StevenK> wallyworld: Looking as soon as Firefox deigns to listen to me
<wallyworld> :-)
<StevenK> wallyworld: Done
<wallyworld> what, already?
<StevenK> wallyworld: It's a very small branch, as you say
<wallyworld> :-)
<wallyworld> thanks
<wallyworld> StevenK: one question. the mp screen says that my original mp request still needs to be claimed, hence the overall review status is "Pending"
<wallyworld> who needs to go and and mark it as approved? me or you?
<StevenK> wallyworld: It's because I can only do 'code*' not 'code'
<wallyworld> what does code* mean?
<StevenK> I'm only a reviwer-in-training
<StevenK> *reviewer-in-training
<wallyworld> oh, ok. i'll wait for tim tomorrow
<wallyworld> thanks for the info. i'm still new at this :-)
<poolie> bac: hi, up yet?
<poolie> is anyone able to review https://code.edge.launchpad.net/~mbp/launchpad/flags-gui/+merge/36415
<jtv> poolie: I'll trade you for https://code.edge.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
* henninge changed the topic of #launchpad-reviews to:  On call: henninge || Reviewing: sinzui || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<poolie> > En passant?
<poolie> i think your description is incompletely translated :-P
<poolie> jtv it seems plausible to me but i've never looked at this before
<jtv> poolie: "In passing."  It's commonly used in chess, I believe.
<poolie> i know
<jtv> Looks like poolie eod'ed.  Can someone else review this simple cleanup for me?  https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
* 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
<jtv> Who's free to review this _easy_ branch for me?  The OCR can't do it due to conflict of interest: https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
<henninge> jtv: you might like this one: https://code.edge.launchpad.net/~henninge/launchpad/devel-update-copyright/+merge/36701
 * jtv makes a quick guess
 * henninge lunches
* henninge changed the topic of #launchpad-reviews to:  On call: henninge || Reviewing: lunch || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: you're saying that rewriting all our source files just before committing them to mainline does not need testing?  :-)
<jtv> Non-OCR reviewer needed!  https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
* 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
<noodles775> salgado: hi! I've done a new demo based on your ui review (for blacklisting distro series differences): http://people.canonical.com/~michaeln/tmp/644328-review-salgado.ogv
<noodles775> Let me know if you've time to take a look.
<salgado> noodles775, sure, checking
<noodles775> Thanks. Just for reference, it's this MP: https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442
<noodles775> salgado: thanks for the review - can you pls add an approve review with the review type as ui*, if you're approving?
<salgado> noodles775, oh, sure, forgot about that
<noodles775> salgado: and I just found that wrapping the radio inputs in an empty form element gets the checked radio displaying too :)
<salgado> cool!
<achuni> henninge: hi! have a minute for a one-byte review? :)
<henninge> achuni: sure, fling it over
<henninge> the byte ;)
<achuni> :D
<achuni> https://code.launchpad.net/~elachuni/launchpad/use-latest-shipit/+merge/36720
<henninge> achuni: r=me ;)
<achuni> henninge: neat, thanks :)
* noodles775 changed the topic of #launchpad-reviews to:  On call: henninge || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi henninge! I'm just updating the windmill test, but if you've time, can you take a look at https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: -,- || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: noodles775,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: Hi!
 * henninge was thinking UI review ... ;-)
<henninge> noodles775: sure, I can do a code review for you ...
<noodles775> henninge: nah, just code, but if you've anything to add to the ui review, feel free :)
<noodles775> Thanks!
<henninge> noodles775: 38	+    """A mixin for classes that share some method implementations."""
<henninge> Oh, really!?
<henninge> ;-)
<henninge> noodles775: I mean, that's what a mixin is all about, is it not? ;)
<noodles775> henninge: line 66 :)
<henninge> ah, hadn't gotten that far ;)
<noodles775> (ie. I didn't change it, just moved it, but can change if you'd like)
<henninge> yes, please
<henninge> "non-sensical comments are useless" ...
<noodles775> Yep, TBH I didn't even read it when moveing the class, but I'll update it.
<henninge> noodles775: "test_is_ajax" should be two test methods
<noodles775> OK, and the same will go for two other tests you'll find later then.
<noodles775> (test_show_blacklist_options and test_blacklist_options_initial_values)
<henninge> noodles775: yup
 * henninge is a fan of the "one assert per test" idea
<noodles775> yeah, same, but in those cases it seemed a bit overboard, but perhaps not.
 * noodles775 notices an import he forgot to move to the top too.
* jcsackett changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: noodles775,- || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> abentley: you free for a review?
<abentley> jcsackett: yes.
<jcsackett> abentley, fantastic! mp is at https://code.edge.launchpad.net/~jcsackett/launchpad/bad-state-transition-641266/+merge/36726
<jcsackett> it's not a very big one. :-)
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: noodles775,jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jcsackett: On line 19, you have """comment""".  Please put an actual docstring.
<jcsackett> abentley: yeah, that would be better, wouldn't it? sorry, forgot i had put in that place holder.
<abentley> jcsackett: I have never seen raise expose( done before, even for exceptions that need to be exposed.  Are you sure you need it?
<jcsackett> sinzui and i discussed it, and it was key to resolving a similar oops.
<jcsackett> abentley: my understanding was that the entire purpose of expose() was to make certain the exception gets raised across the API appropriately.
<abentley> jcsackett: IME you just need webservice_error(httplib.BAD_REQUEST) for that.
<jcsackett> abentley: i'll try making that change and seeing how tests work; assuming everything still passes i'll commit the removal of expose.
<abentley> jcsackett: there are only two uses of the string 'raise expose' under lib/lp.  I bet they're either unnecessary, or used to wrap an externally-defined exception.
<jcsackett> abentley: good to know. test is running right now.
<jcsackett> abentley: results will take a bit longer; need to make build after merging in devel this morning.
<abentley> jcsackett: so the "Unknown status: " exception cannot be encountered over the web service?
<jcsackett> not with launchpadlib--it catches it internally; as the branch explicitly targets the invalid transition over launchpadlib that's what i focused testing on.
<abentley> jcsackett: so it could be encountered with other clients that aren't launchpadlib?
<jcsackett> i would assume; as i said, i felt looking through all of those options was out of scope for the branch/bug.
<jcsackett> that said i'm not entirely against adding additional test coverage; i put this up for MP b/c i thought the scope was met, but i can be convinced otherwise.
<noodles775> henninge: I've just pushed r9811 which updates a windmill test to check the blacklisting UI.
<henninge> oh, ok.
<jcsackett> abentley: without expose() the status is returned as 500, not 400.
<jcsackett> abentley: i have pushed up a change with a docstring in place of the placeholder i missed.
<abentley> jcsackett: in lp/code/model/branch.py BranchMergeProposalExists is raised without expose, and lp/code/stories/branchmergeproposal.txt shows it gives a 400.
<jcsackett> abentley: i'm simply reporting what is happening here. given there is very little complexity to what's going on, it's my intuition that expose is needed here, since otherwise expected behavior doesn't occur.
<jcsackett> i will take a look at branchmergeproposal
<abentley> jcsackett: It would be unfortunate if we had to start wrapping expose() around everything we were raising.  I'd like to get to the bottom of this.
<jcsackett> abentley: it doesn't look like branchmergeproposal is using launchpadlib for its test.
<abentley> jcsackett: correct.  That test pre-dates the use of launchpadlib for testing the API.
<jcsackett> abentley: i've already encountered places where launchpadlib behaves differently--mightn't it be the case that therein lies the issue?
<abentley> jcsackett: launchpadlib shouldn't translate 400 errors into 500 errors, if that's what you mean.
<jcsackett> of course not. i simply mean that as the clearest difference in access, i would guess something is going on there.
<james_w> webservice_error isn't quite enough to do what you want
<james_w> one moment while I find the file
<james_w> ./lib/lp/code/interfaces/webservice.py for why BranchMergeProposalExists returns 400 in that test
<james_w> registry apparently doesn't use the new preferred pattern for that yet
<jcsackett> james_w: i'm not sure i understand; i'm seeing a page of imports, nothing else. is this to get around cyclical import errors?
<james_w> jcsackett: no, it's to make lazr.restful aware of the errors
<james_w> it introspects the classes rather than the obects when they are raised IIRC
<jcsackett> james_w: is the mechanism or requirement for this documented anywhere? i can understand importing the errors, but am unsure why the other imports exist given that explanation.
 * jcsackett is a trifle confused.
<james_w> the other things are things that have exported() declarations IIRC
<james_w> I thought this was written on the wiki somewhere but I can't find it now
<jcsackett> james_w: thanks.
<jcsackett> i'll try creating a similar file and removing expose.
<james_w> lib/canonical/launchpad/interfaces/
<james_w> that's where registry and the other components that haven't created their own file dump all of that stuff
<james_w> code has lib/lp/code/configure.zcml:  <webservice:register module="lp.code.interfaces.webservice" />
<jcsackett> ah ha! that's the magic.
<jcsackett> i will set that up. may be a bit before i can ping back though as i have another branch i need to take a look at.
<jcsackett> abentley: given the above, you may want to put me in Needs Info or similar and get me out of the queue for now.
* henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> sinzui: I did this after my review this morning. If you find it useful, you can approve it ;-)
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/devel-update-copyright/+merge/36701
<sinzui> sweet!
 * henninge has to go now
* sinzui changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: jcsackett || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> abentley, I have two chr related branches for review if you have time today, contact-launchpad-0, contact-launchpad-2
<leonardr> abentley, i'd like your opinion of https://code.edge.launchpad.net/~leonardr/lazr.restful/invalid-uri/+merge/36759
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> sinzui: singular of "emails" is "email" on line 128
<abentley> sinzui: I am confused about the sort order at 190.  (doesn't look alphabetical...)
<abentley> sinzui: nm.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> leonardr: it looks reasonable to me.
<abentley> leonardr: you asked for my opinion, but I'd be happy to approve it as well.
<leonardr> abentley, yes, that was a polite way of asking for a review
<abentley> leonardr: r=me
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* 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
<rockstar> Anyone around for a review?
<rockstar> sinzui? ^^
<sinzui> rockstar, yes
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/edit-recipe-too-new/+merge/36800
<sinzui> rockstar, r=me
<rockstar> sinzui, thanks
#launchpad-reviews 2010-09-28
<wallyworld> https://code.edge.launchpad.net/~wallyworld/launchpad/reassign-reviewer-cancel-option/+merge/36671
<wallyworld> StevenK: ^^^^^^^^^^^^
<wallyworld> thanks ;-)
<poolie> can someone review https://code.edge.launchpad.net/~jameinel/launchpad/lp-service/+merge/35877
<poolie> it's a big performance improvement
<wallyworld> StevenK: ping?
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: just one little thing: I don't think putting an all-caps CURRENT and ALWAYS in test names names is the right thing to do. I'd just keep the names low key ... ;)
<henninge> s/names names/method names/
<noodles775> henninge: as in test_blacklist_options_initial_values_CURRENT ? Right - I initiall was reflecting the enum. I've sent that branch to land, but will updated it in the next.
<noodles775> s/initiall/initially
<henninge> noodles775: that's fine, it's not a big deal.
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv,StevenK] || 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: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* 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
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: poolie || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> gmb: Can I foist a trivial review onto you?
<bigjools> and me please! *2
<gmb> allenap, bigjools: Queue 'em up.
* allenap changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: poolie || queue: [jtv, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> gmb: Danke.
* bigjools changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: poolie || queue: [jtv, allenap, bigjools*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> buckets
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: jtv || queue: [allenap, bigjools^2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: did you change your mind about my branch?
<gmb> jtv: No, but I've had poolie's branch in my queue since Friday
<gmb> Seemed a good idea to get it out of the way first.
<jtv> gmb: ironically, he owes me one.
<jtv> I traded reviews with him, but never got mine.
<gmb> Shocking behaviour.
<jtv> (And at that time he couldn't have known how rough I'd be on his, heh heh)
<gmb> :)
<gmb> jtv: r=me
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: allenap || queue: [bigjools^2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: thanks
<gmb> allenap: r=me
<allenap> gmb: Cheers :)
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: bigjools || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bigjools: r=me * 1
<bigjools> gmb: thanks - the other one errored on the email MP proposal, I just did another by hand
<gmb> bigjools: Yep, looking now. So is this one a correction of the stuff that we rammed in on rollout day?
<gmb> Or am I confusing different 'enabled' fields
<bigjools> gmb: not exactly but nearly - the rollout day change was just the db change
<gmb> Ah, yes. Of course.
<bigjools> I need to tweak the code now
<gmb> Cool
<bigjools> if you need help understanding the test let me know
<gmb> k
<bigjools> you need to know how arch-all publishing works to appreciate it :)
<gmb> bigjools: The test makes sense to an extent but for the sake of completeness... how does arch-all publishing work ? :)
<bigjools> gmb: although the binary is only built in one architecture, it has to be published everywhere
<bigjools> the code I tweaked calculates the other archtectures for publishing
<gmb> Right.
<gmb> bigjools: Pretty much as I thought, then.
<bigjools> ok cool
<gmb> bigjools: r=me; looks good
<bigjools> thanks gmb
<gmb> np
* 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
<bigjools> ah wgrant is stalking me again
<wgrant> bigjools: Yeah, sorry.
<wgrant> I didn't see it was being discussed in here.
<bigjools> wgrant: I'll fix copying separately
<wgrant> Hmmm.
<wgrant> I suppose.
<bigjools> it could do with refactoring
<wgrant> I'm also slightly worried about letting new binaries through the publisher unchecked. But I guess that's not a huge problem.
<bigjools> anyway the urgent case is the upload one
<bigjools> unchecked?
<wgrant> I think I'd prefer it if the publisher whinged if it tried to publish binaries in a disabled arch.
<wgrant> This is messy :(
<bigjools> what about the case where you want to delete?
<wgrant> Hm?
<bigjools> actually, hmmm, getPendingPublications doesn't look for that
<bigjools> it could whinge but it's not necessary - preventing creating publications is the important bit
<wgrant> I guess we can fix any screwup there later.
<wgrant> It's the indices that are the important bit.
<wgrant> Since they can never change after release.
<bigjools> yes
<wgrant> But copies need to be fixed soon, since there'll be SRUs possibly even before release.
<wgrant> Hrrrrrm.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: vittles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* 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 changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> gmb: can you please review the extra revision I added to the second branch you did for me earlier?
<bigjools> it adds another check - yay for duplicated code logic. (bug filed BTW)
<gmb> bigjools: Sure.
<gmb> bigjools: What's the MP URL again?
<bigjools> gmb: https://code.edge.launchpad.net/~julian-edwards/launchpad/no-disabled-arch-publications-bug-648715/+merge/36838
<bigjools> cheers
<bigjools> whoever in the Code team added the partial diff attachment processing, you're totally awesome
<gmb> +1. That's the first time I've seen that.
<bigjools> it's been there for a while
<gmb> bigjools: Yeah, but no-one ever does any work round here.
<bigjools> Lancaster?
<bigjools> :)
<gmb> Touche.
<gmb> bigjools: r=me on the changes.
<bigjools> gmb: thanks muchly
<gmb> np
<abentley> gmb, leonardr: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/incremental-diffs/+merge/36875 ?
* abentley changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: - || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> abentley, sure
<abentley> leonardr: thanks.
* leonardr changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> gmb or leonardr: time for a review?
<leonardr> jcsackett, put it in the queue
* jcsackett changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: abentley || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> leonardr: you got it. MP is https://code.edge.launchpad.net/~jcsackett/launchpad/bad-state-transition-641266/+merge/36726
<leonardr> jcsackett: and give me the mp url
<jcsackett> one step ahead of you. :-P
<leonardr> abentley, all the fields in IIncrementalDiff are exported, but you don't call export_as_webservice_entry(). do you want these objects to be published through the web service?
<abentley> leonardr: I guess so.
<abentley> gmb, leonardr: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/incremental-diff-job/+merge/36881 ?
<leonardr> abentley: try running the webservice tests with your branch as-is and see if you get any failures. i'm curious.
<gmb> abentley: I'll take a look at that branch for you.
<abentley> leonardr: How do I do that?
<abentley> gmb: thanks.
<gmb> ... after I've looked at jcsackett's. Argh; spent too much time looking at tests and am now all turned about.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: jcsackett, abentley || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: abentley || queue: [jcsackett, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: jcsackett, abentley || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> abentley: bin/test -vvt webservice
<leonardr> abentley: even if the existing webservice tests pass, i suspect your branch will break as soon as someone does something to retrieve a diff from the web service, and the diff they get happens to be an incremental diff
<abentley> leonardr: No, I don't think so.  Incremental diffs aren't diffs, and don't have any URL at all.
<leonardr> abentley: in that case, i don't think you need to call exported() on their fields
<abentley> leonardr: So either I should go all the way and export incremental diffs over the API, or I should drop the export calls?
<leonardr> abentley: exactly. export() is you telling lazr.restful which parts of the incremental diffs should be visible
<abentley> leonardr: Fair enough.  I'll probably export them, just because sooner or later, we'll get bug reports if I don't.
<leonardr> abentley: bug reports saying "export these pls"?
<abentley> leonardr: yeah.
<leonardr> abentley: ok, i think you should do that in a separate branch
<leonardr> since you'd also have to give an incremental diff a url
<abentley> leonardr: okay.
<leonardr> abentley, is an incremental diff really a 'thing' with an independent existence? it seems more like the product of an algorithm. that doesn't preclude giving it a url, but it might make the work more complicated
<abentley> leonardr: You could look at it as merely cached data.
<abentley> leonardr: Same for any diff, I guess.
<leonardr> abentley: at this point we're beyond a review of this branch, but if you get into this, maybe we can talk about how existing diffs are published in launchpad-dev?
<abentley> leonardr: sure.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: abentley, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> abentley: can you explain this line? IMasterStore(IncrementalDiff).add(incremental_diff)
<leonardr> are you caching the diff in the database for later?
<leonardr> and what's the difference between IMasterStore.of() and Store.of()? it looks like one takes a class and the other an instance?
<abentley> leonardr: I am storing the value in the database.  We cannot generate incremental diffs on demand.  We will generate them in a cron job
<abentley> leonardr: IMasterStore(class) returns the writable store that is meant to be used for that class.  Store.of(instance) returns the store the the instance was retrieved from.
<leonardr> abentley: ok, that does give the incremental diff an independent existence
<abentley> leonardr: export removed, diff is updated.
<abentley> leonardr: s/export/expose
<leonardr> abentley, line 292, i think the end quotes need to be on their own line
<abentley> leonardr: done.
<leonardr> going through tests now
<gmb> abentley: r=me on the incremental diff job branch with a few small changes requested.
* gmb changed the topic of #launchpad-reviews to:  On call: leonardr || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> abentley, i think in test_generateIncrementaldiff there are several multi-line loc you could combine and stay under the character limit
* sinzui changed the topic of #launchpad-reviews to:  On call: leonardr || Reviewing: abentley || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> gmb: thanks.
<sinzui> leonardr, I have a branch the refactors our rdf page.
<leonardr> and i could really use some comments explaining what all these branches have to do with each other. it's tough to figure out just by looking
<leonardr> sinzui: ok, send me the mp
<sinzui> https://code.edge.launchpad.net/~sinzui/launchpad/rdf-links-1/+merge/36865
<leonardr> abentley: or some more expressive commits--right now all the commits are nearly identical except for one or two single-character differences
<leonardr> i think it would help a lot to have a comment explaining the original state of the precursor branch, the state of the prerequisite branch, and then the changes made by the new branch
<abentley> leonardr: I was only able fix one multi-line loc.
<leonardr> abentley: ok, i must have misjudged. what do you think of the comment idea?
<abentley> leonardr: Sure, I can do that.
<leonardr> a way to make it simpler without comments would be to have a method that both creates a branch and does a commit
<abentley> leonardr: http://pastebin.ubuntu.com/502197/
<abentley> leonardr: half of those commits are merges, though.
<leonardr> abentley: that makes it a lot clearer
<leonardr> i don't know if the method that creates branch and does commit would help much, but it might help distinguish between the branches as they were "before", and the branches that get changed "during" the test
<leonardr> abentley: a couple nosy questions about difftacular, which i see you wrote yourself during july (?)
<leonardr> how hard would it be to make an egg of it? i get the impression we prefer eggs to sourcedeps
<leonardr> granted that you have tests in difftacular, but did you get someone to review that code?
<abentley> leonardr: impossible.  bzr plugins cannot be provided as eggs.
<leonardr> ok
<abentley> leonardr: I did not get someone to review that code.
<abentley> leonardr: I'm sorry, but I need to go now to meet someone for lunch.
<leonardr> abentley: no problem. i'll approve this mp with your changes, contigent on getting difftacular reviewed. i can probably review it today if it's not too complicated, since i need to leave some mps for a trainee who's ocr tomorrow
* leonardr changed the topic of #launchpad-reviews to:  On call: leonardr || Reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> sinzui, just out of curiosity, have people been complaining about the rdf and the owl?
<sinzui> leonardr, none directly...they cannot find it. We have circuitous complaints that RDF is not documented
<sinzui> oh, Google knows about it
<sinzui> bots don't complain though, they just 404
<leonardr> sinzui, obvious typo: "it's" in line 204
 * sinzui can fix that
<leonardr> sinzui: it looks like there was a problem where both the rdf spec and the link to it were called "rdf". was that the actual source of the problem (like a conflict) or was it just something that looked confusing so you fixed it?
<EdwinGrubbs> abentley-lunch: I didn't get the job runner cron script ready in time for your OCR day. Do you have time to review it now, or do you know someone else who is familiar enough to handle it?
<sinzui> leonardr,  that was part of the problem. browser resource loses the mimetype. lazr folder fixes that
<sinzui> leonardr, I think it was first broken because the director or zcml moved and the publisher could not find the dir.
<leonardr> sinzui: i see
<leonardr> ok, r=me
<abentley-lunch> EdwinGrubbs: I can look at it now.
* leonardr changed the topic of #launchpad-reviews to:  On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> abentley-lunch: when you have time, i've looked through the difftacular codebase and i have some questions at various granularities
<abentley> leonardr: sure.
<leonardr> abentley: my biggest problem is that i don't understand what ignore_heads does
<leonardr> it looks like it takes one branch, and a bunch of other branches, and returns a bunch of revision ids in the first branch that can be ignored?
<abentley> leonardr: it's the changes introduced by those revisions that can must be ignored.
<leonardr> abentley: how about iter_lca_revision? i don't know what an lca is, but i'm guessing this is what strips out the revisions to be ignored?
<abentley> leonardr: an LCA is a Least Common Ancestor.  It is an ancestor shared by both revisions that has no descendants that are also ancestors of the revisions.
<leonardr> so the revisions included in the diff are the lcas of the original revision and all the revisions being ignored?
<abentley> leonardr: we don't really diff revisions.  We generate two trees and diff those.
<abentley> The "new" tree is the tree for the "new_revision".
<abentley> The "old" tree is generated by merging the changes from the ignore_branches that were merged into the new_revision into the old_revision as well.
<EdwinGrubbs>  abentley: thanks, here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-jobqueue-cron-script/+merge/36910
<leonardr> ok, that's similar to what i was trying to say
<leonardr> let me take another look at the tests now that i understand ignore_heads
<abentley> EdwinGrubbs: I am looking into it, but I am also multitasking pretty hard.
<EdwinGrubbs> abentley: no problem, I need to get some lunch anyway.
<leonardr> abentley: why does ignore_heads return an empty list in test_ignore_heads_already_merged? is it because the merge happened in rev2, not between rev2 and rev3?
<abentley> leonardr: it is because those branches haven't been merged into bar.
<leonardr> oh, they were merged into foo and you checked bar
<leonardr> no, i'm confused again. 'repo' is foo.branch.repository
<leonardr> you're generating a diff between foo rev2 and foo rev3
<leonardr> and you're ignoring bar
<leonardr> but bar was merged into foo during rev2, so it doesn't show up
<leonardr> that makes sense to me, but it's not what you're testing?
<abentley> leonardr: sorry, misread it.
<abentley> leonardr: yes, your original idea was right.
<leonardr> ok
<leonardr> abentley: hopefully the final question before the suggestions
<leonardr> what exactly is going on in the unignored merge scenario?
<leonardr> i suspect it's not unignored on its own, but that you use it in a way where bar.branch is not ignored where it would otherwise be ignored
<leonardr> or, you create a new branch, merge it into foo, and its changes are picked up while bar's changes are ignored
<abentley> leonardr: I don't understand what "unignored on its own" would mean.  Everything depends on how you invoke the diff generation.
<leonardr> abentley: i don't think "unignored on its own" has any meaning, but "create_unignored_merge_scenario" implies that it does
<leonardr> i'm trying to figure out what is special about the scenarios where you call that functino. such that the "unignored" branch is not ignored
<abentley> leonardr: I think it means that the merge would not be ignored if you did a normal diff.
<leonardr> abentley: in the two tests where you use it, the merge is *not* ignored. you ignore bar instead, and the "d" line shows up in the diff
<leonardr> which makes sense, given the name. i'm just not sure whether that's what you're testing
<abentley> leonardr: I'm testing that when the branch isn't in the list of branches to ignore merges from, its merges aren't ignored.
<abentley> leonardr: unlike mainline_diff, which will ignore such merges.
<leonardr> ok, i get it now, but it's confusing that the tests are called "ignore_x" and you're checking that only *certain* x are ignored
<leonardr> abentley: for lack of a better place to put these comments, i'm going to append them to my review of your launchpad branch
<abentley> leonardr: okay.
<leonardr> abentley: i've approved your launchpad branch. let me know when you do a difftastic follow-up and i'll review that
* leonardr 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> leonardr: the preview tree must be used as the input to the next merge.  Otherwise, you're not doing proper merges.
<leonardr> abentley: ok, i missed that you were reassigning tree
<abentley> EdwinGrubbs: "for child in children: child.wait()": doesn't this mean that you can't re-run a job source until all job sources have terminated?
<abentley> EdwinGrubbs: I was thinking that we would exit after all the JobSources had been forked, not after they all completed.
<EdwinGrubbs> abentley: I guess it does. The problem I was having is that if the parent process doesn't wait for the children to exit, when you run it manually, the child processes continue to spew output to the terminal. One solution to this problem would be to not have the parent process use a lockfile, since the child processes already have their own lockfiles.
<abentley> EdwinGrubbs: All the output should be redirected to logfiles anyhow.
<abentley> EdwinGrubbs: Waiting doesn't prevent the children from spewing output to the terminal, it just means they won't do it while you're using the terminal for something else.  Which isn't really a risk for cron scripts.
<EdwinGrubbs> abentley: To prevent it from becoming more complicated to gather the output in the tests, I think I should add a command-line option so the tests can force it to wait, but I'll try running the tests without the waiting to see if that works at all.
<EdwinGrubbs> I meant "spewing" to indicate that the output was coming at an annoying time. I actually wanted the children's output, but I just didn't want to be surprised by it.
<sinzui> bac: those stories do not ready like a story. should we be concerned that foo bar is being called an owner in a story?
 * sinzui thinks stories need an owner/contributor browser and that is Sample Person
<bac> sinzui: yes, it required more set up
<sinzui> who owns applets?
<bac> sinzui: i welcome ideas on how to make it looks less mechanical
<bac> sinzui: either foobar or sample
<abentley> EdwinGrubbs: Why do you default runner_class to JobRunner when an unknown runner class is specified?
<sinzui> The code team have been turning their mechanical tests into unittests that still use testbrowser
<bac> sinzui: foobar owns applet
 * sinzui bangs head on a wall
<sinzui> The whole test is using a god to manage an upstream project
<bac> sinzui: i didn't intend for this to spiral.  i made requested UI changes and broke the world.  and then backfilled.
 * bac bad
<sinzui> bac: right. I am thinking how do we stop hurting ourselves with this.
<abentley> EdwinGrubbs: What about phrasing it like this? http://pastebin.ubuntu.com/502299/
<sinzui> bac: did you check for test duplication? maybe we can delete instead of add to make it pass
<EdwinGrubbs> abentley: oh, that was left over from the old solution. It handled defaulting to JobRunner if it didn't exist, but getattr() wouldn't default to JobRunner if it was set to null. Although, it's unlikely that someone will specify the field without filling it in with something valid, so I could probably go back to just using getattr() and let it blow up later if the config is invalid.
<bac> sinzui: no, i didn't look carefully for duplicates.  being unfamiliar with the code tests i'm thinking that would take a while
 * sinzui takes a quick look
<EdwinGrubbs> abentley: that's a nice rewrite. I'll use that.
<abentley> EdwinGrubbs: I think it's better if it blows up the value is invalid.
<sinzui> bac: code/browser/tests does not appear to intersect
<sinzui> oh, the list is nice
<bac> what list?
<bac> sinzui: ^
<sinzui> bac: I like the listification of the hosting options
<abentley> EdwinGrubbs: Do you need NoErrorOptionParser?  Can't you just blow up in ProcessJobSource.__init__?
<bac> sinzui: yes, +1 to mrevell for the suggestion
<sinzui> bac: I have been thinking about privacy too long. I keep seeing context/displayname and thinking that will raise a 403
<EdwinGrubbs> abentley: the problem is if I want to pass "-v" into ProcessJobSource, the OptionParser in the __main__ section needs to ignore it.
<sinzui> bac: you cleaned up a generator, but 'v' is does not conform to lp standards
 * sinzui argued for 1-char vars in comprehensions
<bac> sinzui: i'll join you in that argument
<bac> but for now i'll fix it
<abentley> EdwinGrubbs: But if you do the argument parsing in ProcessJobSource.__init__, you'll have the real parser available to you.
<bac> sinzui: and then have to split the line, resulting in it being more unreadable
<sinzui>  bac: branch-listing.pt may render faster if we used "sprite branch" instead to using an <img> element 75 times
 * sinzui was confused
<bac> sinzui: IRC hide-n-seek?
<EdwinGrubbs> abentley: yes, but I can't get the real parser until after I parse the JOB_SOURCE name out of the command line arguments, since that is necessary for looking in the configs to find out which runner class to pass to ProcessJobSource.__init__().
<sinzui> maverick has enabled tap on my magic mouse :(
<abentley> EdwinGrubbs: You mean which job source class to pass to ProcessJobSource.__init__() ?
<sinzui> Bac: this paste looks good. I do not see what can be done about the tests. Given the age and value of the branch, I think it is ready to land.
<EdwinGrubbs> abentley: sorry, I mean that ProcessJobSource.__init__() would be looking up the runner class, which it would be passing to JobCronScript.__init__()
<bac> sinzui: i've made sprite change and the generator change.  thanks.
<abentley> EdwinGrubbs: So I think the simplest thing is to override runner_class after you've done JobCronScript.__init__()
<sinzui> bac: you have my r for code.
<bac> sinzui: and UI?
<sinzui> That too, am I stepping on salgado though?
<bac> sinzui: i'll send this off to ec2 for testing and wait for salgado to reply.
<sinzui> bac: yes please
<mwhudson> salgado is not working today, i think, if that's relevant
<EdwinGrubbs> abentley: ok, it looks like I just have to override config_name and dbuser also, but init_zca doesn't get called in the __init__(), so I can get rid of the extra parser.
<bac> mwhudson: will he be around tomorrow?
<mwhudson> bac: hopefully
<abentley> EdwinGrubbs: have you considered providing a list of crontab_groups in [process-job-source-groups] and then providing configuration for each group?
<abentley> EdwinGrubbs: I think then you wouldn't need all_job_sources or grouped_sources.
<EdwinGrubbs> abentley: my main concern then is that a job class could become a member of multiple groups. Even if the cronscript would spit out an error when that happened, it would make the config file less convenient to use, and the config file will be edited a lot more than the cronscript's code.
<abentley> EdwinGrubbs: fair enough.
<abentley> EdwinGrubbs: could you please add documentation to your tests?  Each test is supposed to have at least one line.
<abentley> EdwinGrubbs: is there a reason why test_setstatus_silent and test_setstatus_admin differ by more than the silent=True parameter?
<EdwinGrubbs> abentley: ugh, another problem with not passing in the job_source to __init__() so that we can re-use its option parser is that the job_source is used to determine the config_name, and the config_name is passed to LaunchpadCronScript.__init__() as the name it uses for the cron control file, which determines whether the script runs at all.
<abentley> EdwinGrubbs: so my meta-suggestion is "if the way things fit together is getting in our way, let's fix it instead of routing around it or doing double handling".
<EdwinGrubbs> abentley: I'll add some comments to the tests. setStatus(silent) can only be done by a Launchpad admin, so I made extra changes for that restriction after I had already finished test_setstatus_admin(), which just refers to the team admin.
<abentley> EdwinGrubbs: since you're doing admin = person_set.getByEmail('admin@canonical.com') you can use login_person instead of login, if you like.
<lifeless> also, if I can suggest
<lifeless> from lp.testing.sampledata import ADMIN_EMAIL
<abentley> EdwinGrubbs: Have you tried using canonical.launchpad.scripts.tests.run_script?
<EdwinGrubbs> abentley: no, I just copied the first test for a cronjob that I found.
<abentley> EdwinGrubbs: Perhaps I should have gone back and fixed all the tests to use that when I wrote it, but I was not very aggressive back then.
<EdwinGrubbs> abentley: regarding your meta-suggestion, I think it will all work better if I move all the setup logic from __init__() to run() in all the parent classes. The only downside is that it will create the lockfile briefly before it exits if the cron crontrol file tells it not to run.
<EdwinGrubbs> abentley: I'll switch to run_script.
<abentley> EdwinGrubbs: that sounds reasonable.
<abentley> EdwinGrubbs: (moving the setup logic to run) sounds reasonable.
<abentley> EdwinGrubbs: It's past EOD for me.  Can we pick this up tomorrow?
<EdwinGrubbs> abentley: sure, talk to you later
<abentley> EdwinGrubbs: cool.
#launchpad-reviews 2010-09-29
<thumper> mwhudson: ping
<mwhudson> thumper: otp
* StevenK changed the topic of #launchpad-reviews to:  On call: StevenK || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/branch-distro-avoid-scan/+merge/36945
<thumper> mwhudson: trivial 3 line review
<StevenK> Argh, I can't review that :-)
<thumper> StevenK: well you could
<thumper> StevenK: but it is super trivial
<mwhudson> i can mentor the review i guess
<mwhudson> thumper: oh haha, so the branches on disk were ok but the db was wrong?
<thumper> mwhudson: yep
<mwhudson> oops
<thumper> wasn't too bad
<thumper> just a tweak to avoid the scan
<thumper> mwhudson: I'm looking at writing a script to fix up the stacking of the maverick branches that are stacked on lucid
<mwhudson> thumper: do we have any idea what happened there?
<thumper> not yet
<thumper> I was thinking I should talk to james_w to ask
<thumper> but was too busy this morning
<mwhudson> thumper: branch reviewed
<thumper> ta
<thumper> wallyworld__: ping
<wallyworld__> thumper: pong
<thumper> wallyworld__: skype
<jtv> stub: I've completed my follow-up.
<jtv> Sorry, poolie.
<jtv> Not stub.
<poolie> jtv, any chance of yet another review?
<jtv> poolie: arrh!  Well, I _am_ supposed to be OCR today (though being lazy because of health issues)
<jtv> poolie: or do you mean, another round of the same one?
<poolie> another round of the same one
<poolie> it actually got simpler, and perhaps smaller
<poolie> if you can count it as OCR that would be a great use of the role
 * jtv looks
<jtv> poolie: what is a "relevant" feature controller?  What are the irrelevant ones?
<jtv> I guess the "workaround for bug 631884" comment explains itâfor as long as we keep that comment.
<_mup_> Bug #631884: multiple variables pointing to FeatureController get out of sync easily <feature-flags> <qa-untestable> <Launchpad Foundations:New for lifeless> <https://launchpad.net/bugs/631884>
<jtv> thank you mup, you're always there when we _don't_ need your help :)
<poolie> :)
<poolie> "the one you should look at"
<poolie> i could change it to "for request" or "for thread"
<poolie> but i wasn't sure i wanted to commit to that
<poolie> perhaps if you pass nothing it must be per thread in some way
<jtv> poolie: given that it's complex, I can understand your reluctance.  But it shows that there's often more good material for a docstring than you'd expect!  Then, in line 74 of the diff (the __all__ in lib/lp/services/features/browser/edit.py), there should be a comma.
<poolie> k
<jtv> poolie: btw are all fields required to be nonempty?
<jtv> (I'm guessing yes)
<poolie> yes
<jtv> That's good to know.  And I guess the parser reads multiple spaces as a single separator?
<poolie> yes
<jtv> Is that covered by tests?
<poolie> yes
<poolie> test_setAllRulesFromText
<jtv> Ah.  It's implicit.
<jtv> (And still weirdly indented)
<jtv> Illustrates how quickly you can have too much data and verify too many things in one test.
<jtv> If you feel inclined to point out to me that I'm a fascist reviewer, rest assured that I've been told this before.
<poolie> ah, i am feeling a bit impatient with this at the moment
<poolie> in bzr, i feel that it's more important to build up the quality of the contributors than the quality of their early patches
<poolie> i don't mean to sound snarky
<poolie> what i mean is that it's useful to describe the rules so people will use them automatically next time
<poolie> more so than fixing up stuff that's unidiomatic
<poolie> it's interesting nobody picked up on my camelCase functions in the earlier landings in this theme
<jtv> poolie: ah!  My apologies.  I've known you as a team member so long that it never really occurred to me that you've been working more in a different codebase.
 * lifeless doesn't care particularly about camel-or-not
<poolie> this is about my 6th lp patch ever
<lifeless> it seems inconsequential because we work with heterogenous stuff
<poolie> certainly the biggest so far
<jtv> And I've been assuming that you've heard all this before.
<jtv> My mistake.
<poolie> np
<poolie> i appreciate being told
<lifeless> so enforcing it in LP simply makes the inclination to do stuff that should be out of LP in LP even stronger
<poolie> maybe my 10th patch; certainly less than the 20th
<jtv> All in the same ballpark.  :-)
<jtv> lifeless: about the camel (or dromedary, I guess, what with the one missing hump), you intended at one point to take the difference with PEP8 to the list.
<jtv> lifeless: did you ever get around to that?
<lifeless> jtv: I did, I'm frying one fish at a time.
<lifeless> the current one is review size limits
<poolie> lifeless: you want to push them down or dispense with them?
<jtv> lifeless: I must have missed it.  :(  Anyway, my reaction to what you said was to accept either dromedary or lower-with-underscores for methods; but afaik dromedary for stand-alone functions is wrong either way, right?
<poolie> i care more about the elapsed time than the number of lines
<lifeless> poolie: I want to remove them from the scope of reviews.
<lifeless> poolie: I think its a valueless overhead to worry about
<poolie> surely it is in the submitter's best interests to do small patches anyhow
<jtv> lifeless: I do see our lives being appreciably better now that we have the limit though.
<poolie> if they're enlightened enough to realize it
<lifeless> well
<poolie> though there are overhead factors that push people away from doing small landings, istm
<jtv> The limit is more a reminder than a limit, in practice.
<lifeless> line count in the diff is a poor surrogate
<lifeless> for anything.
<poolie> right
<lifeless> there are two questions, in my mind.
<lifeless> a) do we need a 'limit or negotiate' clause at all
<lifeless> b) if we do, whats the right metric.
<lifeless> I have points on both a) and b)
<lifeless> for a), assuming we have a perfect metric that isn't a poor surrogate
<poolie> also, and more urgently
<jtv> there's a wide gap between "perfect metric" and "not a poor surrogate"
<poolie> any other feedback, jtv?
<jtv> poolie: yes, I was letting lifeless finish, but now that you ask:
<lifeless> jtv: sure, assuming we have a non-poor surrogate
<lifeless> not all reviewers are equally fluent across the code base
<jtv> poolie: you use check_permission to see if the poster is permitted to make the change.  I'm not very confident on this one, but ISTM you can just leave that to Zope permissions checks.
<lifeless> and across the domains
<poolie> jtv i wondered about that too
<poolie> it does seem unnecessary
<poolie> bit of belt-and-braces there
<lifeless> so the metric will need to be specific to the reviewer, which seems pretty awkward
<poolie> i could just remove it
<jtv> lifeless: to me, 800 lines works pretty wellâboth to justify effective time management for the OCR and to remind the developer that it's time to cut off.
<lifeless> jtv: well, that talks to b)
<jtv> poolie: yes, I'd test (if it's not tested already; can't recall just now) and then remove the check
<lifeless> jtv: staying with a), I think the primary thing is that patches be large enough to be confident in the change and small enough to be confident in it.
<poolie> why would anyone want to do a large patch?
<lifeless> poolie: say they were rewriting the http stack for lp
<jtv> Sometimes there's significant overhead to breaking things up in smaller self-contained chunks.
<poolie> there may be reasons, but it would be interesting to know what they are
<lifeless> it will be very hard to be sure that the direction is right until you've migrated everything across
<poolie> right
<lifeless> but there is lot of risk in changing that at all, so doing it twice is undesirable
<lifeless> which is one of the forms of overhead jtv refers to
<poolie> assuming arguendo that's true, having done it, you might as well land it
<jtv> Or consider what you've done a proof-of-concept.
<poolie> right, or perhaps it is useful to see if you can improve just one area
<poolie> but it would depend on the case
<jtv> Conversely it can also happen that things get clearer and better just from the act of re-doing it in chunks.
<lifeless> so, I'm saying that the pragmatic reality is:
<lifeless>  - patches smaller than $metric sometimes get asked to be rearranged
<poolie> jtv, my thing does have tests for access control so i'll remove the check
<poolie> hm, well, there was one thing there
<jtv> poolie: great, then the tests will tell you whether we're making a mistake.  :-)
<poolie> it was about trying to give read access more broadly than write access
<lifeless>  - patches larger are routinely accepted when from trusted individuals or by particularly confident reviewers
<poolie> however it seems like ~registry has launchpad.Edit on ILaunchpadRoot anyhow
<lifeless> having a policy that doesn't match reality adds to the learning curve for new contributors
<poolie> i need to go and cook soon
<lifeless> so we should make it match reality, make reality match it, or just nuke it.
<lifeless> for b), I don't think there is a good nonsurrogate for 'how complex and risky a change is'. Plenty of one lines will pass tests but utterly break LP.
<poolie> jtv, so that's the point of it
<lifeless> plenty of 1000liner patches will just be pure and wonderful
<jtv> poolie: as long as you test { anonymous, unprivileged, edit, admin} Ã { view, modify} it should be safe to trust zope with the whole thing
<lifeless> e.g. don't trust zope at all
<lifeless> this is a bug in our security framework that such tests are needed at all.
<poolie> do people actually bounce patches for being too big?
<jtv> Shouldn't normally happen, but if the reviewer doesn't have much time or isn't very confident, it gives them a reason.
<poolie> well, i added them because i didn't trust my understanding of it
<lifeless> I've had a 200 line one deferred to another reviewer
<jtv> Also, we're not supposed to chuck oversized branches on the queue without first discussing it with the OCR.
<lifeless> but most of my branches are > 800, and reviewers generally take about 5 minutes to review them.
<lifeless> This is one of the reasons I think the metric is poor; its clearly possible to write textually long changes that are easily understandable
<jtv> But that's rarely been an issue.  Plenty of branches get submitted as "this branch is over the limit, _but_ it's mostly deletions" (or sampledata updates, etc.)
<poolie> jtv ok, so
<poolie> i've cleaned up the formatting and added a comment
<lifeless> anyhow, its late, I have a 4am start to get to sydney tomorrow
<poolie> i think the permission check is at most harmless
 * lifeless waves
<poolie> i know!
<poolie> go to bed, see you tomorrow
<jtv> \o lifeless
<poolie> we can have lunch with steph if that suits your schedule
<lifeless> poolie: to avoid confusion; you're staying at home, we'll get to st leonards on our own steam.
<poolie> right
<lifeless> poolie: I'll sms you when I'm in st leonards.
<poolie> great
<poolie> fly well
<lifeless> thanks
<lifeless> lunch could be fun
<lifeless> I'm with you  / at the urban all thurs
<poolie> jtv, i pushed a few more changes, if you're not unhappy with the overall impact could you land it for me?
<poolie> i don't have pqm access atm
<jtv> poolie: OK; got a few more small things such as "x.get('foo') or ''" â "x.get('foo', '')"
<poolie> that's not the same thing
<jtv> But that shouldn't stop it from landing.
<poolie> in the case of {a=None}
<jtv> oic
<poolie> nice unicode though :)
<jtv> <compose> - >
<jtv> Waited for years for that one to be supportedâ¦
<jtv> Still no Dutch "ij" ligature though.  :(  It's a letter.  It's capitalized as one: IJsselmeer.
<jtv> To type Å egan, use <compose> c S
<poolie> mm i know
<poolie> it's a shame there's no good ui to discover these things
<jtv> Ooh yes these tests are much nicer
<poolie> other than googling 'compose key'
<poolie> ok, really going now
<poolie> thanks very much for hte reviews
<jtv> np
<jtv> cya
* noodles775 changed the topic of #launchpad-reviews to:  On call: StevenK || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> StevenK: hi! I'm guessing you're not really still reviewing, but if you're keen for one: https://code.edge.launchpad.net/~michael.nelson/launchpad/649599-ajax-comment-on-dsdiff/+merge/36966
<StevenK> noodles775: I'm so not, sorry
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> StevenK: no worries, enjoy your evening!
* allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775, allenap] || 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: [noodles775, allenap, StevenK] || 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: noodles775 || queue: [allenap, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Hi jelmer; another one for your queue if that's okay with you.
* gmb changed the topic of #launchpad-reviews to:  On call: jelmer || Reviewing: noodles775 || queue: [allenap, StevenK, gmb(http://bit.ly/gmb-bw-branch)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> gmb: np
* jelmer changed the topic of #launchpad-reviews to:  On call: jelmer || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch), jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> jelmer, EdwinGrubbs: when you have time to get to mine in the queue, MP is here: https://code.edge.launchpad.net/~jcsackett/launchpad/registry-errors-649836/+merge/36994
* sinzui changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch), jcsackett, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> jelmer: I must have used the wrong branch with bzr lp-propose-merge (although I thought I used db-devel).
<noodles775> bzr lp-propose-merge lp:~launchpad-pqm/launchpad/db-devel <-- was what I used, not sure why it ended up targeted to devel.
<noodles775> Ah, it was the first of two. nm.
<noodles775> Thanks for the review.
<EdwinGrubbs> StevenK: are you around?
<salgado> sinzui, bdmurray, the continue/cancel links don't work if you access the page on the main vhost
<bdmurray> salgado-lunch: ah! so from launchpad.dev not bugs.launchpad.dev?
<bdmurray> salgado-lunch: okay, I've recreated it now and will fix it.  thanks
<leonardr> jelmer, edwingrubbs, i'm putting a simple branch into the queue
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/474522-type-error/+merge/37018
* leonardr changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch), jcsackett, sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap, jcsacket || queue: [StevenK, sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> bac: would do you have time to read an apocalyptic branch https://code.launchpad.net/~sinzui/launchpad/apocalypse-interface-imports-3/+merge/37032
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap, sinzui || queue: [StevenK, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui: yes
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: jelmer, EdwinGrubbs || Reviewing: allenap, leonardr || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui: i need to work on a [testfix] so your code review will have to wait
* jelmer changed the topic of #launchpad-reviews to:  On call: EdwinGrubbs || Reviewing: leonardr || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui: do you think the example.zcml had no purpose?
<sinzui> I never used it. Did you know it existed?
<bac> sinzui: yeah, i'd seen it a time or two
 * sinzui find children
<bac> sinzui: seems like it could have utility, unless it requires a lot of upkeep
<sinzui> bac, where could I move the file? We have ignored it for 5 years
<bac> sinzui: ok, delete it.
<abentley> rockstar: I can has review? https://code.edge.launchpad.net/~abentley/launchpad/test-feature-flags/+merge/37061
<rockstar> abentley, ack.
<rockstar> abentley, r=me
<abentley> rockstar: thanks.
#launchpad-reviews 2010-09-30
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: - || Reviewing: leonardr || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* EdwinGrubbs 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
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/bug-627701/+merge/37094
<lifeless> if anyone is up for that, it will allow very specific timeouts.
<mwhudson> lifeless: "def _get_request_timeout" ignores the 'now' parameter
<mwhudson> in fact, given what it seems to do, the tests of it could be precise
<mwhudson> abs(x-y) < 0.001 always creeps me out
<lifeless> mwhudson: floats creep me out
<rockstar> lifeless, even the root beer kind?
<lifeless> grah yes ;)
<thumper> root beer is awful
<lifeless> mwhudson: so was that 'please land with the now parameter removed'
<lifeless> mwhudson: or 'please find another reviewer I've gone to get beer'?
<rockstar> thumper, I've had what kiwis call "root beer." It is a bad forgery.
<lifeless> ok, looks like I needs a reviewer for https://code.edge.launchpad.net/~lifeless/launchpad/bug-627701/+merge/37094 (updated)
<lifeless> mwhudson: if you do return, and see this, please clickyclicky ;)
* noodles775 changed the topic of #launchpad-reviews to:  On call: noodles775 || Reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> StevenK: ah, this is the same branch that we were chatting about earlier... should I wait, or start working through it?
<noodles775> StevenK: btw, have you tried `bzr lp-propose-merge`? It will create a pretty handy template for your MPs.
<StevenK> noodles775: There's two: https://code.edge.launchpad.net/~stevenk/launchpad/classy-packagecloner/+merge/36963 is the one in the topic
<noodles775> GReat, I'll start on that one then :)
<noodles775> StevenK: I'm just looking at the class-packagecloner branch - did you have a pre-imp. call with someone (bigjools?) about it? I don't understand why you're wanting to use instance variables to store state on a utility...
<StevenK> noodles775: I mentioned it to Julian, yes. It just seems ... messy
<noodles775> StevenK: yes, but I don't think storing state for an individual call on a shared utility object is safe? Remember, two separate callsites might be using the utility at the same time (which is why - I assume - all the params were being passed in).
<StevenK> noodles775: The only entry point is still clonePackages() and they get their own instance, don't they?
<noodles775> StevenK: no, getUtility(IPackageCloner) will return the same object each time (AFAIUI).
 * noodles775 checks the source for getUtility.
<StevenK> noodles775: Okay, if that's the case, then yes, this branch is a bad idea.
* allenap changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: StevenK, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Morning noodles775 :)
<noodles775> Hi allenap!
* StevenK changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> danilos: https://code.edge.launchpad.net/~henninge/launchpad/recife-bug-611674-convert-imports/+merge/37108
<henninge> danilos: I still have to push lint changes
<henninge> pushed ;)
<danilos> henninge, thanks, I'll take a look at it as soon as I am back, got to get out now :)
<henninge> danilos: np
* allenap changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: -, Edwin || 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: noodles775, allenap || Reviewing: -, Edwin || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi henninge, can I interest you in another UI review? http://people.canonical.com/~michaeln/tmp/649559-blacklisting-with-comments.ogv
<noodles775> Actual MP here: https://code.edge.launchpad.net/~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2/+merge/37114
* noodles775 changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: StevenK, Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: sure, but I have to finish something first.
 * henninge watches screencast right away, though ... ;)
<noodles775> henninge: no problem (or if you're busy, salgado will be around a bit later too). Thanks.
<noodles775> Ah :)
<henninge> noodles775: hm, aren't comments always sorted from oldest to newest in LP? We should stick to that.
<noodles775> henninge: I wasn't sure in this case - it is more like a wall... as a user, the information I want is the most recent comments. But it's easy to switch.
<henninge> noodles775: also, in the same vein, "Add comment" is usually placed at the end of the list of comments.
<henninge> noodles775: Did I mention that this looks great! ;-D
<henninge> noodles775: sorry for starting off with the negative things ... ;)
<noodles775> henninge: yes, if the comments are going oldest to newest, then of course it would be at the end. It's only there because I did it more like a wall.
<noodles775> No problem! Thanks for the feedback.
<henninge> noodles775: well, I don't see how this case is different from other commenting we have in LP
<noodles775> henninge: The biggest difference, IMO, is that we're displaying lists of comments for multiple objects on the one page. Ideally, if I had more time, we'd only be showing the 5 latest comments for each (in the details section).
<henninge> noodles775: mp's have a "Add a review or comment" button on top of the comments that scrolls you down to the end.
<henninge> dunno why bugs don't.
<henninge> noodles775: I am not opposed to introducing a "wall" concept for short comments - but they should look different to mark the difference in behaviour.
<noodles775> james_w: if you're around and have a minute, would you also prefer standard oldest to newest comments for the differences (see above screencast). I've currently got them with the newest first, assuming that's the info that will be needed, but it's easy to switch back.
<noodles775> henninge: If the heading was "Most recent comments" instead of "Comments" (and later we added a link to the full details for a particular difference with all comments) would that help differentiate?
<noodles775> (er, and we only showed the 5 most recent on this page?)
<henninge> noodles775: That would probably help but I still think people will get confused about the ordering.
<henninge> noodles775: I actually think the reverse ordering is preferrable
<noodles775> henninge: yeah, perhaps. Let's see what james_w or other people who will be using this say. I'm happy to change it - it's an easy switch.
<henninge> noodles775: I don't know if it has been discussed before but maybe you could post your screencast to the list and start a discussion to adopt this for all our comments?
<henninge> but maybe that is not always wanted ...
<henninge> hm
<noodles775> henninge: no, I think a merge proposal is a story... it should be ordered from oldest to newest.
<noodles775> The few comments that will ever be used for these differences are things like "Waiting for version 2.5".
<henninge> noodles775: I put that all in a review comment. Not approved until we decide on a way to go with this.
<henninge> noodles775: but thanks a lot for the great work! ;-)
<noodles775> henninge: yep, that's great (and I don't mind which way... just what works for the people using it). Thanks!
* henninge changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: StevenK, Edwin || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775, allenap: I have some mechanical removal of "canonical.launchpad.interfaces" imports here: https://code.edge.launchpad.net/~henninge/launchpad/devel-remove-c-l-i/+merge/37116
<noodles775> oooh, exciting :)
* allenap changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: StevenK, henninge || 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: noodles775, allenap || Reviewing: StevenK, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: lunch, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to:  On call: noodles775, allenap || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<james_w> noodles775: I think newest first makes some sense here, but I don't know if consistency should win out
<noodles775> OK, thanks james_w - let's see what the ui reviewers (henninge and sinzui) decide.
* noodles775 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
<noodles775> sinzui: Hi! When you've time, can you pls take a look at the following (no rush): https://code.edge.launchpad.net/~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2/+merge/37114
 * sinzui will in 20 minutes
<noodles775> Great, thanks sinzui
* allenap changed the topic of #launchpad-reviews to:  On call: allenap || Reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> EdwinGrubbs: a tiny follow-up to the branch you reviewed yesterday. i added a doctest that benji wrote to complement the pagetests:
<leonardr> http://pastebin.ubuntu.com/503270/
<leonardr> take a look and i'll land the branch
* 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
* rockstar changed the topic of #launchpad-reviews to:  On call: allenap, rockstar || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> sinzui: I'm happy to email the dev list regarding activity-streams etc., but I need to keep working on the next branch. Would you be ok if I re-order the comments and put the add-comment link at the bottom as henninge suggested, so that I'm not blocked on this?
<sinzui> noodles775, yes
<noodles775> Great, thanks.
<EdwinGrubbs> leonardr: I'm looking at it now.
<leonardr> k
<henninge> noodles775, sinzui: I agree that this is an acceptable temporary solution. ;)
<noodles775> Great, thanks henninge
<sinzui> yes, I really think reverse listing for activity are what users want
<EdwinGrubbs> leonardr: looks good. r=me
<noodles775> Yes, me too.
 * rockstar relocates to coffee shop
* jcsackett changed the topic of #launchpad-reviews to:  On call: allenap, rockstar || Reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> rockstar, allenap: https://code.edge.launchpad.net/~jcsackett/launchpad/remove-cache-private-650668/+merge/37143
<jcsackett> i believe it's a two line diff.
<allenap> jcsackett: It says 566 :)
 * jcsackett boggles.
<jcsackett> allenap, that can't be right; let me see what i screwed up. :-P
<noodles775> probably targetted to db-devel instead of devel (or vice-versa)?
<jcsackett> allenap: i targeted the wrong branch. please take a look at this much smaller, much better MP: https://code.edge.launchpad.net/~jcsackett/launchpad/remove-cache-private-650668/+merge/37145
<jcsackett> :-P
<jcsackett> noodles775 was correct.
 * noodles775 has done the same thing multiple times :)
<jcsackett> honestly, i'm surprised i haven't done that before.
<allenap> jcsackett: +1
<jcsackett> thanks, allenap.
<rockstar> allenap, is jcsackett still in the queue?
<jcsackett> rockstar, nope, i'm not.
<allenap> rockstar: Ah, no. Done.
* rockstar changed the topic of #launchpad-reviews to:  On call: allenap, rockstar || Reviewing: - || 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: rockstar || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Cheerio rockstar, have a good OCR stint.
<rockstar> allenap, thanks!
 * rockstar takes the reins
<james_w> rockstar: perhpas you would take a look at https://code.edge.launchpad.net/~james-w/bzr-builder/fix-text-of-nest-parts/+merge/37080 ?
<james_w> it's an important bugfix for launchpad as well
<rockstar> james_w, I'm amazed that no one took you up on this last night.
<rockstar> Oh, nevermind.  It's a superseded merge.
<rockstar> james_w, I can't set the mp status to Approved.
<rockstar> Also, apparently I'm a community vote.
<james_w> rockstar: I'll add you to ~bzr-builder-devs?
 * rockstar joins team.
<james_w> there we go
<james_w> thanks for the review
<james_w> rockstar: do you want a release to get these things in to LP?
<rockstar> james_w, I think abentley is already working on that.
<james_w> great
 * rockstar lunches
<benji> rockstar: I'm about to knock off, but want to throw https://code.edge.launchpad.net/~benji/lazr.restful/tweak-etag/+merge/37194 at you before I go.
* benji changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-10-01
<thumper> wallyworld: better to add a comment than edit the description
<thumper> then it becomes part of the conversation
<wallyworld> thumper: ack
 * thumper goes to make coffee before attacking error messages
<wallyworld> any one up for a review? https://code.edge.launchpad.net/~wallyworld/launchpad/windmill-1.3r1544/+merge/37209
<wallyworld> thumper: before you EOD, assuming your branch fixes are done, did you want to look at review-without-reviewer and hopefully approve?
 * thumper will try
 * wallyworld says thanks as he migrates to the coffee machine for a hit
* rockstar 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
* adeuring 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
<noodles775> Hi adeuring, would you mind looking at this branch (it's got a bit of js): https://code.edge.launchpad.net/~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2/+merge/37114
<adeuring> noodles775: sure, I'll look
<noodles775> Thanks!
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: I've just updated the MP with some details (things I wasn't happy about code-wise, but can't see a way around).
<adeuring> noodles775: thanks!
<adeuring> noodles775: r=me; 1 nitpick
<noodles775> Great, thanks adeuring
* adeuring 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
<allenap> jelmer: How's the review of lp:~allenap/launchpad/structural-subscriptions-with-filters going?
<jelmer> allenap: sorry, I suck. I have the review here, just not sent out yet. Let me dig it up.
<allenap> jelmer: Cool, thanks :)
<adeuring> allenap: I think the importance/status JOINs in your branch lp:~allenap/launchpad/structural-subscriptions-with-filters-2 are wrong
<adeuring> see a comment in the MP
<allenap> adeuring: Cool, thanks.
<allenap> adeuring: There's some code earlier in getSubscriptionsForBug() that tries to select the relevant bug task.
<adeuring> allenap: ah, right, I missed that! sorry for the noise
<allenap> adeuring: No worries, thanks for looking at it.
<adeuring> allenap: r=me
<allenap> adeuring: Thanks!
<sinzui> jml: https://code.launchpad.net/~sinzui/launchpad/move-binaryandsourcepackagename-0/+merge/37258 is an apocalypse branch
* sinzui changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> adeuring, I have a refactoring to clean the tree ^
<adeuring> sinzui: I'll look
<adeuring> sinzui: r=me
* adeuring 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
<allenap> adeuring: Fancy a branch?
<adeuring> allenap: sure
<allenap> adeuring: https://code.edge.launchpad.net/~allenap/launchpad/structural-subscriptions-with-filters-3/+merge/37270
<allenap> Thanks :)
<adeuring> allenap: r=me
<allenap> adeuring: Thank you :)
<malaria> Hi, I would like a review of this: https://code.launchpad.net/~malaria/launchpad/fixes-bug-608631/+merge/37287
<malaria> adeuring, perhaps?
<adeuring> malaria: its already EOD for me...
<malaria> adeuring: ok, at least I tried ;)
* 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 abgeÃ¤ndert
* You're now known as ubuntulog
<jcsackett_> anyone available to review this? https://code.edge.launchpad.net/~jcsackett/launchpad/kill-uses-launchpad-for-650648/+merge/37252
<jcsackett_> it's pretty much all deletions; templates, some code, and stories.
<jcsackett_> sinzui: any chance you might have time for a review today? i got sidetracked and haven't been able to martial this through in a timely fashion: https://code.edge.launchpad.net/~jcsackett/launchpad/kill-uses-launchpad-for-650648/+merge/37252
<sinzui> I can
<sinzui> let me finish fixing a botched merge first
<jcsackett_> sinzui: take your time; i won't be seeing it land till the weekend anyway. thanks!
<sinzui> jcsackett_, ping
<jcsackett_> sinzui: pong.
<sinzui> i think you can delete more from xx-product-launchpad-usage.txt. I see a check for +edit in th test. Products never use that page. maybe this whole test is not needed since Involvement replaces it
<jcsackett_> sinzui: i was actually thinking about that, but wasn't sure.
<sinzui> well, I see some Involvement tests in it, but this is not a story. I see no mention of who is changing the state and why
<sinzui> This is a dubious test
<jcsackett_> sinzui: i agree. i'm happy to remove it.
 * jcsackett_ thinks.
<jcsackett_> though i'll need to verify good involvment tests first.
<sinzui> yes, verify it. This looks like state tests. the tests for the view that changes the state are very complete so we only care about explaining that an owner can enable/disable involvement links from applications using the menu or a link from the application root page.
<jcsackett_> sinzui: dig. i'll do that.
<sinzui> I think project group also showed usages
 * sinzui looks at lp-project
<jcsackett_> sinzui: I'm not seeing anything like that on the mozilla overview page.
<sinzui> We really need to remove the bug tracker from project group. you can set it, but it does nothing :(
 * jcsackett_ nods.
<sinzui> jcsackett_, this looks good to land. Please review that test, I think its has lost its raison d'etra
<jcsackett_> sinzui: i will.
<jcsackett_> thanks.
#launchpad-reviews 2010-10-03
<lifeless> mwhudson: hey
<lifeless> lp:~lifeless/launchpad/test
<lifeless> has some more work in it just fixing ec2 detected failures.
<lifeless> more stale-librarian processes mainly.
<lifeless> I'm sending it in to find more, but would like to know if the recent commits are clear to you
<mwhudson> lifeless: wow, branch of doom
<lifeless> mwhudson: I knew this would uncover a pile of mess.
<lifeless> and it is
<lifeless> its very clear to me why we were ending up with 4-5 librarians running mid-test-suite-run
<lifeless> :)
<mwhudson> yeah
<mwhudson> lifeless: the changes look basically fine
<mwhudson> lifeless: does LaunchpadScriptLayer.tearDown need to be conditional like that?
<lifeless> well
<lifeless> unregisterProvider returns False if it did not unregister.
<lifeless> it might not unregister because:
<lifeless>  - its not registered
<lifeless>  - something prevents it being unregistered
<lifeless> the former might happen if setUp wasn't called/blew up
<lifeless> or if the base object got replaced with an empty registry.
<lifeless> or the mailbox object got replaced.
<mwhudson> lifeless: i wonder if noisly exploding would be the thing to do there?
<mwhudson> (i'm not trying to ask leading questions, just encourage discussion/thinking)
<lifeless> eventually, perhaps
<mwhudson> lifeless: i guess it's not for this branch, but "ls --versioned --recursive --null --kind file | xargs -0 fgrep 'Setup().setUp()'" show more work to be done at some point :-)
<mwhudson> lifeless: ok, i'm fine with the branch as-is, i hope there's not too much more to go before landing :-)
<lifeless> mwhudson: yeah
<lifeless> mwhudson: me too :P
<lifeless> I'm only doing this cause I want parallel testing
<lifeless> mwhudson: so we could raise LayerInvarient
<lifeless> (sp?)
<mwhudson> yeah, something like that
<lifeless> I'd like to work on NotImplemented as a thing separately
<mwhudson> (sorry, brain not working today)
<lifeless> firstly I want to make it stateful
<lifeless> then unique instances
<lifeless> then parallel
<mwhudson> lifeless: 'it' ?
<lifeless> then cleanup
<lifeless> the test environment
<mwhudson> ah ok
<mwhudson> lifeless: a problem that i've not yet had any good ideas about is the problem of hard coded paths/ports/etc in configs for tests that launch subprocesses
<lifeless> mwhudson: See my thread where I describe how to fix that. I'd love a critical eye. So far its been 'mgmt' saying 'wow lovely' :P
<mwhudson> lifeless: subject?
<mwhudson> i have a feeling i only skimmed that mail
<lifeless> stories for ...
<mwhudson> ah right
<mwhudson> lifeless: ok i replied to the mail
<mwhudson> lifeless: slightly incoherently, sorry about that
