=== 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
[06:09] <lifeless> thumper: got 2 minutes?
[06:10] <thumper> yeah
[06:10] <lifeless> incremental eyeballs on a  branch of mine
[06:10] <lifeless> just need to know if one commit is sane
[06:10] <lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/oops/revision/11529
[06:10]  * thumper looks
[06:10] <lifeless> well, I know its sane, but maybe I'm wrong :P
[06:11] <thumper> fcking loggerhead
[06:12] <lifeless> basically I missed some stuff and ec2 found it for me
[06:14] <thumper> lifeless: I'm pretty sure you don't need the ... on a line between Traceback (most recent call last): and the exception itself
[06:15] <thumper> lifeless: it is handled by the doctest
[06:15] <thumper> lifeless: but up to you if you feel like removing it
[06:15] <lifeless> oh, ok, I was following suite
[06:16] <thumper> yeah
[06:16] <thumper> I don't think many people know about that
[06:16] <thumper> and I don't think we have a standardised practice
[06:16] <lifeless> shrug
[06:16] <lifeless> doctest spec says it ignores it or something
[06:17] <lifeless> I don't think standardising that is going to be helpful
[06:29] <lifeless> thumper: so its ok?
[06:35] <lifeless> mwhudson: ping
[06:35] <lifeless> mwhudson: https://code.edge.launchpad.net/~lifeless/launchpad/memcache/+merge/35220 you approved but you didn't comment, ec2land hates that
[06:35] <lifeless> (probably suffered in the timeout mess we had)
[06:36] <lifeless> jtv: I'm guessing mwhudson has EOD'd, perhaps you could be a proxy for him ?
[06:37] <jtv> lifeless: what's it about?  I'm stuck in the rain using batteries and a GSM, so hard to load entire LP pages.
[06:37] <lifeless> making memcache controlled by feature flags
[06:38] <lifeless> its been reviewed by mwhudson
[06:38] <jtv> but not landed?
[06:38] <lifeless> but he forgot the vote bit :P
[06:38] <jtv> I can land that when the rain stops and I get home.
[06:38] <lifeless> so its marked approved, has a comment from him saying approved, but no -vote-
[06:38] <StevenK> No one has claimed the review
[06:39] <lifeless> StevenK: you don't need to claim
[06:39] <lifeless> just do a review, and if you're in the team you effectively claim it
[06:39] <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.
[06:40] <jtv> StevenK: can you take this one then?
[06:40] <StevenK> thumper: Are you around to rubber-stamp too?
[06:42] <StevenK> lifeless: That should be good now?
[06:42] <lifeless> thanks
[06:42] <StevenK> lifeless: Now read my PM, damn it :-P
[06:42] <lifeless> did you have a question in there?
[06:42] <lifeless> I thought it was just commentary ;P
[06:43] <StevenK> I was expecting at least a comment on the commentary :-)
[06:43] <lifeless> sorry, magic 8 ball says ask again later
[08:17] <henninge> jtv: good morning!
[08:17] <jtv> hi henninge!
[08:17] <henninge> jtv: you are not on irc.c.c
[08:17] <henninge> ;-)
[08:17] <jtv> :(
[08:17] <jtv> I guess it's something this provider blocks… _sometimes_
[08:17] <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
[08:18] <henninge> strange
[08:19] <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)
[08:20] <jtv> I'll set up an illegal proxy.  Hang on.
[08:21] <henninge> :-D
[08:21] <henninge> jtv: I'll come visit you in jail ...
[08:21] <jtv> Not sure that's allowed.
[08:21] <henninge> ... good reason for a sprint in Bangkok.
[08:22] <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.
[08:23] <henninge> We'll expense it.
[08:28] <henninge> So, jtv, how is life in Mordor?
[08:29] <jtv> henninge: well the first thing you notice is this enormous black cloud hanging over it
[08:29] <jtv> I don't care much for the constant nazgûl patrols either.
[08:29] <henninge> yes, that does get your spirits down.
[08:30] <henninge> I hear the black gate is about to open!
[08:30] <henninge> but you should not leave that way, there's a huge human army waiting on the other side.
[08:33] <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
[08:43] <stub> noodles775: versien isn't spelled that way
[08:44] <noodles775> hrm... "base version to derived versien." I'll update it.
[08:45] <stub> noodles775: So source_version and parent_source_version are debian version strings? If so we might want a CHECK constraint to ensure that.
[08:46] <stub> ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK (valid_debian_version(source_version));
[08:47] <noodles775> stub: yes they are. Thanks.
[08:49] <stub> noodles775: With the new UNIQUE constraint, you can drop the distroseriesdifference__derived_series__idx index.
[08:50] <stub> noodles775: The name of the new UNIQUE constraint should be distroseriesdifference__derived_series__source_package_name__key
[08:51] <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.
[08:51] <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
[08:51] <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?
[08:52] <noodles775> stub: er, ignore that (I was thinking about the versions fields).
[08:52] <stub> If that is what our code expects, then yes. ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid...
[08:52] <stub> ok
[08:53] <stub> noodles775: patch-2208-12-0.sql
[08:54] <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.
[08:54] <noodles775> Thanks.
[08:55] <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)
[08:55] <noodles775> Great, thanks.
[08:56] <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)
[08:58] <stub> or just 'source_version IS NOT NULL OR parent_source_version IS NOT NULL' is better
[08:58] <noodles775> Yep.
[08:58] <stub> I'm not sure which of those two rules is appropriate
[09:00] <stub> Can they both be NULL? Can they both be NOT NULL?
[09:00] <noodles775> They cannot both be null, but they can both be NOT NULL.
[09:02] <stub> ok... so CHECK (source_version IS NOT NULL OR parent_source_version IS NOT NULL)
=== mrevell is now known as mrevell-lunch
=== mrevell-lunch is now known as mrevell
[13:59] <jcsackett> salgado, ping.
[14:00] <salgado> jcsackett, hi there.  let me guess, it's about that UI review? :)
[14:00] <jcsackett> yup. :-) just making sure you knew about it.
[14:01] <salgado> jcsackett, I do, yes.  haven't done it yet because I was off last Thursday and Friday, but will do it shortly
[14:01] <jcsackett> salgado. very cool. thanks!
[14:27] <adeuring> henninge: could you review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-581626/+merge/35281 (short, simple, boring)
=== Ursinha-afk is now known as Ursinha
[14:30] <henninge> adeuring: looking
[14:30] <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
[14:45] <henninge> adeuring: "The text for each bigs ..." is meant to be "bugs", I guess.
[14:45] <adeuring> henninge: ouch... yes, or course
[14:47] <henninge> adeuring: also, the for loop cound be a function that you can use twice, e.g. "print_bug_links"
[14:47] <henninge> s/cound/could/
[14:47] <adeuring> henninge: make sense, I'll add this function
[14:47] <henninge> adeuring: with that: r=me ;-)
[14:48] <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
[14:50] <salgado> jcsackett, did you see that your branch has a few conflicts?
[14:50] <salgado> nm, I reloaded the page and see that they're now fixed
[14:51] <jcsackett> salgado: phew. i was confused for a sec--was sure i had done an update this morning. :-)
[14:52] <bac> jml: can you review https://code.edge.launchpad.net/~bac/launchpad/cherrypick-633926/+merge/35286  for a cherry pick?
[14:52] <jml> bac, sure. after this call.
[14:52] <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
[16:19] <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
[16:19] <abentley> jtv, sure.
[16:19] <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
[16:29] <abentley> jtv, did you end up testing this with chameleon?
[16:30] <jtv> abentley: no, just compared to devel
[16:33] <abentley> jtv, do you think it's likely that chameleon will soon become our template engine and invalidate this work?
[16:34] <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?
[16:35] <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."
[16:35] <jtv> Yes.  Do you think it will be?
[16:35] <jtv> Because the speedup I'm seeing is not to be sneezed at.
[16:36] <jtv> I think it can go one of two ways:
[16:36] <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.
[16:37] <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.
[16:37] <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.
[16:38] <jtv> abentley: I hope I don't sound contrary.  You're raising valid points and I'm trying to give you a useful answer.
[16:39] <abentley> jtv, I don't have any idea yet of whether we should merge this.
[16:39] <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.
[16:40] <abentley> jtv, I don't know how feasible it is to try it out in chameleon, even if it is in tree.
[16:40] <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.
[16:41] <jtv> Scratch the "was."
[16:41] <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.
[16:42] <abentley> jtv: in the b) case, I always worry about whether there will be follow-up.  If there isn't we get tech debt.
[16:42] <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.
[16:44] <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.
[16:45] <jtv> I'm using "since" in the purely temporal sense here.
[16:46] <abentley> Okay, I'm comfortable with that.
[16:47] <jtv> Note that the main loop is still TAL, so we may still get _some_ speedup from a faster templating engine.  ;-)
[16:48] <jtv> I'm being flippant.
[16:48] <jtv> Note to self: don't do that in reviews.
[16:49] <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.
[16:50] <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!
[16:52] <abentley> jtv, Ah.  So it's not generating lots of links?  Or it's generating lots of links but without lots of DB cost?
[16:52] <jtv> abentley: the latter.
[16:52] <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.
=== matsubara is now known as matsubara-lunch
[16:59] <abentley> jtv, Do you know why canonical_url is slow?  Would you lose your performance gains if you used the ObjectFormatter directly?
[16:59] <jtv> abentley: permissions checks seem to be the main culprit.
[17:00] <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."
[17:00] <jtv> (from the current object up to the site root, that is)
[17:01] <jtv> canonical_url is recursive, and could possibly do with some caching for this sort of repetitive work.
[17:01] <jtv> I say "possibly" because I think it'd have to be one of those ultra-small, catch-one-common-case caches.
[17:02] <jtv> Which is easy for me to say, but who knows if it actually works out (and faster than not doing it)
=== salgado is now known as salgado-afk
[17:15] <abentley> jtv, what's the purpose of getUserAndEmail?
[17:15] <abentley> jtv, I mean, _makeUserAndEmail
[17:15] <jtv> abentley: to create a user that I can log in as
[17:15] <abentley> jtv, You can log in as anyone you create with factory.makePerson().
[17:15] <jtv> So small that it's not worth moving to a factory, but it saves just enough that it's useful for the test
[17:16] <jtv> Can I just do login(person.email)?
[17:16] <abentley> jtv, You know about login_person and "with person_logged_in"?
[17:16] <jtv> No
[17:16]  * jtv greps
[17:17] <abentley> jtv, lp.testing, I believe.
[17:17] <jtv> yup, found it.  I'll use that fortwith.
[17:17] <jtv> forthwith.
[17:21] <jtv> I'll also pull the login out of _makeView
[17:25] <abentley> jtv, cool.
[17:25] <jtv> abentley: change pushed.
[17:29] <abentley> jtv, r=me
[17:30] <jtv> abentley: \o/ thanks!
=== Ursinha is now known as Ursinha-lunch
=== deryck is now known as deryck[lunch]
=== Ursinha-lunch is now known as Ursinha
=== matsubara-lunch is now known as matsubara
=== deryck[lunch] is now known as deryck
=== 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
[19:43] <jcsackett> jtv: https://code.edge.launchpad.net/~jcsackett/launchpad/no-canonical-urls-628247/+merge/35301
[19:45] <jcsackett> jtv, ignore me, i misread the header. :-P
[19:45] <jcsackett> abentley, instead i must bother you with the above. :-)
[19:45] <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
[19:49] <abentley> jcsackett, Could you explain the conditional in lib/canonical/launchpad/interfaces/validation.py, please?
[19:51] <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.
=== Ursinha is now known as Ursinha-brb
[19:51] <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.
[19:56] <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?
[19:59] <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
[20:00] <jcsackett> i'll make that change.
[20:00] <abentley> jcsackett, cool.  My main reason is it looks unnecessary to have the same three-way conditional in two spots.
[20:01] <abentley> jcsackett, it looks like you have unnecessary wrapping on 182 and 282 in the patch.
[20:02] <abentley> Sorry, 182 and *202*
[20:03] <abentley> jcsackett, why the blank line at 76?
[20:04] <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.
[20:04] <jcsackett> abentley line 76 looks like my editor's import reflow macro errorred. fixing that as well.
[20:06] <abentley> jcsackett, at 187, can you pass in an email address so you don't need to remove the security proxy later?
[20:06] <jcsackett> abentley: i can.
[20:06] <abentley> jcsackett, please do, then.
[20:07] <jcsackett> abentley: just did. :-P
[20:08] <abentley> jcsackett, I think that's all the changes I can see.
[20:09] <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.
[20:10] <abentley> jcsackett, you could just raise LaunchpadValidationError.
[20:11] <jcsackett> abentley: ah yes. that's obvious. i must need more coffee.
[20:11] <jcsackett> thanks.
[20:12] <abentley> jcsackett, np.
[20:13] <abentley> jcsackett, oh, your spelling of "associaiton" should probably be tweaked if that text makes it into the final version.
[20:13] <jcsackett> abentley: agreed, but i don't think the comment will survive the refactor. :-)
[20:24] <deryck> sinzui, hi.  Can you do a CSS/UI review for me?
[20:25] <sinzui> deryck, I can, but I hope salgado-afk can have a turn first so that he can become a UI reviewer
[20:25] <deryck> Ah, ok.  I can request him as a reviewer and wait.  I'm not in a hurry.
[20:33] <abentley> jcsackett, you'll ping me when you've pushed your changes?
[20:33] <jcsackett> abentley: i was just about to ping you, as a matter of fact. changes pushed and an incremental diff added as a comment.
[20:33] <abentley> jcsackett, thanks.
[20:34] <jcsackett> abentley: np.
[20:35] <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.
[20:35] <jcsackett> abentey: that's good to know, thanks.
[20:35] <jcsackett> (both the attach, and the incrementals being automatic)
[20:37] <abentley> jcsackett, r=me.
[20:37] <jcsackett> abentley: thanks. and thanks for helping me rethink that--in retrospect that was some truly ugly code. :-)
[20:37] <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
[20:48] <sinzui> abentley, I have a few branches from the weekend that can be reviewed (they are low priority)
[20:48] <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
=== matsubara is now known as matsubara-afk
=== 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
[21:52] <mwhudson> lifeless: i'm pretty sure i voted on everything, but maybe stuff got eaten by the issues yesterday :(
[21:54] <lifeless> yes
[21:54] <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
[22:30] <mwhudson> i thought i did check everything
[22:30] <mwhudson> oh well
[22:38] <lifeless> mwhudson: you commented 'seems fine', approved the rview, but your comment wasn't marked 'approve'
[22:38] <mwhudson> ah