#launchpad-reviews 2009-09-11
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: - ,shipit || queue: []
* sinzui changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: - ,shipit || queue: [sinzui]
<sinzui> adeuring: salgado-lunch: My MP will arrive shortly. I need to take a lunch to my daughter, I'll be away about an hour
<adeuring> sinzui: I'm a bit worn-out (18:40 local time)... salgado-lunch, can you take it?
<sinzui> adeuring: thanks. I am sure you are worn from all the small things in my last branch
<adeuring> sinzui: ;)
<sinzui> Barry may be available in a few hours
<salgado> adeuring, sinzui, sure, I'll take it
* salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: - ,sinzui || queue: []
* adeuring changed the topic of #launchpad-reviews to: on call: salgado || reviewing: sinzui || queue: []
<gary_poster> salgado: adding https://code.edge.launchpad.net/~gary/launchpadlib/lp-dev-utils/+merge/11607 to your queue, if that's ok.
* gary_poster changed the topic of #launchpad-reviews to: on call: salgado || reviewing: sinzui || queue: [gary]
 * gary_poster is being hopeful ;-)
<salgado> gary_poster, definitely.  I'll get to it after sinzui's
<gary_poster> thank you!
<sinzui> salgado: I'm taking a look at your person branch now.
<salgado> thanks sinzui 
<salgado> sinzui, I think the solution to my problem is in the branch I'm reviewing now
<sinzui> salgado:  I think so to
<salgado> just put all the <dl>s inside a single div class="two-column-list"
<sinzui> salgado: the dl's are conditional in one div
<sinzui> salgado: +1
<sinzui> salgado: note that the really long items are not in the div
<salgado> yeah, that's what I initially did to the email addresses in that contact-details portlet
<sinzui> salgado: I am moving the Contact this user in to the action menu. I think Edwin already reported a bug about this.
<salgado> sinzui, is it not a problem that the link text might be really long?
<salgado> in some cases, that is
<sinzui> hmm
<sinzui> right
<salgado> I guess it's only really long when you're contacting a team
<sinzui> indeed, that was edwin's concern
<sinzui> salgado: person-portlet-contact-details.pt is not used by team.
<salgado> right, but it'd be nice to be consistent about where we put the link to contact people/teams
<salgado> although we're not consistent about plenty of other things there, so there's no need to worry about this
<salgado> 664	+ tal:define="when python: bugtask.date_closed or
<salgado> 665	+ bugtask.date_fix_committed or
<salgado> 666	+ bugtask.date_inprogress or
<salgado> 667	+ bugtask.date_left_new"
<salgado> sinzui, how about turning that (^) into an IBugTask property? :)
<sinzui> oh, I forgot about that hack
<sinzui> salgado: I was contemplating adding it to the decorated bugtask that is used for prenstation in lists
<salgado> yeah, that sounds better
<sinzui> salgado: *I* want that there, no user as ever asked for this feature
<sinzui> I'll update the decorator class in a short while.
<sinzui> salgado: the best this about this new design is that the map does not fit in columns when the browser is set at 1027 wide.
<salgado> sinzui, why is that a good thing?
<sinzui> I cannot put the map in a column near the top of the page ;)
<sinzui> I do think the timezone should be near the email address though
<salgado> where it used to be in 1.0
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: gary || queue: []
<gary_poster> salgado: adding another one to queue.  You'll like this one: three lines changed, and our make run is no longer overwhelmed with zc.zservertracelog blather: https://code.edge.launchpad.net/~gary/launchpad/tracelog/+merge/11618
<sinzui> salgado: I have some improvement for your branch. I am tempted to make some changes to the working on section. Do you have any uncommitted work on that portlet?
* gary_poster changed the topic of #launchpad-reviews to: on call: salgado || reviewing: gary || queue: [gary]
<salgado> sinzui, nope
<salgado> gary_poster, is that to compensate for the 1500 lines diff of the other branch? ;)
<salgado> maybe that's why the MP has no diff -- it's above the limit (800 lines) so the script doesn't even bother generating it
<gary_poster> salgado: lol, yeah I guess so.  I hope you don't feel like you have to review that all, though: it is just moving files from lp-dev-utils that have already been reviewed
<gary_poster> salgado: no, I, um, have, ah, been over the 800 line limit once or twice before. :-)
<gary_poster> and the diffs showed up
<salgado> yeah, I was just kidding
<gary_poster> :-) my branches tend to get big sometimes :-/
<salgado> you're not alone there
<salgado> I have one which was at nearly 1000 lines when I sent to sinzui, and he's been doing lots of changes in it, so I imagine it will come back to me a lot bigger
<gary_poster> heh
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: gary || queue: []
<sinzui> salgado: I will happily review it
<salgado> gary_poster, anyway, r=me on the first one
<gary_poster> salgado: actually, the diff for https://code.edge.launchpad.net/~gary/launchpad/tracelog/+merge/11618 looks odd
<gary_poster> I didn't make those changes :-/
<gary_poster> That diff should only have the .conf changes
<gary_poster> doublechecking
<salgado> to me it looks big
<salgado> huge, actually
<gary_poster> Yeah it should be three lines like I said :-/
<salgado> let me try merging the branch
<gary_poster> ok.  This is what I expected: 
<gary_poster> https://pastebin.canonical.com/22049/
<gary_poster> salgado: ah-ha
<gary_poster> salgado: I forgot to change the merge target
<salgado> oh, right, it's targeted to db-devel
<gary_poster> should be devel not db-devel
<gary_poster> right
<gary_poster> sorry about that
<salgado> no worries
<salgado> r=me on that too
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: []
<gary_poster> cool thanks :-)
<salgado> gary_poster, I didn't notice it was against db-devel because the UI says it's against "lp:launchpad", but that's because we have db-devel configured as the development focus
<gary_poster> right, me too
<salgado> when devel was the development focus, it was obvious when a BMP's target was db-devel
<gary_poster> and you had to make an explicit gesture for the unusual thing, while the usual thing was the default.  that was nice. ;-)
<salgado> indeed
<salgado> gary_poster, but if you use 'bzr send' it should always create BMPs against devel, no?
<salgado> at least that's what it does for me
<gary_poster> salgado: ah, I have not yet migrated to that new wonderfulness.  I do it through the web right now.  I should probably look up how to set that up...
<gary_poster> (through the browser I mean, of course)
<salgado> you should -- bzr send works great
<gary_poster> ok cool
<gary_poster> salgado: you hadn't officially approved the merge into db-devel so I took the liberty of deleting the MP and creating a correct one to devel: https://code.edge.launchpad.net/~gary/launchpad/tracelog/+merge/11623 .  If you want to approve now, the diff is now much more sane ;-)
<salgado> that looks like something I'd approve. :)
<gary_poster> heh
<salgado> done
<gary_poster> thanks
* salgado changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []
#launchpad-reviews 2010-09-13
* lifeless 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
<lifeless> thumper: got 2 minutes?
<thumper> yeah
<lifeless> incremental eyeballs on a  branch of mine
<lifeless> just need to know if one commit is sane
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/oops/revision/11529
 * thumper looks
<lifeless> well, I know its sane, but maybe I'm wrong :P
<thumper> fcking loggerhead
<lifeless> basically I missed some stuff and ec2 found it for me
<thumper> lifeless: I'm pretty sure you don't need the ... on a line between Traceback (most recent call last): and the exception itself
<thumper> lifeless: it is handled by the doctest
<thumper> lifeless: but up to you if you feel like removing it
<lifeless> oh, ok, I was following suite
<thumper> yeah
<thumper> I don't think many people know about that
<thumper> and I don't think we have a standardised practice
<lifeless> shrug
<lifeless> doctest spec says it ignores it or something
<lifeless> I don't think standardising that is going to be helpful
<lifeless> thumper: so its ok?
<lifeless> mwhudson: ping
<lifeless> mwhudson: https://code.edge.launchpad.net/~lifeless/launchpad/memcache/+merge/35220 you approved but you didn't comment, ec2land hates that
<lifeless> (probably suffered in the timeout mess we had)
<lifeless> jtv: I'm guessing mwhudson has EOD'd, perhaps you could be a proxy for him ?
<jtv> lifeless: what's it about?  I'm stuck in the rain using batteries and a GSM, so hard to load entire LP pages.
<lifeless> making memcache controlled by feature flags
<lifeless> its been reviewed by mwhudson
<jtv> but not landed?
<lifeless> but he forgot the vote bit :P
<jtv> I can land that when the rain stops and I get home.
<lifeless> so its marked approved, has a comment from him saying approved, but no -vote-
<StevenK> No one has claimed the review
<lifeless> StevenK: you don't need to claim
<lifeless> just do a review, and if you're in the team you effectively claim it
<jtv> StevenK: we had a bug for a while where you had to claim the review before you could do it, but that's been fixed.
<jtv> StevenK: can you take this one then?
<StevenK> thumper: Are you around to rubber-stamp too?
<StevenK> lifeless: That should be good now?
<lifeless> thanks
<StevenK> lifeless: Now read my PM, damn it :-P
<lifeless> did you have a question in there?
<lifeless> I thought it was just commentary ;P
<StevenK> I was expecting at least a comment on the commentary :-)
<lifeless> sorry, magic 8 ball says ask again later
<henninge> jtv: good morning!
<jtv> hi henninge!
<henninge> jtv: you are not on irc.c.c
<henninge> ;-)
<jtv> :(
<jtv> I guess it's something this provider blocksâ¦ _sometimes_
<jtv> Or at least, it used not to.
* 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
<henninge> strange
<jtv> no, it's technically illegal for me to connect to the internal IRC server (and due to contradictory laws, there's no legal way for the provider to let me connect to any chat at all)
<jtv> I'll set up an illegal proxy.  Hang on.
<henninge> :-D
<henninge> jtv: I'll come visit you in jail ...
<jtv> Not sure that's allowed.
<henninge> ... good reason for a sprint in Bangkok.
<jtv> They make you pay extra for the right to sleep on the floorânot sure sprint facilities are in the cards without serious bribery.
<henninge> We'll expense it.
<henninge> So, jtv, how is life in Mordor?
<jtv> henninge: well the first thing you notice is this enormous black cloud hanging over it
<jtv> I don't care much for the constant nazgÃ»l patrols either.
<henninge> yes, that does get your spirits down.
<henninge> I hear the black gate is about to open!
<henninge> but you should not leave that way, there's a huge human army waiting on the other side.
<noodles775> stub or lifeless: time to look at a schema MP? https://code.edge.launchpad.net/~michael.nelson/launchpad/627957-differences-schema-update/+merge/35073
<stub> noodles775: versien isn't spelled that way
<noodles775> hrm... "base version to derived versien." I'll update it.
<stub> noodles775: So source_version and parent_source_version are debian version strings? If so we might want a CHECK constraint to ensure that.
<stub> ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK (valid_debian_version(source_version));
<noodles775> stub: yes they are. Thanks.
<stub> noodles775: With the new UNIQUE constraint, you can drop the distroseriesdifference__derived_series__idx index.
<stub> noodles775: The name of the new UNIQUE constraint should be distroseriesdifference__derived_series__source_package_name__key
<noodles775> stub: Great - I didn't realised the two-column index makes the single-column foreign-key index redundant. And yep, I'll rename the constraint.
<stub> So a NULL diff means it hasn't been calculated yet I assume. What do NULL versions mean? Probaly worth clarifying this in comments.sql
<noodles775> stub: yes, different types of differences... (unique to the derived series, missing from the derived series etc.). Should there be a constraint to ensure that both are not null together?
<noodles775> stub: er, ignore that (I was thinking about the versions fields).
<stub> If that is what our code expects, then yes. ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid...
<stub> ok
<stub> noodles775: patch-2208-12-0.sql
<noodles775> *sigh*. I need a tea. So yes, you had asked about NULL versions, so we should have a constraint to ensure both versions are not null.
<noodles775> Thanks.
<stub> If only one or the other can be NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (source_version IS NULL != parent_source_version IS NULL)
<noodles775> Great, thanks.
<stub> If we just want to check at most one is NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (coalesce(source_version, parent_source_version) IS NOT NULL)
<stub> or just 'source_version IS NOT NULL OR parent_source_version IS NOT NULL' is better
<noodles775> Yep.
<stub> I'm not sure which of those two rules is appropriate
<stub> Can they both be NULL? Can they both be NOT NULL?
<noodles775> They cannot both be null, but they can both be NOT NULL.
<stub> ok... so CHECK (source_version IS NOT NULL OR parent_source_version IS NOT NULL)
<jcsackett> salgado, ping.
<salgado> jcsackett, hi there.  let me guess, it's about that UI review? :)
<jcsackett> yup. :-) just making sure you knew about it.
<salgado> jcsackett, I do, yes.  haven't done it yet because I was off last Thursday and Friday, but will do it shortly
<jcsackett> salgado. very cool. thanks!
<adeuring> henninge: could you review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-581626/+merge/35281 (short, simple, boring)
<henninge> adeuring: looking
<adeuring> thanks!
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> adeuring: "The text for each bigs ..." is meant to be "bugs", I guess.
<adeuring> henninge: ouch... yes, or course
<henninge> adeuring: also, the for loop cound be a function that you can use twice, e.g. "print_bug_links"
<henninge> s/cound/could/
<adeuring> henninge: make sense, I'll add this function
<henninge> adeuring: with that: r=me ;-)
<adeuring> henninge: thanks!
* 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
<salgado> jcsackett, did you see that your branch has a few conflicts?
<salgado> nm, I reloaded the page and see that they're now fixed
<jcsackett> salgado: phew. i was confused for a sec--was sure i had done an update this morning. :-)
<bac> jml: can you review https://code.edge.launchpad.net/~bac/launchpad/cherrypick-633926/+merge/35286  for a cherry pick?
<jml> bac, sure. after this call.
<bac> jml: thanks
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge 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
<jtv> abentley: got time for one just under the review limit?  It's this one: https://code.launchpad.net/~jtv/launchpad/templates-listing/+merge/35185
<abentley> jtv, sure.
<jtv> cool, thanks
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jtv, did you end up testing this with chameleon?
<jtv> abentley: no, just compared to devel
<abentley> jtv, do you think it's likely that chameleon will soon become our template engine and invalidate this work?
<jtv> abentley: I don't know about "invalidate"âif chameleon would be faster than this, then maybe.  But would we be enthusiastic about moving conditionals into the TAL?
<abentley> jtv, you said "If the new template engine turns out to be faster than straight-through python code, then we should probably scuttle this branch."
<jtv> Yes.  Do you think it will be?
<jtv> Because the speedup I'm seeing is not to be sneezed at.
<jtv> I think it can go one of two ways:
<abentley> jtv, I don't know.  You suggested that was possible, and it seems like a risk to me.  AIUI, chameleon is compiled to pyc, so its performance is equivalent to python.
<jtv> (a) We hold this branch back and wait, and continue to suffer performance problems and continue to hate the logic and model access we put into the TAL, and _maybe_ chameleon will solve the performance part.
<abentley> jtv, I am not sneezing.  I am looking at the issues that have been raised about this code and trying to determine their status.
<jtv> abentley: I hope I don't sound contrary.  You're raising valid points and I'm trying to give you a useful answer.
<abentley> jtv, I don't have any idea yet of whether we should merge this.
<jtv> (b) We land this branch, and then later maybe find that chamelon is super-fast in which case we move stuff back into TAL, but I think keeping some things in the view to keep the TAL nice and readable.
<abentley> jtv, I don't know how feasible it is to try it out in chameleon, even if it is in tree.
<jtv> I have no idea eitherâ¦  one thing though: adi and danilo did say that a lot of the overhead was seemed to be in link formatters rather than in TAL per se.
<jtv> Scratch the "was."
<jtv> But they identified the worst time sinks at the time, and addressed those.  After that I suppose the picture become more diverse, as it goes.
<abentley> jtv: in the b) case, I always worry about whether there will be follow-up.  If there isn't we get tech debt.
<jtv> True.  And in this case I think we have it either way: browser code that could be in TAL, or TAL code that could be in the view.
<jtv> But I think the tech debt only happens if either chameleon turns out to be blazingly fast (since tonight I think it's not going to match the kind of speedup I got) or we want some change that matches very poorly with my browser code.
<jtv> I'm using "since" in the purely temporal sense here.
<abentley> Okay, I'm comfortable with that.
<jtv> Note that the main loop is still TAL, so we may still get _some_ speedup from a faster templating engine.  ;-)
<jtv> I'm being flippant.
<jtv> Note to self: don't do that in reviews.
<abentley> jtv, since the purpose of constructTemplateURL(self, template) is to allow optimization, would it help to take a list of templates and return a list of URLs?  That way you could do it in one database hit.
<jtv> abentley: cool idea!  But the templates are already fetched at that point.  Adi & Danilo took care of that.  Only 11 queries for the entire page!
<abentley> jtv, Ah.  So it's not generating lots of links?  Or it's generating lots of links but without lots of DB cost?
<jtv> abentley: the latter.
<jtv> Fetches all that information in one go.  There'll be a bit of Storm work to patch up the references, but that's it.
<abentley> jtv, Do you know why canonical_url is slow?  Would you lose your performance gains if you used the ObjectFormatter directly?
<jtv> abentley: permissions checks seem to be the main culprit.
<jtv> That's why I also wanted the "context/required:launchpad.AnyUser" out of there.  I don't know whether that will "walk the tree."
<jtv> (from the current object up to the site root, that is)
<jtv> canonical_url is recursive, and could possibly do with some caching for this sort of repetitive work.
<jtv> I say "possibly" because I think it'd have to be one of those ultra-small, catch-one-common-case caches.
<jtv> Which is easy for me to say, but who knows if it actually works out (and faster than not doing it)
<abentley> jtv, what's the purpose of getUserAndEmail?
<abentley> jtv, I mean, _makeUserAndEmail
<jtv> abentley: to create a user that I can log in as
<abentley> jtv, You can log in as anyone you create with factory.makePerson().
<jtv> So small that it's not worth moving to a factory, but it saves just enough that it's useful for the test
<jtv> Can I just do login(person.email)?
<abentley> jtv, You know about login_person and "with person_logged_in"?
<jtv> No
 * jtv greps
<abentley> jtv, lp.testing, I believe.
<jtv> yup, found it.  I'll use that fortwith.
<jtv> forthwith.
<jtv> I'll also pull the login out of _makeView
<abentley> jtv, cool.
<jtv> abentley: change pushed.
<abentley> jtv, r=me
<jtv> abentley: \o/ thanks!
* jcsackett changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: jtv || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> jtv: https://code.edge.launchpad.net/~jcsackett/launchpad/no-canonical-urls-628247/+merge/35301
<jcsackett> jtv, ignore me, i misread the header. :-P
<jcsackett> abentley, instead i must bother you with the above. :-)
<abentley> jcsackett, no bother.
* abentley 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
<abentley> jcsackett, Could you explain the conditional in lib/canonical/launchpad/interfaces/validation.py, please?
<jcsackett> abentley: sure, that's sort of the crux of the fix. previously in that file it just fetched an email address and then the email address's Person and constructed the error message in the last clause (the else block). the bug this is fixing would then be introduced if you had an account in some state of inactive/unfinished, which would claim the email but had no person.
<jcsackett> the if block there now fetches the best association the email address has, and then determines what sort of entity it's working with to create as informative a message as possible.
<abentley> jcsackett, I'm not sure that getEmailAssociation is generally-useful.  Have you considered formatting it as "checkAddressAvailable" which would raise an exception with the relevant text?
<jcsackett> abentley: i did; i think i was influenced by the other validators in the file--something about a validator just repackaging an exception bothered me, but not in a way that i can articulate well enough to argue the point. :-P
<jcsackett> i'll make that change.
<abentley> jcsackett, cool.  My main reason is it looks unnecessary to have the same three-way conditional in two spots.
<abentley> jcsackett, it looks like you have unnecessary wrapping on 182 and 282 in the patch.
<abentley> Sorry, 182 and *202*
<abentley> jcsackett, why the blank line at 76?
<jcsackett> on 202 lint complained before; i didn't check if it was right, just wrapped it. could be lint or i got confused. 182 you're right, i had to wrap an earlier version and didn't unwrap it.
<jcsackett> abentley line 76 looks like my editor's import reflow macro errorred. fixing that as well.
<abentley> jcsackett, at 187, can you pass in an email address so you don't need to remove the security proxy later?
<jcsackett> abentley: i can.
<abentley> jcsackett, please do, then.
<jcsackett> abentley: just did. :-P
<abentley> jcsackett, I think that's all the changes I can see.
<jcsackett> abentley: going back to your thoughts on "getEmailAssociation" how do you feel about it returning an error_msg or none, and checking that? the flow of calling a function expecting nothing to happen at all and then reraising an exception feels sort of off.
<abentley> jcsackett, you could just raise LaunchpadValidationError.
<jcsackett> abentley: ah yes. that's obvious. i must need more coffee.
<jcsackett> thanks.
<abentley> jcsackett, np.
<abentley> jcsackett, oh, your spelling of "associaiton" should probably be tweaked if that text makes it into the final version.
<jcsackett> abentley: agreed, but i don't think the comment will survive the refactor. :-)
<deryck> sinzui, hi.  Can you do a CSS/UI review for me?
<sinzui> deryck, I can, but I hope salgado-afk can have a turn first so that he can become a UI reviewer
<deryck> Ah, ok.  I can request him as a reviewer and wait.  I'm not in a hurry.
<abentley> jcsackett, you'll ping me when you've pushed your changes?
<jcsackett> abentley: i was just about to ping you, as a matter of fact. changes pushed and an incremental diff added as a comment.
<abentley> jcsackett, thanks.
<jcsackett> abentley: np.
<abentley> jcsackett, if you attach it, rather than pasting it inline, it'll be rendered as a diff.  But I'm actually working on implementing automatic incremental diffs.
<jcsackett> abentey: that's good to know, thanks.
<jcsackett> (both the attach, and the incrementals being automatic)
<abentley> jcsackett, r=me.
<jcsackett> abentley: thanks. and thanks for helping me rethink that--in retrospect that was some truly ugly code. :-)
<abentley> jcsackett, np.
* sinzui changed the topic of #launchpad-reviews to:  On call: abentley || Reviewing: jcsackett || queue: [sinzui, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> abentley, I have a few branches from the weekend that can be reviewed (they are low priority)
<abentley> sinzui, okay, OTP.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [sinzui, sinzui] || 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: sinzui || queue: [sinzui] || 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: sinzui || 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: abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mwhudson> lifeless: i'm pretty sure i voted on everything, but maybe stuff got eaten by the issues yesterday :(
<lifeless> yes
<lifeless> you can check if you want :P
* 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
<mwhudson> i thought i did check everything
<mwhudson> oh well
<lifeless> mwhudson: you commented 'seems fine', approved the rview, but your comment wasn't marked 'approve'
<mwhudson> ah
#launchpad-reviews 2010-09-14
<lifeless> anyone up for : https://code.launchpad.net/~lifeless/launchpad/registry/+merge/35354
<lifeless> thumper: ^
<wallyworld_> https://code.edge.launchpad.net/~wallyworld/launchpad/tales-linkify-broken-links/+merge/35268
<thumper> lifeless: got a few minutes to chat?
<wallyworld_> thumper: ^^^ as requested, for when you have time later
<thumper> wallyworld_: ack
<lifeless> lib/lp/scripts/utilities/pageperformancereport.py
<lifeless> lib/lp/services/timeline/timedaction.py
<lifeless> see the _td_to_ms function
<lifeless> td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6) / 10**6)
<lifeless> array = numpy.asarray(times, numpy.float32)
<lifeless> blah
<lifeless> thumper: select * from pg_catalog.pg_locks ;
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/35363
<lifeless> this will fix an operational issue
<lifeless> thumper: ^ I want a release-critical from you please
<lifeless> mwhudson: can you perhaps review? its tiny
<mwhudson> lifeless: not easy to test this?
<lifeless> mwhudson: easy enough but its a noddy test: its structurally unsound and I'm going to fix that in a non-getting-cowboyed branch
<mwhudson> ok
<lifeless> mwhudson: the structural fix will have tests that any number of tracers stacked always enter-and-exit
<mwhudson> lifeless: approved, and i voted and everything
<lifeless> thanks!
<lifeless> thumper: ping
<lifeless> mwhudson: https://code.edge.launchpad.net/~lifeless/launchpad/cp/+merge/35364 - same thing, prod version, if you'd be so kind
<lifeless> thumper: ^ thats the one that needs release-critical
<mwhudson> heh, missed my chance
<lifeless> mwhudson: no, its still open ;P
<mwhudson> ok
<mwhudson> done
<lifeless> thanks
* 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
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bdmurray || 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: thumper || 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: deryck || 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
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: I have a big oneâdo you think you can take it?
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/536819-display/+merge/35085
<jtv> I'm so exhausted that I couldn't produce a good cover letter for it; but I can answer questions on IRC.
<jtv> (It's sometimes frustrating work)
<gmb> jtv, Sure. Give me a couple of minutes and I'll take a look.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: great, thanks
* 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
<adeuring> gmb: can I add this branch to your queue: https://code.edge.launchpad.net/~adeuring/launchpad/bug-615763/+merge/35394 ?
<gmb> adeuring, Sure
<adeuring> gmb: thanks!
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jtv, code r=me. Nice work :).
<jtv> gmb: *blush* thanks  :)
<gmb> jtv, This needs a UI review too, doesn't it?
<jtv> gmb: hmm I guessâ¦ there's not much there, but yes
<gmb> Yeah, better safe than sorry.
<gmb> Also, I have the UI reviewing skills of a rubber bulldog.
<jtv> noodles775: is that something you could do?  UI-review the page for a TranslationTemplatesBuild?
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> jtv: I can if henninge doesn't want it?
<jtv> that's a thought
<jtv> not as familiar with build farm, but he might like the practice
<henninge> practice is good
<noodles775> Yeah, I'll wait for him to let...
<noodles775> :)
<jtv> :)
<jtv> henninge: hang on, I'll see how to get you a snapshot
<henninge> jtv: that would be great
<gmb> adeuring, r=me
* 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
<jtv> henninge: still putting together that UI snapshot
<adeuring> gmb: thanks!
<henninge> jtv: I am patient ;)
<sinzui> salgado, do you have time fora UI review of https://code.edge.launchpad.net/~brian-murray/launchpad/limited-subscriptions-page/+merge/35177
<salgado> sinzui, I was on leave for most of the last 3 days, so I'm still catching up with things. if that's the only UI review I have then I certainly have time, but if I already have others in my queue, then I don't think I will.  let me check
<salgado> sinzui, I have another one from deryck, so I'd rather not do this one
<deryck> Hi salgado.  Was just about to ping you about that actually.
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> deryck, I'll get to it soon. :)
<deryck> salgado, no worries. :-)  Not in a hurry, just wanted to make sure you saw I requested from you.
<mars> gmb, ping, would you be willing to review a branch I have for the qa-tagger project?  It closely follows the LP code conventions
<gmb> mars, Sure, in about 20 minutes or so, if that's okay.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [mars] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> salgado, deryck, bdmurray, henninge: I will certainly do UI reviews. We are giving trainees every opportunity to get experience. rockstar and I can do an urgent UI review immediately.
<mars> gmb, that sounds great to me, and thanks for taking it.  Here is the MP: https://code.launchpad.net/~mars/qa-tagger/deployment-summaries/+merge/35093
<deryck> mine isn't urgent, but thanks for the notice sinzui
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: mars || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: here's how to review the TranslationTemplatesBuild page.  Bear in mind that I'm heavily constrained by the architectural possibilities at the moment (not to mention the branch limit).
<jtv> http://paste.ubuntu.com/493645/
<gmb> mars, On line 324 of the diff you need to indent the ]. Other than that, r=me :)
<mars> gmb, cool, thank you very much.
<mars> gmb, you wouldn't happen to have time for the follow-up branch to this one?  Another qa-tagger branch that builds on the DeploymentSummary work.
<mars> bzr pipeline rocks
<mars> split the work into three 500-line branches
<mars> beautiful
<gmb> mars, Sure.
<mars> gmb, thanks!  Here is the branch that builds on it: https://code.launchpad.net/~mars/qa-tagger/report-contents/+merge/35103
 * gmb looks
<gmb> mars, Is there any reason to define a function in the template? I'm not familiar with qa-tagger so I don't know if that's acceptable.
<henninge> jtv: argh!
<mars> gmb, in this case it is acceptable.  I use the template function to produce a list of <a> tags - I don't want to write HTML in the Python file
<jtv> henninge: argh?
<henninge> jtv: I am sorry I cannot run that at the moment ... ;(
<gmb> mars, Fair enough. I'm just not familiar with that pattern :)
<henninge> I have to fix my LP first ...
<jtv> henninge: maverick?
<henninge> yup
<jtv> maverick
<jtv> I knew an Ubuntu release with _two_ animals for a name could be no good
<henninge> jtv: actually, it is psycopg and our code
<gmb> mars, r=me
<jtv> henninge: I'll land what I have then as per procedure, and any UI chances you come up with can go into a separate branch.  These 3 are big enough already, and blocking people.
<mars> gmb, great, thank you for the reviews
<henninge> jtv: fine by me
<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
<henninge> jtv: sorry for the delay
<jtv> hi ho, hi ho, to ec2 I go
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: your branch is running here now, looking
<jtv> cool
* noodles775 changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb,mars: if either of you have time: https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-1/+merge/35408
<mars> looking
<noodles775> Thanks.
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> mars: I just updated the descripton with a link to the LEP and the mockup UI.
<mars> ok
<mars> noodles775, nice idea for the demo harness script - that is a  neat trick
<noodles775> mars: thanks - I usually need to create it for myself anyway, so it's easy to include (when the setup is a bit complicated).
<mars> that is a really useful pattern.  I wonder if anyone else uses this trick too?
<mars> the script is kind of like "turn on feature FOO and add sample data BAR"
<mars> so it is great for acceptance testing
<mars> a demo script
<noodles775> oh right, I'd forgot about the feature flag - yes, that makes a script useful even when the sample data setup is trivial - until we have a UI for feature flags :) ).
<mars> even then, I think it is really useful
<mars> there is a switch to Python, and hopefully iharness, that lets you load scripts on startup
<noodles775> ah, nice.
<mars> so if you have a demo script, you might be able to just do "bin/harness -m demo-feature.py"
<jtv> Can someone remind me what the current process for database reviews and patch number assignment was again?
<mars> gmb or noodles775, you two hack on models - maybe you would know the answer to jtv's question?
<noodles775> jtv: https://dev.launchpad.net/PolicyAndProcess/DatabaseSchemaChangesProcess is pretty up to date.
<jtv> noodles775: ah great, thanks
<noodles775> mars: bin/harness -m `wget http://paste....` or similar would be nice (save the person downloading it etc., given it's not in the branch)
<mars> interesting
<mars> I think you need to use -s or -c for that
<mars> python --help
<mars> noodles775, looks great, r=mars
<noodles775> Thanks mars
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,- || 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: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> salgado, sinzui: i've pushed up everything i think needs doing per my active review, along with incrementals and some comments.
<sinzui> thank. I will get to in in 45 minutes.
<jcsackett> sinzui: sounds good; i need to get lunch anyhow, so there's no huge rush on my part. :-)
<bdmurray> mars: https://code.edge.launchpad.net/~brian-murray/launchpad/subscribe-oops-bug-636412/+merge/35459 could use review and it might actually need a ui review
<mars> bdmurray, certainly, I can review that
<mars> bdmurray, I suggest asking sinzui to suggest an appropriate UI reviewer - a student or himself
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> bdmurray, ping
<bdmurray> mars: heya
<mars> Hi bdmurray, I had a question about your branch
<mars> You added a storyin bugs/stories
<mars> but IIRC the bugs team was working to kill all doctests in their app
<mars> it made me wonder if this branch should be using unit tests instead of doctests?
<bdmurray> mars: this is the test that deryck pointed me at when we were talking about testing this
<bdmurray> 08:39 < deryck> bdmurray, as for tests, see  lp.bugs.stories.structural-subscriptions.xx-bug-subscriptions,txt
<mars> ah, ok, that's fine then.  Just wanted to make sure
<bdmurray> 08:39 < deryck> I think this will fail with the new change, but if not, the change  should be accounted for there.
<mars> bdmurray, thanks
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> sinzui: I've a couple branchs that could use UI review
<sinzui> bdmurray I have been giving salgado and henninge a chance to review first. I can review any that really needs to be done now
<bdmurray> sinzui: ah, I've one that will end up as a cherry-pick so doing it soon would be great - https://code.edge.launchpad.net/~brian-murray/launchpad/subscribe-oops-bug-636412/+merge/35459
<sinzui> I am looking now
<sinzui> bdmurray. I am a bit concerned. I see you changes, but I can subscribe to ubuntu (https://bugs.launchpad.dev/ubuntu/+subscribe) as Sample Person and Foo Bar in dev
<sinzui> bdmurray is this because Ubuntu in dev does not have a supervisor?
<bdmurray> sinzui: yes that is correct
 * sinzui sets supervisor
<sinzui> bdmurray r=me , I will approve the mp
<bdmurray> sinzui: great, thanks
<jcsackett> salgado: ping
<jcsackett> nm. i'll try and get in touch tomorrow.
* mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-09-15
<wallyworld_> https://code.edge.launchpad.net/~wallyworld/launchpad/link-bugs-in-merge-proposal/+merge/34826
<wallyworld_> rockstar: ^^^^^^ you still around? Can you plz give the latest screenshot (#3, in comments) the tick of approval?
<rockstar> wallyworld_, ah, yes.
<rockstar> wallyworld_, looks fine to me.
<wallyworld_> rockstar: awesome, thanks
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/bug-618367/+merge/35489
<lifeless> rockstar: ^ perhaps you could eyeball that, its go a small UI caveat.
<lifeless> rockstar: (A full review would be better, and only a little more effort :P)
 * lifeless looks for a reviewer
<lifeless> hmm
<lifeless> mwhudson: oh hai
<lifeless> thumper: around?
<rockstar> lifeless, hi.  Still need a review?
<lifeless> please
<lifeless> this should squash Distribution:+assignments
<lifeless> which is in https://devpad.canonical.com/~stub/ppr/lpnet/latest-daily-timeout-candidates.html
<lifeless> as number 2 yesterday
<rockstar> lifeless, ugh at blueprints...
<lifeless> rockstar: yeah, But its holding us back.
<lifeless> so, I fixinate.
<rockstar> lifeless, yeah.  I just hate that we have to pay the maintainer's penalty for that code at all.
<lifeless> me too
<lifeless> still, it could be worse.
<rockstar> lifeless, one day, a happy community member may take it over.
<lifeless> mtaylor will if we can just get him over the activation energy needed to get starting
<lifeless> rockstar: so what do you think ?
<rockstar> lifeless, can you give me a brief explanation of BatchNavigator?
<lifeless> did you see my peformance tuesday mail ?
<lifeless> I may have been too brief in it. So yes, I can and will explain.
<lifeless> BN adapts iterables in general, ResultSets in specific for use in batches
<lifeless> it is used by the API and by various web pages (e.g. branch listing, bug listings, email moderation as of yesterday, ...)
<lifeless> we should probably combine it with HugeVocab's guts
<lifeless> its currentBatch() function returns a slice of the iterable
<lifeless> (perhaps it adapts slicable, more specificlly?..)
<lifeless> the batch navigator object itself is used for generation of navigation links, popups and so on in the navigation view/templates
<rockstar> lifeless, and how does this affect performance?
<rockstar> If you already have the ResultSet, hasn't the query already been run?
<lifeless> as long as you give it a resultset, not a listfied-resultset, its better
<lifeless> rockstar: no, ResultSets are lazy
<lifeless> len(rs) -> COUNT(*)
<lifeless> list(rs) -> SELECT ...
<lifeless> list(rs) again -> SELECT. ... (again(
<rockstar> lifeless, so it somehow combines all the queries that need to run to get their resultsets?
<lifeless> slice = rs[1:3]
<lifeless> list(slice) -> SELECT ... OFFSET 1 LIMIT 3
<rockstar> lifeless, yeah, so how does the BatchNavigator make it faster?
<lifeless> rockstar: no, the thing you adapt (in this case, self.specs) needs to be a single resultset that will Do The Right Thing
<lifeless> rockstar: two ways
<lifeless> firstly, by not bring back everything.
<lifeless> 'specs' in the Ubuntu context has 3000 rows
<rockstar> Ah, so it gives you a 500 slice.
<lifeless> this patch means we'll only bring back 500 of them.
<lifeless> secondly, the database can (in some circumstances) do less work when returning an LIMIT result (if the indices support that)
<rockstar> lifeless, ah, okay.
<rockstar> So if BatchNavigator makes a single resultset that will Do The Right Thing, what was the wrong thing?
<lifeless> and thats very important in gigantic collections (like ubuntu bugs)
<rockstar> Just returning 3000 results?
<lifeless> but in our case its only the first thing we care about
<lifeless> rockstar: BN takes a single result set and slices it.
<lifeless> rockstar: the wrong thing is returning a very large slice to template code to iterate over.
<rockstar> lifeless, ah!  I see.
<lifeless> rockstar: particularly when that may grow without bound.
<lifeless> every UDS we add 500 or so specs
<lifeless> maybe its only 250. Anyhow, lots.
<rockstar> lifeless, we have this pattern in a few places.  I wonder if it might be of interest in bring up on the the list a way to create a GenericListing page that already has some of the performance stuff built in.
<rockstar> Er, we have this pattern of doing listings of many objects (sometimes without bound).
<lifeless> well, BN is the generic solution
<rockstar> lifeless, so would you suggest that BN be used in all listing pages?
<lifeless> note that the template code is pretty close to being entirely reusable
<lifeless> rockstar: we should use it whereever the folllowing is true:
<rockstar> lifeless, are there any cases where BN might not be the best solution?
<lifeless>  - the listing can grow and grow to more than is useful for a human on the page
<lifeless>  - or the rows are very expensive to generate (and we've checked the sql and *everything*)
<lifeless> rockstar: you'd be best to ask curtis that last one.
* 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
<lifeless> I'm not sure if its ready for use on a page that shows several distinct collections
<lifeless> like +activereviews
<lifeless> but OTOH +activereviews is meant to be self limited
<lifeless> by only showing a work queue
<lifeless> rockstar: I'd certainly consider using BN if I had a listing of hundreds or thousands of things.
<lifeless> rockstar: AFAIK its the only pagination solution we have
<rockstar> lifeless, okay.  Thanks for the run-down.
<lifeless> anytime
<rockstar> lifeless, why 123	+            person_ids.discard(None)
<lifeless> because assignee etc can be NULL
<rockstar> Ah, okay.
<lifeless> and we don't want to query for Person where Person.id==NULL
<lifeless> phrased as Person.id in (NULL, 1, 4, 5) I suspect bad things happen :)
<rockstar> lifeless, r=me
<lifeless> thanks!
* jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> lifeless: I believe the current procedure for db patches is to get reviews from stub and you
<lifeless> jtv: https://dev.launchpad.net/PolicyAndProcess/DatabaseSchemaChangesProcess
<jtv> lifeless: https://code.edge.launchpad.net/~jtv/launchpad/translationtemplatesbuild/+merge/34952
<jtv> :-)
<jtv> "Say it with URLs"
<lifeless> jtv: well the answer is 'no'
<lifeless> but I thought more info than that would help you.
<jtv> "Submit a merge proposal for your branch into lp:launchpad (lp:~launchpad-pqm/launchpad/db-devel), requesting twoÂ dbÂ reviews from both the technical architect (lifeless) and the DBA (stub)"
<lifeless> yes
<lifeless> keep reading
<jtv> You mean the "if either is away" part?
<lifeless> yes
<lifeless> stub is not away
<lifeless> so you need his review and the patch # will come from him
<lifeless> I'm interested in the change and will look at it
<lifeless> but I'm not part of the landing path
<jtv> OK, then I wait for stub
<jtv> lifeless: I think the wiki page implies the opposite of what you just said though.
<lifeless> how so ?
<jtv> The wiki page says "request reviews from both the dba and the architect; _if one of them is away_ then just the other is enough."  Just now you said "stub is not away, therefore I am not necessary to the process."
<lifeless> thats not quite what it says
<lifeless> you always request from both
<lifeless> thats what it says
<lifeless> and separately
<lifeless> If one of the DBA or Technical Architect is away, the other will allocate database patch numbers and provide reviews
<jtv> So how is that not what I said?
<jtv> It covers those two points, giving an unintentional suggestion that approval by just you _or just stub_ is a backup option for the case where the other is not available.
<lifeless> thats the intent
<lifeless> only one of us has to approve for it to land
<lifeless> And if stub is not on leave, stub does it all.
<lifeless> I'm confused
<jtv> Then the page should say that.  Right now it implies through omission that stub shouldn't do the review alone unless necessary.
<jtv> Wait, wait, I missed a bit.
<jtv> It's hidden at the end of point 4.
<jtv> (I say "hidden" because when I see this much text, I instantly filter out a lot of it)
<jtv> (Particularly so when the text is structured along steps-to-take lines when I want statement-of-essentials or vice versa)
<lifeless> If you can improve it, do so :)
<jtv> I can try
<jtv> lifeless: could you have a look at the top two sections?  I added the first, revised the second, deleted the third.  https://dev.launchpad.net/PolicyAndProcess/DatabaseSchemaChangesProcess
<jtv> I deleted the third section because it played into the confusion: when I was scanning for answers, the "help help what to do if either the dba or architect is away" header grabbed priority over the step-by-step recipe text.
<jtv> Would it help if we had a launchpad-db review team and had engineers request a single db review from there?
<lifeless> the diffs looked fine to me
<jtv> Thanks.
 * jtv dreams of a "check all review types that apply" in the MP formâ¦
<StevenK> jtv: That's a brilliant idea
<StevenK> jtv: File a bug?
<jtv> StevenK: thanksâbut maybe discuss it a bit further to avoid falling into the "I dreamed up this great idea that solves the problem as _I_ encounter it, I'll spec it out in great detail, and then all the engineers have to do is code it up" pattern?
<jtv> _Our_ usage pattern is: we have a set of review types, each with one default reviewer, and we may need a combination of those.
<jtv> Would anyone else's use differ?
<jtv> I don't think so, but I'm not anyone else.  :)
<StevenK> jtv: No, that's not the plan. "I had this brilliant idea, let me share it with you, and we can use this usual bug report page to track it."
<StevenK> s/usual/useful/
<jtv> StevenK: I'm glad that's not the plan, but I for one am human and fallible and often the victim of the very same thing.  :)
<jtv> Ah what the hell, you're right.
<jtv> Looking through existing bugs list for a matchâ¦
<jtv> StevenK, lifeless: just filed bug 638631
<StevenK> We don't have a bug bot in here? That's rather mean.
<jtv> So it would seem.
<jtv> stub: want db review!  https://code.launchpad.net/~jtv/launchpad/translationtemplatesbuild/+merge/34952
<jtv> henninge: good morningâyou're not on the other irc
<henninge> jtv: Hi! ;)
 * henninge reconnects the other irc
 * stub looks
<stub> TranslationTemplatesBuild instead of TanslationTemplateBuild? If we had a TranslationTemplate table it would be called TranslationTemplate
<jtv> stub: well it's a build of all translation templates in a branch, and at that stage we don't even know which those will be.  So there's no a-priory tie to POTemplate.
<jtv> It's also consistent with the other class names; I wanted to avoid any impression that the build might belong to a single template, or to any specific template.
<jtv> If, in a completely imaginary thought exercise, we renamed POTemplate to TranslationTemplate and wanted to create a reference from this new build class to TranslationTemplate, we'd have m:n and a linking table.
<jtv> In that thought exercise, the linking table might be called TranslationTemplateBuild (if it weren't for the confusing similarity in names, of course)
<stub> hmm...
<jtv> Did I really write a-priory?  I meant a-priori, of course.
<stub> SPEAK ENGLISH!
<jtv> oi sorry guv
<jtv> A Branch goes in, a set of TranslationImportQueueEntrys comes out.  Those later get assigned to respective POTemplates by the queue gardener.
<stub> approved anyway
<jtv> Thanks.
* StevenK changed the topic of #launchpad-reviews to: On call: jtv || 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/decoratedresultset/+merge/35507
<lifeless> jml: ^ care to do a micro review to start your day up ?
<jml> lifeless, https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1719ED569
<lifeless> 'did not match any oops'
<jml> lifeless, they take a while to be synced.
<lifeless> I know
<lifeless> thats on my hit list too
<lifeless> but its not there yet.
<lifeless> which is the worry
<jml> 10 minutes, I thought.
<lifeless> jml: this is the more interesting MP - https://code.edge.launchpad.net/~lifeless/launchpad/malone/+merge/35511
<lifeless> jml: and the oops still isn't visible
<lifeless> jml: can you change your vote to approve, because of ec2land ?
<jml> lifeless, sure.
<lifeless> I really hate that style of function formatting btw
<jml> lifeless, I hate the other style :)
<lifeless> same as I hate it for function wrapping.
<lifeless> I'm doing it, just whinging.
<jml> we don't do it for calls :)
<lifeless> jml: actually, its one of the 'approved styles'
 * jml eyebrows
<lifeless> anyhow
<lifeless> I shall blithley go on doing everything to the whims of reviewers :>
<lifeless> jml: thanks for reviewing the prerequisite
<lifeless> still no oops
<lifeless> well, I'm off to sleep
<jml> lifeless, g'night
<bac> hi jtv, you still reviewing?
* bac changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [StevenK, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-638420/+merge/35477
<jcsackett> salgado, ping. :-)
<jtv> bac: sorry, bit late hereâ¦ I'll update the topic.
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> hi jcsackett, I guess your branch is ready for another look?
<jcsackett> salgado: yup. sinzui had one comment on it after i pushed up changes that i've dealt with.
<jcsackett> so it is ready for you to take another look at.
<salgado> cool, I'll do it now
<deryck> salgado, hi.  Who is mentoring you for UI reviews?
<salgado> deryck, sinzui
<deryck> ok, thanks.
<deryck> sinzui, could I get your final review of https://code.edge.launchpad.net/~deryck/launchpad/description-editing-ubuntu-font/+merge/35321 which salgado reviewed?
<sinzui> I will
<sinzui> deryck, salgado your have my r=me. I will update the MP to explain why will *not* be removing UbuntuBeta from any Canonical website
<deryck> excellent, thanks!
<bac> EdwinGrubbs: i've got a tiny branch when you start reviewing
<StevenK> I suspect my branch in the queue is even tinier
<salgado> jcsackett, can you give me some URLs for pages that had UI changes?
<jcsackett> salgado: the one i've been using is http://launchpad.dev/thunderbird in devel vs in my branch. i can send you a screenshot of the devel version so you don't have to jump between two branches, if you like.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> jcsackett, yeah, that'd be nice, thanks
<jcsackett> salgado: the changes are only there if you configure blueprints to the various not on launchpad settings (EXTERNAL, UNKNOWN or NOT_APPLICABLE)
<jcsackett> ok, i'll send a screenshot in a moment.
<salgado> jcsackett, for the future, in these cases it's nice to add new *dev* sampledata (e.g. projects with blueprints configured to the various possible states) so that one can easily see the changes without having to change the settings on the web UI
<salgado> that helps yourself when developing and your UI reviewer. :)
<jcsackett> salgado: i hadn't thought of that. i'll keep that in mind, thanks. :-)
<EdwinGrubbs> StevenK: I think you meant your if-statement to be    if len(sys.argv) > 1:
<salgado> jcsackett, what's the difference between unknown and not_applicable?
<EdwinGrubbs> StevenK: actually, you might want to allow the user to supply multiple branches on the command line, so you could set the variable as:     branches = sys.argv[1:]
<jcsackett> salgado: unknown is when nothing has been set: so a new project has it as the default. not_applicable represents when a project states they don't use it at all.
<StevenK> EdwinGrubbs: Yes, I just came to that conclusion myself :-)
<salgado> jcsackett, oh, right, I was confused because the UI seemed to be the same in both cases, but now I see that's not true
<jcsackett> it is *very* similar. :-P
<StevenK> EdwinGrubbs: Changes pushed
<jcsackett> salgado: tar of screenshots here: http://dl.dropbox.com/u/375578/blueprints-screenshots.tar.gz
<salgado> jcsackett, thanks!
<jcsackett> salgado: i found one typo while making those screenshots--i'm pushing up a change that addresses it (not_applicable had the copy "Blah does not use track ..." instead of "Blah does not track ...")
<salgado> jcsackett, yeah, I found that too
<salgado> :)
<salgado> but already sent the review, so just ignore it
<jcsackett> will do. :-)
<EdwinGrubbs> StevenK: since that script has a chdir() in it, relative path names don't work unless you are already in the directory it's switching to. You can probably fix that by putting the chdir() in the else-block. As long as you have an else-block, you can set branches with listdir() in there instead of before the if-statement.
* jelmer changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK, bac, jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> EdwinGrubbs: Can I add myself to the queue?
<EdwinGrubbs> allenap: sure
* allenap changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK, bac, jelmer, allenap]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> EdwinGrubbs: Thanks :)
<EdwinGrubbs>  bac: r=me
<bac> thanks edwin
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: jelmer || queue: [StevenK, allenap]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Do I need to get a database review for security.cfg changes?
<bigjools> allenap: I don't think so
<allenap> bigjools: Cool, thanks.
<bigjools> at least I've not bothered in the past, it affects the code more than the db
<EdwinGrubbs> jelmer: the old findBuild() compared the pocket, distroseries, and archive to determine whether to raise an exception, but storeObjectsInDatabase() only compares the source_package_release. Is this right?
<jelmer> EdwinGrubbs: Yes, the consistency checks from findBuild() have now moved to checkBuild()
<jelmer> edwingrubbs: The idea being that checkBuild() always gets called and findBuild() will only get called in some situations (as we may pass the build object in rather than having to look for it).
<EdwinGrubbs> jelmer: if the build argument is passed into storeObjectsInDatabase(), it will effectively skip the build from the first binary_package_file in bpfs_to_create. Is this intended?
<jelmer> EdwinGrubbs: that will only happen if there is a single entry in bpfs_to_create (there's an assert to make sure this is the case)
<EdwinGrubbs> jelmer: would it make it clearer to make the block in the for-loop its own function. Right now, you are effectively passing in build as a parameter to the for-loop, which is why the logic is hard to follow.
<jelmer> EdwinGrubbs: This code is about to change again for the follow-up branch, would it be ok if I improved the logic in that branch instead?
<jelmer> EdwinGrubbs: I see your point though.
<EdwinGrubbs> jelmer: sure
<EdwinGrubbs> jelmer: r=me
<jelmer> EdwinGrubbs, thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: allenap || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> EdwinGrubbs: Are you reviewing bugsubscription-to-storm or my other branch? I've renamed the other one so any browser windows you have open to the merge proposal will fail form submission. It's now https://code.edge.launchpad.net/~allenap/launchpad/bug-subscription-filter-models-bug-639749/+merge/35546
<allenap> EdwinGrubbs: Also, I have to go now; dinner time with the kids. Is that okay?
<EdwinGrubbs> allenap: which one do you want me to review? It's fine if you need to leave.
<jelmer> EdwinGrubbs: Would you have time for another review ? It's the follow-up to the other branch that you reviewed.
<EdwinGrubbs> jelmer: sure
<jelmer> EdwinGrubbs, The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/506256-remove-popen-2/+merge/35412
<jelmer> EdwinGrubbs: Unfortunately it contains the diff for the other branch as well, since it hasn't landed yet and I already have a different prerequisite branch
<EdwinGrubbs> jelmer: wow, I merged in both archiveuploader-build-handling nad 506256-remove-popen, and 506256-remove-popen-2 is still 1300 lines.
<EdwinGrubbs> jelmer: are there any other prereq branches besides those two?
<lifeless> EdwinGrubbs: https://code.edge.launchpad.net/~lifeless/launchpad/malone/+merge/35511
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: jelmer || queue: [allenap, lifeless]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> EdwinGrubbs: No, there aren't any others. If it's 1300 lines even with those branches merged in, perhaps I should have a look at splititng it up further.
<jelmer> EdwinGrubbs: I'll mark it as on hold for the time being - sorry for the trouble.
<EdwinGrubbs> no problem
<lifeless> EdwinGrubbs: hi?
<EdwinGrubbs> lifeless: hi
<lifeless> are you reviewing allenaps still ? or I can has review?
<EdwinGrubbs> lifeless: I'll be done with his shortly
<lifeless> cool
<lifeless> I'm just excited :)
<EdwinGrubbs> it's my job as a reviewer to crush excitment
<lifeless> nae knave, tis not
<lifeless> EdwinGrubbs: btw - bugsubscription-to-storm - we need a storm base class with cache integration before moving things with cachedproperty around (maybe I'm wrong and it doesn't have any)
<EdwinGrubbs> lifeless: ok, I'm probably only going to review allenap's other branch today.
<lifeless> EdwinGrubbs: do you mean I should another reviewer? (Just occured to me there are multipe parse trees for your statement)
<EdwinGrubbs> lifeless: oh, no. I'll review your branch today, also.
<lifeless> cool
<jcsackett> EdwinGrubbs: i've got an MP to throw in the queue, are you likely to have time for it?
<lifeless> jcsackett: I'm on call now too
<lifeless> submarine style
<lifeless> jcsackett: whats the MP
<jcsackett> lifeless: https://code.edge.launchpad.net/~jcsackett/launchpad/user-email-existing-account-576757/+merge/35575
<jcsackett> it's fairly simple.
<EdwinGrubbs> jcsackett: in about 1.5 hours
<jcsackett> EdwinGrubbs: unless i'm mistaken, lifeless is offering to take mine.
<EdwinGrubbs> oh, nevermind
<jelmer> EdwinGrubbs: I've got an updated (smaller) branch when you're back.
<lifeless> jcsackett: what is it
<lifeless> sorry
<lifeless> jelmer: what is it
<lifeless> jcsackett: done
<lifeless> jelmer: whats the MP url
<jelmer> lifeless: https://code.edge.launchpad.net/~jelmer/launchpad/no-more-buildid/+merge/35572
<jelmer> lifeless: removing the --buildid argument from archiveuploader and access to the command line options object from deep inside archiveuploader
<jcsackett> lifeless, thanks.
<lifeless> jelmer: done
<jelmer> lifeless: Thanks!
<lifeless> EdwinGrubbs: I'm pushing up rev 11549 to my branch, just some fine tuning
<EdwinGrubbs> lifeless: review sent. The only comment I have about rev 11549 is the same thing I noted already about store.using(tables) with storm objects.
<lifeless> thanks
<lifeless> EdwinGrubbs: I might stay with the literal string here
<EdwinGrubbs> lifeless: If you stick with string literals, could you format them so they are easier to read? For example, one table per line, and if the conditional is really long, indent it over multiple lines.
<lifeless> I'll fiddle with it a little yes
<lifeless> I need a review https://code.edge.launchpad.net/~lifeless/launchpad/lessGetLastOops/+merge/35605
<lifeless> this may help with the ec2 crashes
 * jelmer looks
<jelmer> lifeless: r=me
#launchpad-reviews 2010-09-16
<lifeless> jml: your oops has come up : https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1719ED569
<lifeless> jml: that doesn't look like you to me.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: jelmer || queue: [allenap, lifeless]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> lifeless: If you have time, I have a branch that needs a db review. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-registry-jobqueue-schema/+merge/35619
<StevenK> EdwinGrubbs: Hi! Can you please look at the MP you reviewed for me last night?
<StevenK> EdwinGrubbs: I agree with your changes, I've applied and tested them.
<lifeless> EdwinGrubbs: hi, stub needs to review it
<lifeless> EdwinGrubbs: the policy is 'request from both of us so we both know' and 'stub does it unless hes on leave'
<EdwinGrubbs> StevenK: I'll look at it now.
<EdwinGrubbs> StevenK: that looks good. r=me
<StevenK> EdwinGrubbs: Thanks!
<EdwinGrubbs> lifeless: ok, thanks
<lifeless> EdwinGrubbs: you did raise an interesting question though and I've commented there
<lifeless> EdwinGrubbs: I was talking with deryck this morning about having a single deferred email dispatcher
<lifeless> that would jsonify-or-so *all* the emails an appserver wants to send, shove that into one job, and then it gets committed as part of the transaction
<lifeless> and a Job would process those for the whole system after
<StevenK> Someone around who can review a small branch?
<StevenK> 1 small code change, mostly just lint
<lifeless> yes
<StevenK> lifeless: https://code.edge.launchpad.net/~stevenk/launchpad/db-clean-up-lint-idsjob/+merge/35621
<StevenK> lifeless: Sorry to do this, but I'm about to head out, and I'd like that branch on ec2 while I'm out, can you push it up your stack, please?
<lifeless> StevenK: I did it already
<StevenK> lifeless: Sorry, I'm a muppet.
<lifeless> yes
<lifeless> :P
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: jelmer || queue: [allenap, lifeless, StevenK]  || 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 || Reviewing: jelmer || queue: [allenap, lifeless, StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> resolved 7 conflicted files.. posting the diff now
<noodles775> Thanks jelmer
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jelmer || queue: [allenap, lifeless, StevenK, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> henninge: I've got another MP needing UI thought if you've time (but no rush): https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
<henninge> noodles775: wow, with screencast?
<henninge> noodles775: you rock!
<noodles775> henninge: well, I try to make it easy to understand (hard to describe in text) :)
<henninge> noodles775: really great! Didn't you describe somewhere how you do these? Or did someone else?
<noodles775> henninge: I did put something on the wiki at some point... it's just gtk-recordmydesktop, thank them :)
<henninge> noodles775: I would not expect to have a standard for comments with logos because we have never had any of those.
<henninge> noodles775: on first view they struck me as taking up a lot of space.
<henninge> noodles775: my take: we should not have two types of comments "with" and "without" logos.
<noodles775> henninge: yeah, I noted that in the MP, saying I should probably just do the same as mp/bug comments. The space is because the person logo (ie. it would be there photo), only comes in the 64x64 size.
<noodles775> s/there/their
 * henninge did not read ... just watched in awe .. ;)
 * henninge reads now
<henninge> noodles775: yes, I think the comments should look like other comments we have elsewhere.
<henninge> or we should commit to changing all comments to include headshots ... ;)
<noodles775> yep, agreed.
<henninge> noodles775: that multiline comment - does it have double new lines or why is the line spacing so big?
<noodles775> henninge: the formatter being applied treats double new-lines in text as paragraph boundaries... I think I need to ensure paragraphs have less margin in those comments.
<henninge> noodles775: It should be treated by the css for comments, like on any other comment.
<henninge> noodles775: Meaning: I don't think you need to worry about that much.
<noodles775> henninge: yes, that's what I meant (ie. I need to check...)
<noodles775> henninge: are you running maverick?
<henninge> yes
<noodles775> Can you please just run one test for db-devel and see if you get the error I mention on the MP?
<noodles775> I've just tried the following with r9787 of db-devel (which went through buildbot recently), but always get the error linked on the MP.
<noodles775> bin/test -vv -m test_archive -t test_getPublications_returns_all_published_publications
<noodles775> (just a random test).
* allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, - || queue: [allenap, lifeless, StevenK, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> (well, one that uses the DB).
<allenap> Good morning noodles775 :)
<noodles775> Hi allenap! Welcome back!
<henninge> noodles775: you need to downgrade psycopg
<allenap> noodles775: Cheers :) Right, on to lifeless's branch.
* allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, lifeless || queue: [allenap, StevenK, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> henninge: ah, I do remember glancing over something with that info... thanks.
<henninge> noodles775: download from here https://edge.launchpad.net/ubuntu/lucid/i386/python-psycopg2/2.0.13-2ubuntu2
<henninge> (edge css is broken again)
<henninge> noodles775: then do "dpkg -i --force-downgrade" with it.
<noodles775> Thanks.
* allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, StevenK || queue: [allenap, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Gah, not doing very well this morning.
<noodles775> Oh? I just thought you were super fast :)
<allenap> noodles775: Which of jelmer's branches are you reviewing? I'll do the other.
<noodles775> allenap: sorry, this one: https://code.edge.launchpad.net/~jelmer/launchpad/506256-remove-popen-2/+merge/35412
<noodles775> I've just claimed to avoid more confusion :)
* allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, jelmer || queue: [allenap, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: I don't understand what the "Pacakge differences" section is telling me.
<noodles775> henninge: yes - it may need more explanation. So when there are different versions of a package in the two distro series...
<noodles775> there will be a base version (ie. the last common version before they diverged)
<noodles775> Those differences will be available on request (or generated automatically - still need to do that), to show the difference between the base version and each distro series version.
<henninge> noodles775: Ah, I see. Those will be clickable and expand, too?
<noodles775> henninge: no, that wasn't planned, but could be nice.
<henninge> noodles775: isn't the "last common version" information missing?
<henninge> noodles775: but they will be linkified to an extra page?
<noodles775> henninge: yes, it says "unimplemented" in the demo (as I need to add a db column for it).
 * henninge is blind
<noodles775> And yes, the diffs will link to librarian files.
<henninge> noodles775: and it will always be those two link: "base to parent" and "base to derived" ?
<noodles775> henninge: sometimes just one (ie. when a package hasn't been updated in the parent series, but only the derived, it's no point generating the base-parent diff)
<henninge> noodles775: so, because they are downloads AFAIUI, they should be preceeded by the download logo.
<noodles775> They don't work as downloads on other pages... let me check.
<noodles775> (not that that means they shouldn't)
<henninge> noodles775: links to librarian?
<noodles775> links to text files on the librarian.
<henninge> that depends on your browser settings, I think
<noodles775> henninge: right, and it's a .gz anyway...
<noodles775> So currently PackageDiffs aren't marked up as download links, but that would be great to change. +1
<henninge> noodles775: maybe call the section "Package differences from base version:"
<henninge> noodles775: oh, where do I normally see them?
 * henninge has not worked with those before
<noodles785> *sigh*
<noodles785> Last thing I said was:Expand 'hello' and you'll see a link for an available diff.
<noodles785> (for the link https://edge.launchpad.net/~michael.nelson/+archive/pocketsphinx/+packages?field.name_filter=&field.status_filter=&field.series_filter= )
<henninge> noodles775: yes, those should have the download icon, too.
<henninge> noodles775: I also wonder if a little indention sound be fitting because it is a list (mostly two entries).
<noodles775> jelmer: You've added some delete perms to the uploader (now that you're removing the buildqueue entries earlier), but does that mean we can remove DELETE perms elsewhere?
<noodles775> henninge: yes on both counts.
<jelmer> noodles775: That's a good point, let me check
<henninge> I don't know how "sound" got in there ... ;)
<henninge> I probably meant "would" ...
<noodles775> Yeah, I assumed you meant 'could' :)
<noodles775> jelmer: also, I guess you tried store.flush() before you ended up with the txn.commit()?
<noodles775> (or you know why that wouldn't work :) ).
* allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, lunch || queue: [allenap, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> noodles775: Yeah, I did - although not for all instances individually.
<henninge> noodles775: reviewed, needs-fixing as discussed.
<noodles775> Great, thanks henninge, I'll finish the windmill test too before requesting a re-review.
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: lunch, lunch || queue: [allenap, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> allenap: did you review StevenK's branch? I can't see one on the active reviews page either, but I did think he mentioned he had one. StevenK ?
<noodles775> jelmer: review sent... great work, just a few questions.
<jelmer> noodles775: Thanks
<StevenK> noodles775: Sorry, had the silly thing as WIP
<StevenK> noodles775: https://code.edge.launchpad.net/~stevenk/launchpad/db-add-derivedistroseries-api/+merge/35500
<jelmer> allenap: Thanks for the review
<allenap> jelmer: You're welcome :)
* allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: lunch, noodles775 || queue: [allenap]  || 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: lunch, StevenK || queue: [allenap]  || 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: lunch, wallyworld || queue: [allenap]  || 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: allenap, wallyworld || 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: allenap, StevenK || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> allenap: what order of subscriptions are returned by getSubscriptionsFromDuplicates()?
 * allenap looks
<noodles775> sorry, I meant how many subscriptions - and whether it's currently a method with performance issues or not.
<allenap> noodles775: I don't know if it has performance problems, but the code certainly appears sub-optimal. It should return one subscription per subscriber. Each subscriber could have more than one subscription so I've ensured that the earliest is always returned. I /think/ it was undefined before.
<noodles775> allenap: why is it valid for a person to have multiple (current/valid) subscriptions to the same bug? Or is it just because it can also be via teamparticipation?
<noodles775> Actually, looking at that query, it's only subscriptions for the person directly, so why can they have more than one?
<allenap> noodles775: It's because the subscriptions are to duplicates. There could be several duplicates for a bug and one person could be subscriber to more than one.
<noodles775> Right - just spotted that... sorry for the noise.
<allenap> noodles775: It's not noise! :) It's quicker to ask than to pore over code. Although I regularly pore over code when I could ask, ahem.
<allenap> noodles775: http://paste.ubuntu.com/494739/ <-- a small optimization to that branch. Do you mind if I smuggle that in?
<noodles775> 'course not :)
<allenap> noodles775: Thanks :)
<jelmer> noodles775: I've done the checks you mentioned your the review, and managed to avoid the remoteSecurityProxy call. However, I'd like to make an additional change - flushing the store of build in packagebuild to avoid potential race conditions between buildmaster and the upload processor.
<jelmer> Does that seem reasonable?
<noodles775> jelmer: sure.
<noodles775> allenap: OK, done. I added a thought for possibly doing that query all in the db... but haven't checked to see if it would work.
<allenap> noodles775: I'm interested.
<noodles775> jelmer: have you/can you push an incremental?
<jelmer> noodles775: ok
<jelmer> noodles775: updated diff posted
<noodles775> Thanks jelmer
<jelmer> noodles775: I haven't looked at splitting up those two tests yet.
<noodles775> jelmer: that's fine - it was just a thought, I didn't expect you to necessarily do it unless you were really keen :)
<allenap> noodles775: Thanks for the query suggestion. Does this look sane to you? http://paste.ubuntu.com/494766/
<noodles775> allenap: yeah, great stuff! (assuming it works?)
<allenap> noodles775: The tests pass ;)
<noodles775> Wonderful :)
<allenap> noodles775: To be honest, I'm not sure that there's sufficient coverage on this method, so I'm going to add another test.
<noodles775> allenap: ensuring that the bug subscription is indeed the first one, or something else?
<allenap> noodles775: Yes. I didn't look closely before, but now I find that the only direct test I can find for getSubscriptionsFromDuplicates is right at the end of a doctest, and it doesn't make me feel awfully comfortable.
<noodles775> Sounds excellent (and you could even remove the doctest example if it's not relevant documentation-wise?)
<noodles775> jelmer: did you mean to paste the whole diff in that comment, or just the incremental? (the MP will update the diff when you push your branch right?). I'm trying to get an incremental but it looks like you haven't pushed your branch? (last rev. is 11106 20hrs ago).
<jelmer> noodles775: http://pastebin.ubuntu.com/494772/
<noodles775> jelmer: looks fine. Were those tests failing previously? (the one you've updated there that assumed the upload log would be present, but now asserts that it is not)?
<jelmer> noodles775: I indented build.storeUploadLog(), which changed the behaviour slightly.
<jelmer> noodles775: So we're not storing the upload log when the upload succeeded, consistent with previous behaviour (see the discussin in #launchpad-dev earlier for background)
<noodles775> Yeah, I saw parts of it flash by :)
* noodles775 changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: StevenK || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> allenap, i have a slightly oversized branch coming towards you
<allenap> leonardr: I might not have time to do it today :-/
<leonardr> allenap: ok, np
<leonardr> take a look once i get the mp up
<allenap> Cool.
<bdmurray> Is there somebody I should ping about my UI review for https://code.edge.launchpad.net/~brian-murray/launchpad/limited-subscriptions-page/+merge/35177 ?
<bdmurray> salgado: is there a queue for ui reviews?
<salgado> bdmurray, not that I know of
<bdmurray> salgado: Then I just ping a UI reviewer?
<salgado> bdmurray, yep, that's how people have been telling me about ui reviews
<bdmurray> salgado: ping ;-) https://code.edge.launchpad.net/~brian-murray/launchpad/limited-subscriptions-page/+merge/35177
<salgado> bdmurray, heh, ok, I should be able to review it later today, but can't promise because I need to wrap up other stuff and I'm kinda slow because I've got one arm in a sling
<leonardr> well, allenap can't take it, but maybe someone else can look at https://code.edge.launchpad.net/~leonardr/launchpad/accept-oauth-signatures/+merge/35697 ?
<leonardr> i'd really like salgado to look at it but i don't want to put more work on his plate
<leonardr> bdmurray, is your branch something i can look at?
<bdmurray> leonardr: If you can do UI reviews sure.
<leonardr> bdmurray: sorry, not rated for ui reviews
<bdmurray> salgado: sorry about your arm - its no hurry I just wanted to get in some queue
<jelmer> Hi StevenK
<jelmer> StevenK: Can I add a simple soyuz branch to your queue?
<lifeless> jelmer: I think you mean allenap
<jelmer> lifeless: You're right, thanks
<jelmer> I think allenap has probably signed off by now though :-(
<jelmer> allenap: Still there?
<lifeless> jelmer: link the branch earlier ;)
<jelmer> lifeless: :-) It's at https://code.edge.launchpad.net/~jelmer/launchpad/635591-sync-source-unicode/+merge/35715
<lifeless> jelmer: your description references the same bug, not an older one?
<jelmer> lifeless, it's a fix for an issue introduced by the fix for bug 635591, and found while I was QA'ing that.
<lifeless> but it appears to be the same branch
<lifeless> with everything the same
<lifeless> it says its the fix for itself
<jelmer> The last revision is new
<lifeless> there's only one revision there
<lifeless> unmerged that is
<jelmer> Yes, the original fix has already landed on devel.
<lifeless> so where is the original MP?
<lifeless> look at this with fresh eyes
<lifeless> where is the qa-bad tagged bug
<lifeless> so that we know not to rollout the thing that failed qa
<jelmer> https://code.edge.launchpad.net/~jelmer/launchpad/635591-sync-source-unicode/+merge/35637
<lifeless> and why isn't that MP linked to the branch anymore ?
<lifeless> ah someone rebased.
<lifeless> ?
<jelmer> I'm not sure why it's no longer linked.
<leonardr> sinzui or mars, do you have any interest in doing an interactive review?
<jelmer> lifeless: I've se the qa-bad tag.
<sinzui> leonardr, I will have time in an hour
<lifeless> approved
<leonardr> sinzui: ok, you're on
<jelmer> lifeless: Thanks
<sinzui> leonardr, sorry about my delays. I am ready to review
<leonardr> sinzui, great
<leonardr> the branch is https://code.edge.launchpad.net/~leonardr/launchpad/accept-oauth-signatures/+merge/35697
<leonardr> if there's anything you don't understand about the system i'm changing or what i'm aiming for, tell me andi 'll go through it
<sinzui> leonardr, I am ready, the backstory is interesting
<leonardr> sinzui: ok, go through the helpers i added to _webservice.py, and then see what i did with them in authorize-token.txt
<sinzui> leonardr, do we care if I hack my user-agent string?
<leonardr> sinzui: the user-agent thing is mostly to avoid any plausible deniability that an unauthorized client might have "accidentally" acquired someone's GRANT_PERMISSIONS token. it can't provide any real security
<sinzui> from canonical.launchpad.interfaces import IPersonSet becomes
<sinzui> from lp.registry.interfaces.person import IPersonSet
<sinzui> leonardr, ^ c.l.interfaces is deprecated
<leonardr> thanks
<sinzui> Though I am certain that IPersonSet will be the very last thing we remove
<sinzui> leonardr, I am at 435 and I an very impressed.
<sinzui> leonardr, are you using no-priv because this is a well known user? Is there reason to not use a person create with the factory?
<leonardr> sinzui: i don't think it matters, i just haven't used the factory much
<sinzui> leonardr, I have no other questions. This is a nice implementation and I see you tested the state conditions
<leonardr> sinzui, thanks. can you point me to a good example of a pagetest that uses the factory?
<sinzui> I think so
<sinzui> leonardr, the factory is already in the test name space. so the minimum you need it
<sinzui> is user = factory.makePerson(name='test-user')
<sinzui> leonardr, you can also use login_person(user)
<sinzui> look in lp/registry/stories/webservice/xx-project-registry to see a lot of factory uses in a story
<leonardr> i think i have it...
<jelmer> any rc reviewers around?
#launchpad-reviews 2010-09-17
<lifeless> LFR https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/35774
<jtv> allenap: I've got a modest branch up for review as well, if you have time: https://code.launchpad.net/~jtv/launchpad/bug-594220/+merge/35770
<jtv> (bit early for you though innit?)
<jtv> StevenK: is allenap really still reviewing your branch, or is the topic outdated?
<lifeless> stale topic
* 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
<jtv> so there
<jtv> and this system has gotten too weird for me; I'll reboot
<jtv> hi henninge!
<henninge> Hi jtv!
 * StevenK gets more tempted to write a review helper bot that will manage the topic and MPs
<StevenK> I could use PoCo::IRC just to annoy people too :-P
<allenap> jtv: Sorry, I forgot to take my name out of the topic :-/
<jtv> allenap: we figured it out, no worries :)
<StevenK> allenap: And you didn't actually review my branch at all!
<allenap> StevenK: I was a whisker from finishing your branch review last night but I had to go. I'll finish it now.
<StevenK> Hehe
<StevenK> allenap: It's perfectly fine, thanks. :-)
<allenap> StevenK: Done, needs (a bit of) fixing :)
<noodles775> henninge: When you've time: http://launchpadlibrarian.net/55794871/635005-updated.png
<noodles775> There are two things I didn't do (indentation, download link) and one extra thing (binary descriptions).
<noodles775> henninge: I did try the indentation, but it seems that we explicitly set ul's and dl's to margin/padding 0px; so I think that's a site-wide decision that I didn't want to touch as part of this branch.
<noodles775> Regarding the download link, .... actually, I'll send the MP email, so your mentor can also read.
<noodles775> henninge: hrm, I'd already mentioned the link on the previous email. So, nothing more to say :)
<StevenK> allenap: I can't use the ZCML for auth, sadly. Using soyuz-team is a stop-gap first-step. When we want other people to be able to use it, it will switch to require the user being a driver of the distribution.
<allenap> StevenK: Can you explain why?
<StevenK> allenap: Why we require only soyuz-team, or the user being a driver?
<allenap> StevenK: No, why not ZCML? Is there a technical reason? I'm interested as much as anything.
<StevenK> allenap: I didn't think you can use ZCML if it requires something more complex than the permission the user has.
<allenap> StevenK: Permissions are given to user by a security adapter based upon the context. So, for an IDistroSeries, launchpad.Edit can be granted if the user is in ~soyuz-team. Take a look at some of the adapters in canonical.launchpad.security.
<StevenK> allenap: Ah, sorry -- I was skipping ahead. ~soyuz-team is not the final solution. I don't think I can do it if it's dependant on what distribution the belongs to
<StevenK> Sigh, words are not my friend today
<StevenK> allenap: I have a stand-up starting, well, when bigjools notices what time it is. Would you like a 5 minute chat about it after that?
<allenap> StevenK: :) Sure, ping me when you've got time.
<henninge> noodles775: care to paste the mp url again, please? ;-)
<noodles775> henninge: https://code.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
<StevenK> allenap: Ready when you are, are you mumble-enabled?
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [jtv]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: approved* ;-)
<noodles775> Thanks henninge
 * henninge needs to reboot computer
* bigjools changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [jtv, bigjools]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> happy birthday henninge
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: bigjools || 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
<jtv> hey abel, stealth reviewâthanks!
<noodles775> Hi adeuring, I've got a 797liner if you've time. Just let me know if you'd prefer I split it up:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
<adeuring> noodles775: nah, I'll look at the whole branch
<noodles775> adeuring: great, thanks. I'll just update the description explaining what the bloat is.
* 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: description updated.
<noodles775> adeuring: also (sorry), I've just realised that I no-longer need the generic CommentView... I'll remove it.
<adeuring> noodles775: no problem -- i just realized that I hadn't have lunch yet, will make a lunch break before I start to look at your branch ;)
<noodles775> Great, enjoy :)
<bac> hi adeuring
<bac> can i add one to your queue?
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || Reviewing: noodles775, - || queue: [bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> adeuring:  https://code.edge.launchpad.net/~bac/launchpad/lp-db-setup/+merge/35822
<gmb> adeuring, bac I have a very large (1300 lines) lazr-js branch that needs review. This adds a new Wizard widget. Can either of you guys review it?
<gmb> (A lot of those lines are '},' of course ;))
<bac> gmb: i will but you may still want to run it by a JS expert
<adeuring> bac: sure, let me first finish noodles775 review
<bac> adeuring:  no rush
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || Reviewing: noodles775, gmb-js-from-hell || queue: [bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bac, Eyefankyoo. I'll see if I can find an eggspurt.
<gmb> bac, https://code.edge.launchpad.net/~gmb/lazr-js/wizard-widget/+merge/35827, FTR.
<gmb> mars, Around?
<adeuring> noodles775: r=me
<noodles775> Thanks adeuring
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bac, gmb-js-from-hell || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews abgeÃ¤ndert
<noodles775> rockstar: Hi! When you get around to mentoring henninge's UI review, would you mind also taking a brief look at the code and make sure I'm not abusing the lp.services.comments in a way you guys didn't intend?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
<mars> gmb, pong, what's up?
<gmb> mars, I have a large (~1300-line) lazr-js branch that adds a Wizard widget (building upon work that rockstar had already done, but which hadn't landed). bac is reviewing it but he suggested that I should find a JS expert to take a look, too. Your name was top of my list. Do you have time to take a look?
<mars> gmb, unfortunately I do not have time today.
<mars> gmb, Paul and Edwin are two other good candidates
<gmb> mars, Okay, no worries. I'll ask someone else.
<gmb> Indeed.
<gmb> So, rockstar, are you free to look at how I've mangled your widget?
<gmb> ooh, that sounded wrong.
<adeuring> bac: r=me
<mars> lol
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, gmb-js-from-hell || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> adeuring:  thanks
<bac> gmb:  dumb question - should the examples work if simply loaded into ff
<gmb> bac, Yes. And that said, let me check that I didn't break them, 'cos I've just realised that I might have.
<bac> gmb:  yeah, none of them do squat for me
<bac> gmb:  could you have messed them all up?
<gmb> bac, Works for me (bar one small tweak; fixed and pushed).
<gmb> bac, Did you make build in the root of the branch?
<bac> gmb: gah
<gmb> Hah
<gmb> bac, It took me a while to figure that out, too.
<gmb> JS shouldn't be this hard. YUI, I'm looking at you, here.
<bac> gmb: ok, so i got it built and most JS examples are working now.  your wizard doesn't seem to, though, as it just shows Step One
<bac> gmb: am i doing something else wrong?
<gmb> bac, So, the 'next' button doesn't work, then?
<gmb> Note that I've spent more time on tests than on the example, so if it's lacking, that's why.
<bac> gmb: there is no next button.  just a "re-open the wizard" link
<gmb> ...
<gmb> bac, That's very, very odd.
<gmb> Can you take a look at Firebug, see if there are any errors?
<gmb> bac, In fact, I'd go so far as to say the example should be completely ignored, because it's rubbish compared to other examples.
<bac> gmb: if i load the file i do not get an overlay
<bac> but if i open firebug and then load the file then the overlay appears
 * gmb tries to reproduce
<rockstar> gmb, I'm happy to look at it if you don't have a reviewer yet.
<gmb> rockstar, bac's looking but having some issues; I'd appreciate your eye on the code itself (ignore the example, though, it's bobbins).
<gmb> Huh.
<bac> rockstar: i will do a full review per our discussion a few weeks ago that everyone should do JS reviews
<gmb> bac, I can reproduce that.
<gmb> bac, Try chromium. It works fine there.
<bac> :)
<rockstar> bac, great, thanks.
<gmb> Meanwhile, I'll try to hammer firefox into submission.
<bac> rockstar: but i suggested to gmb that he get a JS expert to look at it too
<rockstar> bac, yeah, I'd at least like to look at it from a "is it YUI-y" standpoint.
<gmb> bac, I know why it happens, I think. Hang on.
<rockstar> gmb, before you land it (and after bac reviews it) I'd love to take a whack at reviewing it as well.
<gmb> bac, Pull and rebuild. Fixed it by a cunning s/console/Y/g
<gmb> rockstar, Please do. I'm sure it's littered with horrors.
<gmb> And things I've forgotten to add tests for.
 * gmb hates doing the tests second.
<rockstar> gmb, yeah, but I think testing this kind of interaction is hard without huge things like windmill.
<gmb> rockstar, Agreed.
<bac> gmb: works brilliantly now
<gmb> bac, Well, it works 100% better than it did, anyway :)
<bac> gmb: your testing is very thorough and easy to follow
<bac> gmb: i am confused by test_wizard_needs_steps
<gmb> bac, How so?
<bac> gmb: in it you seem to be instantiating with no steps, which is a no-no, but assert that it instaniated properly
<bac> gmb: am i misreading it?
<gmb> bac, Ah, JS testing FTFL.
<gmb> bac, See the _should definitions
<bac> yeah, i did but clearly nothing clicked
<gmb> bac, test_wizard_needs_steps is understood to have passed if it raises an error.
<gmb> bac, I'll add a comment to make this clear, but it is beyond horrible that we can't do Assert.throws() or some such.
<bac> so the assert does fail
<bac> and that failure is noted in the _should
<bac> so by failing it passes
<gmb> bac, Yes
 * bac cries
<bac> yes, i knew that at one time
<bac> but my brain purged it as it was jibberish
<gmb> Yeah.
<bac> gmb: in  _renderUIWizard: you check to see that there are some steps
<bac> gmb: is that necessary since you did the check at instantiation?
<gmb> bac, Oops, no. I'll remove that.
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> gmb r=bac but i hope rockstar has time to have a look
<rockstar> bac, I will MAKE time.
<gmb> bac, ta
* adeuring changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bac, It should be noted FTR that rockstar did many of the changes for which you've given positive comments. :). However, sadly, I can't blame any problems on him.
<rockstar> gmb, the fact that you pulled it from the railroad tracks before the train came should also be noted.
<rockstar> gmb, where is this proposal?
<gmb> rockstar, Oh yes, that might help you... https://code.launchpad.net/~gmb/lazr-js/wizard-widget/+merge/35827
<allenap> bac: Could you sanity check this please? http://paste.ubuntu.com/495399/ - it's a change suggested by lifeless in https://code.edge.launchpad.net/~allenap/launchpad/bugsubscription-to-storm/+merge/35537
<allenap> bac: Argh, I've got to go! Don't worry about it. Thanks anyway.
<EdwinGrubbs> bac: I have a 950 line branch. Do you want to review it? It will still need a follow-up branch to add the cronjob before it lands.
<bac> EdwinGrubbs: i'll be happy to review it
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: Edwin || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs let me know when you've submitted a MP
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing:- || queue: [Edwin]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-queue-addmember-emails/+merge/35862
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: Edwin || queue: [-]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: I'm headed to lunch.
<bac> EdwinGrubbs ok
<bac> don't go to that place where they yell at you
<EdwinGrubbs> I wonder how they would do as code reviewers.
<EdwinGrubbs> bac: thanks for the review. I'll try to get the reply done shortly. I tried going to Starbucks after lunch, but freenode had banned its IP address. Stupid caffeinated hackers.
<bac> really?  hadn't heard of that before
<bac> EdwinGrubbs: i would suggest bip but it's a pita
<sinzui> bac ping
<bac> hi sinzui
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [-]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> bac: I am preparing a migration branch for mailman. Will you have time to review it?
<bac> sinzui: how big?
<sinzui> 417, 30 lines are not mechanical--two tests were broken by recent changes
<bac> i can definitely do it tonight
<bac> sinzui: around 6 we're leaving but i'm can do it in the car and send it when we arrive
<sinzui> it is very boring. I was just disappointed to see the tests were broken in july/August
<bac> sinzui: they were broken and then disabled?
<bac> sinzui: or are they only run on demand?
<sinzui> latter --layer=Mailman and they only pass on repeated tries
<bac> outta sight...gonna break
<sinzui> I am so angry with these tests. One that is broken does not need to be on the mailman layer.
<sinzui> I do not know where to begin fixing these tests
<bac> so when do you think the branch will be ready?
<sinzui> 5 minutes
<bac> oh, sweet
<sinzui> bac: I am still waiting for the diff to appear: https://code.edge.launchpad.net/~sinzui/launchpad/discard-list-spam-0/+merge/35885
 * bac makes my own diff
<bac> sinzui: wow!  that was boring
<bac> r=me
<sinzui> running the tests is much more boring
<sinzui> I need caffeine, Guinness, and maybe LSD to find a way to make this testable.
<EdwinGrubbs>  bac: do you know if there is a helper function like assert that raises ValueError> My single line asserts turned into 4-line if-statements.
<EdwinGrubbs> s/>/?/
<bac> EdwinGrubbs: i don't know.
<bac> EdwinGrubbs: it sounds like a good helper, though
<bac> sorry for the clutter
<EdwinGrubbs> sinzui: I was looking at volunteering with Junior Achievement. Apparently, it would take a couple hours a week for eight weeks. Is it ok if I shift my schedule on those days, and is it ok if I miss the standup on those days?
<EdwinGrubbs> sinzui: oops, this should go in the lp-registy channel.
* bac 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
<benji> EdwinGrubbs: I'd have to look at the style guide to see if it's allowed, but removing the newline after the "if" seems reasonable:  "if not value_is_good: raise ValueError('you suck')"
<EdwinGrubbs> benji: well, the main reason that the ValueError is really long is that I was making a very clear error message. I guess I really only need to include the bad value, since the variable name and type can be found in the source code.
<benji> EdwinGrubbs: I suggest a message like "foo_var must be a positive integer, got %r instead" % (foo_var,)
<EdwinGrubbs> providing that long error message over and over again seems like a perfect reason for a helper function, but providing the variable and the variable name either means adding a redundant looking arg, using vars(), or crawling up the stack. bleh.
 * benji goes afk
<lifeless> benji: EdwinGrubbs: FWIW: practicality over purity.
<lifeless> our style guide is a *guide* not a straightjacket.
#launchpad-reviews 2010-09-18
* bryceh changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/test/+merge/35917
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/fakelibrarian/+merge/35918
#launchpad-reviews 2010-09-19
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/malone/+merge/35941
<lifeless> mwhudson: so what happens on monday mornings
<mwhudson> lifeless: not a lot
<lifeless> I ask you for reviews!
<mwhudson> heh heh
<lifeless> mwhudson: up for it?
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/test/+merge/35917 https://code.edge.launchpad.net/~lifeless/launchpad/fakelibrarian/+merge/35918 https://code.edge.launchpad.net/~lifeless/launchpad/malone/+merge/35941
<mwhudson> lifeless: i'll get to them, but possibly not in the next few minutes
<lifeless> no panic
<mwhudson> are any of them now now now urgent?
<lifeless> other than the general sense of urgency I have to help LP be fast, no.
<mwhudson> cool
<mwhudson> then i'll see if any of my email is, and then do the reviews :)
<lifeless> sweet, thanks
<lifeless> mwhudson: also, I'd like just a regular 'approve' vote on https://code.edge.launchpad.net/~lifeless/launchpad/cp/+merge/35965 for ec2land happy happy joy joy ness.
<mwhudson> lifeless: done
<lifeless> thumper: piiing; I'd like your r-c stamp on the same, same reasons as before [toolchain implements old process only, life sucks, yadayadayada
<thumper> lifeless: looking
<mwhudson> lifeless: looking at your branches now
<lifeless> mwhudson: thanks
<mwhudson> lifeless: is the name of the important method on TestWithFixtures 'useFixture'?
<mwhudson> mm, yes
