| === matsubara-afk is now known as matsubara | ||
| === matsubara is now known as matsubara-afk | ||
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/fix-mailman-excessive-queries/+merge/39335 anyone? | 03:51 |
|---|---|---|
| bac | hi thumper, i'll look at it | 04:03 |
| thumper | bac: thanks | 04:03 |
| thumper | check out this: 6278 OOPS-1759XMLP1 MailingListApplication:MailingListAPIView | 04:03 |
| thumper | 6278 statements | 04:03 |
| bac | hi thumper -- really interesting branch...amazing savings | 04:47 |
| thumper | bac: should be | 04:48 |
| bac | i got a little caught up with acknowledgeMessagesWithStats thinking it updated all in the message_set | 04:48 |
| bac | but then i figured it out | 04:48 |
| bac | only one issue. i think you meant acknowledgeMessagesWithStats to be acknowledgeMessagesWithStatus | 04:48 |
| thumper | ah... yeah | 04:49 |
| thumper | copy and paste FTW, at least the code is consistent | 04:49 |
| thumper | fixed typo, and pushing | 04:51 |
| thumper | bac: thanks | 04:52 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jtv1 is now known as jtv | ||
| jtv | adeuring: perhaps you could review my branch? I'd prefer not to wait for an OCR because I believe the next OCR is me. :-) | 12:13 |
| adeuring | jtv: give me 30-40 minutes for lunch, then I'll look | 12:17 |
| jtv | adeuring: that's wonderful, thanks! | 12:17 |
| === matsubara-afk is now known as matsubara | ||
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | leonardr: I'm expecting Abel back in a few; he says he can look at mine if needed. | 12:50 |
| leonardr | jtv: ok, thanks | 12:50 |
| leonardr | StevenK, which branch do you want reviewed? i don't see it | 13:35 |
| StevenK | leonardr: It already has been, sorry. | 13:35 |
| leonardr | ok, np | 13:35 |
| leonardr | jtv, want me to take care of yours? | 13:35 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | leonardr: time is short now, so yes please! | 13:36 |
| StevenK | I threw it at ec2 from davidm's hotel room, but was then unable to connect to IRC from my own room | 13:36 |
| leonardr | jtv: ok | 13:36 |
| jtv | thanks | 13:36 |
| adeuring | leonardr, jtv: I'm more or less done with the review | 13:36 |
| jtv | ah! | 13:36 |
| jtv | In that case, thanks leonardr but your services are no longer required. :-) | 13:37 |
| leonardr | great | 13:37 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| adeuring | jtv: only a minor issue: doc/potmsgset.txt has the new text "+The "suggestive templates" cache is up to date." (line 21). shouldn't this be something like "update the cache"? | 13:38 |
| jtv | adeuring: well it's supposed to be narrative, not code comments. (And for a code comment, that would be a bad one. :-) | 13:39 |
| jtv | It's really just stating the expected situation, not anything interesting that the test does. | 13:39 |
| adeuring | jtv: well, ok. But I inderstand this as a statement "the cache indeed up to date", while you define a helper function which does an update. What about a comment inside the definition od the function? | 13:40 |
| jtv | adeuring: any ideas on what it should say? | 13:41 |
| jtv | adeuring: ah, this is the one doctest where I need the function more than once. | 13:41 |
| adeuring | jtv: argh, I missed the existing doc string... never mind. Let's leave it as it is | 13:41 |
| jtv | Well _of course_ there's a docstring. :-) | 13:42 |
| adeuring | jtv: yeah... doesn'r seem to be my best day... | 13:42 |
| jtv | Well you're helping me out with a review. That's definitely a good deed for the day. | 13:42 |
| adeuring | anyway, r=me then. Just a parnoid question: This cache is already populated via cron job or whatever? | 13:43 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | jcsackett, i'm going to review https://code.edge.launchpad.net/~jcsackett/launchpad/hidden-project-configuration-636000/+merge/39306 | 13:51 |
| jcsackett | leonardr: sounds good. i was going to throw it in the queue sometime this morning anyway. :-) | 13:52 |
| leonardr | all right | 13:54 |
| jtv | adeuring: thanks again | 13:55 |
| adeuring | jtv: welcome. just a paranoid question:; this cache is updated in production on a regular basis, I assume? | 13:56 |
| jtv | adeuring: yes, daily. | 13:56 |
| jtv | We had a replication problem with it last cycle, so good thing we phased it. | 13:57 |
| jtv | Turns out if you don't have foreign keys pointing into the replication set (or the other way, I guess) your table is not replicated by default. | 13:57 |
| adeuring | interesting | 13:57 |
| jtv | Not the word I used at the time. :-) | 13:58 |
| leonardr | jcsackett: i'm sure you have a good reason (?), but i'm honor-bound to ask why you seem to have copied a bunch of javascript into pillar.js. is there any way we can refactor more than toggle_collapsible? | 14:25 |
| jcsackett | leonardr: you mean refactor more than activate_collapsibles? toggle_collapsible was the one i didn't touch and simply sought to use with divs. | 14:26 |
| leonardr | jcsackett: i think we're saying the same thing. you're reusing Y.lp.toggle_collapsible | 14:27 |
| leonardr | but that code is at the end of a huge chunk of code that looks copied from somewhere else (eg. it's got a gmb XXX from over a year ago in it) | 14:27 |
| leonardr | what changes did you make to the code after copying it? can we refactor those changes? | 14:28 |
| jcsackett | leonardr: there's another function called simply activate_collapsible in lp.js that this is borrowed from; i tried refactoring it initially, but it's very specific to one set of expectations and my attempts to refactor led to horror. | 14:30 |
| jcsackett | the majority of changes in this are to set it to work with divs, not fieldsets, and then simplify based on the small set of expectations this has. much of the comments were left b/c they were still applicable. | 14:30 |
| leonardr | i well know the horror of refactoring javascript | 14:31 |
| * jcsackett smiles. | 14:31 | |
| jcsackett | it reached the point where i had a choice of two similar and parsible functions or one terrifying (to me at least) one. | 14:32 |
| leonardr | jcsackett: at the least, we should track the technical debt. can you file a bug mentioning the two places where there's similar code, and add an XXX? that way if we figure out how to refactor in the future it'll be clear where the work lies | 14:33 |
| jcsackett | leonardr: certainly i can do that; i'll probably assign myself the bug for honor's sake too--i wasn't satisfied with this, but it seemed the best of bad alternatives. | 14:34 |
| leonardr | jcsackett: indulge my laziness and tell me what tal:condition="view/configuration_links" does? why would that condition be false? | 14:53 |
| leonardr | jcsackett: it looks like it would be false if you had a view of a pure IPillar rather than anything more specific? | 14:57 |
| jcsackett | jcsackett: predates this branch; i can't return false but it can return none--it provides the configuration links if you can see them. if you can't see them then you can't edit things, i think. | 14:57 |
| jcsackett | or actually not None, but an empty list. | 14:57 |
| jcsackett | i think it also sets up some stuff about what you should see (e.g. configuration status). | 14:57 |
| leonardr | ok, permission, that makes sense | 14:59 |
| leonardr | jcsackett, similar refactoring question about head_epilogue. how much work is it to reuse the collapsible and collapsed styles (making them general styles instead of <div>-specific)? | 15:01 |
| jcsackett | leonardr: ah, that shouldn't be too hard; i just thought based on some other pages if it only applied to one page that's where we put it. i'll move it. | 15:02 |
| leonardr | jcsackett: it won't apply to one page if you reuse it for the fieldset collapsing code? | 15:03 |
| leonardr | if i'm wrong, then don't move it | 15:03 |
| jcsackett | leonardr: the fieldset already has styles set that aren't quite the same. | 15:03 |
| leonardr | jcsackett: ok, don't bother, but add it to the bug so we can take another look later | 15:04 |
| jcsackett | leonardr: dig. | 15:04 |
| benji | leonardr: the exception you showed me on the keyring branch yesterday also happens on launchpadlib trunk | 15:08 |
| leonardr | benji: well, damn | 15:08 |
| leonardr | i'll look into it after i finish this review | 15:08 |
| leonardr | jcsackett: this question may expose my ignorance more than anything else, but... | 15:13 |
| leonardr | lines 387-388 | 15:13 |
| leonardr | client.click(link=u"Configuration links") triggers some javascript. does it then do anything to call waits.forPageLoad? surely the page has already loaded? | 15:14 |
| leonardr | or does forPageLoad do something else? | 15:14 |
| leonardr | maybe mars knows better than me -^ | 15:17 |
| leonardr | jcsackett: i had a question while you were out | 15:19 |
| leonardr | <leonardr> jcsackett: this question may expose my ignorance more than anything else, but... | 15:19 |
| leonardr | <leonardr> lines 387-388 | 15:19 |
| leonardr | <leonardr> client.click(link=u"Configuration links") triggers some javascript. does it then do anything to call waits.forPageLoad? surely the page has already loaded? | 15:19 |
| leonardr | <leonardr> or does forPageLoad do something else? | 15:19 |
| leonardr | also, do the windmill tests run in a deterministic order? can you be sure that test_configured_starts_collapsed runs after test_not_fully_configured_starts_shown? | 15:20 |
| jcsackett | for the first question: so, there's no generic "wait" thing in windmill that i could find, and the test would fail because the class wasn't actually removed (or added depending on test) when it then went to test it. | 15:21 |
| jcsackett | there's wait.sleep() but that's documented as pausing the browser, which isn't really appropriate. | 15:21 |
| jcsackett | i can add a comment explaining that it's a generic wait, and as i look at it i should decrease the timeout. | 15:22 |
| leonardr | jcsackett: sounds good. i bet mars has the answer | 15:22 |
| jcsackett | for the second question: it doesn't matter (or it shouldn't) which order they run in; since the product is created in setup, each test should start with a product that's not configured. | 15:23 |
| leonardr | jcsacket: if that's the case, it looks like both products are created with the same name ('hidden-configs'). how is that possible? | 15:25 |
| jcsackett | leonardr: in my experience all tests seem to have a built in teardown that destroys things created in setUp. | 15:28 |
| leonardr | jcsackett: ok, as long as it works | 15:28 |
| jcsackett | it works in many other unittests; i'm not sure about windmill. | 15:28 |
| jcsackett | would mars be the person to check that with? | 15:28 |
| leonardr | mars: i guess. my concern, now that i've dug through the code, is that the windmill testing layer doesn't inherit from FunctionalLayer, which is where i'd imagine such cleanup code to be | 15:32 |
| leonardr | it's not in TestCaseWithFactory, unless the act of creating a LaunchpadObjectFactory() in setup obliterates any objects created by any other factory | 15:33 |
| leonardr | jcsackett: sorry, misaddressed to mars -^ | 15:33 |
| jcsackett | leonardr: i can add a teardown method to be sure. | 15:34 |
| lifeless | leonardr: old objects are cleaned up by the db reset at the end of the test | 15:34 |
| jcsackett | score. | 15:34 |
| lifeless | so DatabaseLayer | 15:34 |
| leonardr | jcsackett: and i've confirmed that setUp is run before each test | 15:35 |
| leonardr | so you're fine | 15:35 |
| lifeless | jcsackett: btw tearDown is (roughly speaking) deprecated | 15:35 |
| lifeless | jcsackett: its generally much better to use addCleanup (but not always - explore them both, use your judgement) | 15:35 |
| jcsackett | lifless: dig. in my experience so far it's all just nicely taken care of. | 15:36 |
| jcsackett | lifeless, rather. ^ | 15:36 |
| leonardr | jcsackett: this is the kind of thing you get when i review launchpad branches because most of the time i'm off in my own little world | 15:36 |
| jcsackett | leonardr: all good. it helps to have someone examining my assumptions. :) | 15:37 |
| leonardr | jcsackett: r=me with minor changes | 15:49 |
| jcsackett | leonardr: cool, thanks! | 15:49 |
| jcsackett | i'll make those changes shortly. | 15:49 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | mars, do you want a review of https://code.edge.launchpad.net/~mars/launchpad/add-profiling-feature-flag/+merge/39179 or do you need to do some work to address lifeless's comment? | 15:50 |
| === matsubara is now known as matsubara-lunch | ||
| === Ursinha is now known as Ursinha-lunch | ||
| === fjlacoste is now known as flacoste | ||
| bigjools | leonardr: hi, I've got a small branch up for review that exposes /builders on the API if you can take it? | 17:04 |
| === benji is now known as benji-lunch | ||
| leonardr | bigjools, sure | 17:05 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | leonardr: thank you | 17:07 |
| === matsubara-lunch is now known as matsubara | ||
| leonardr | bigjools: "(usually a member of the buildd-admins team)" that seems specific to the way we use launchpad rather than the code itself. is that okay? | 17:22 |
| bigjools | leonardr: buildd-admins are a celebrity in LP | 17:34 |
| leonardr | bigjools: ok, you're good then | 17:34 |
| leonardr | i just have some minor wording suggestions | 17:34 |
| bigjools | sure thing | 17:35 |
| leonardr | bigjools: r=me with very minor changes, https://code.edge.launchpad.net/~julian-edwards/launchpad/api-expose-builders/+merge/39379 | 17:42 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | leonardr: great, thanks | 17:42 |
| bigjools | leonardr: can you mark your review approved, I think you just landed a comment only. | 17:43 |
| leonardr | oops | 17:44 |
| bigjools | thanks for the suggestiond | 17:44 |
| bigjools | leonardr: FWIW it looks like lp_attributes already sorts in a good way | 17:47 |
| leonardr | bigjools: oh, i guess i used to be smarter than i am now | 17:47 |
| bigjools | :) | 17:47 |
| bigjools | it puts the boilerplate stuff first then the sorted attributes | 17:47 |
| === benji-lunch is now known as benji | ||
| === Ursinha-lunch is now known as Ursinha | ||
| leonardr | rockstar, can i get a quick review from you? | 18:58 |
| leonardr | https://code.edge.launchpad.net/~leonardr/launchpadlib/fix-token-authorization/+merge/39391 | 18:58 |
| rockstar | leonardr, it may be lossy, as I'm at UDS. | 18:58 |
| leonardr | rockstar: ok, if mars responds in the next 5 minutes and says he can do it never mind. otherwise, it doesn't have to be super quick | 18:59 |
| leonardr | (or if someone else can take it) | 18:59 |
| rockstar | leonardr, the code looks good, reading the description now. | 18:59 |
| mars | leonardr, ? | 18:59 |
| leonardr | mars, just seeing if you want to take a review, since i don't think you're at uds | 19:00 |
| mars | leonardr, sure | 19:00 |
| mars | I'm in and out, but I'll look - 38 lines, that's a good size :) | 19:00 |
| rockstar | mars, I've got it. | 19:01 |
| rockstar | (If it was any bigger, I would just get it now) | 19:02 |
| leonardr | ok, rockstar has it | 19:02 |
| mars | k | 19:02 |
| mars | thanks rockstar | 19:02 |
| * rockstar high fives mars and yells "TEAMWORK!" | 19:02 | |
| mars | rockstar, btw, if you see Martin Pool, ask him if he has a moment to review my Feature Flags Test Fixture branch - people would love to see that land | 19:03 |
| rockstar | mars, okay. I'll keep an eye out. | 19:03 |
| mars | thank you | 19:03 |
| benji | sounds good | 19:29 |
| benji | leonardr: almost forgot: https://code.edge.launchpad.net/~benji/wadllib/missing-files/+merge/39400 | 20:56 |
| leonardr | benji: r=me | 20:59 |
| benji | leonardr: cool; shall I do a release? | 20:59 |
| leonardr | benji: yes, please. let me know if you're missing anything from the process | 20:59 |
| benji | k | 20:59 |
| === fjlacoste is now known as flacoste | ||
| === Ursinha is now known as Ursinha-brb | ||
| === matsubara is now known as matsubara-afk | ||
| === henninge is now known as henninge_ | ||
| === leonardr changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === henninge_ is now known as henninge | ||
| benji | leonardr: I've responded to your keyring question/issue. | 22:03 |
| leonardr | benji, thanks | 22:03 |
| leonardr | benji: that makes a lot of sense, thanks | 22:04 |
| leonardr | i'm eod but i'll review your changes in a bit | 22:04 |
| leonardr | benji: how tough would it be to test the storage and retrieval of the same application name w/two different service roots? | 22:28 |
| benji | leonardr: trivial; shall I add that? | 22:29 |
| leonardr | benji: yes please | 22:29 |
| benji | leonardr: test added | 22:34 |
| benji | leonardr: oh, I need to be granted access to wadllib on PyPI to complete the release | 22:36 |
| leonardr | benji: sure | 22:40 |
| leonardr | benji, you are benji on pypi? | 22:49 |
| benji | leonardr: yep | 22:50 |
| leonardr | benji, the role is yours | 22:55 |
| benji | thanks, the upload worked | 22:55 |
| leonardr | benji, i can't parse "(this assertion is of the test mechanism, not a test assertion)." | 22:59 |
| leonardr | this assertion is veriyging the test mechanism? | 23:00 |
| benji | exactly; I'm verifying my assumption that there will only be two sets of credentials stored in the fake keyring | 23:00 |
| leonardr | benji: let me run some additional assertions by you | 23:00 |
| leonardr | 1. the fake keyring starts out empty | 23:00 |
| leonardr | 2. the application key has a specific form incorporating the application name and the hostname (?) | 23:01 |
| leonardr | i was hoping for #2 but i'm willing to be convinced it's not necessary/ not appropriate to test here | 23:02 |
| benji | 1 seems reasonable | 23:02 |
| benji | at first I thought 2 was overconstraining things, but we do want to be sure the keys remain reable between releases of launchpadlib, so it makes sense | 23:04 |
| benji | I also want to add an assertion that the application name is include in the key | 23:04 |
| leonardr | isn't that part of 2? | 23:05 |
| benji | indirectly, but I think all three (app name, server name, overall structure) are important | 23:06 |
| benji | and it's cheap to test them all, so... | 23:06 |
| leonardr | ok, go for it | 23:07 |
| benji | leonardr: done | 23:11 |
| * leonardr looks | 23:14 | |
| leonardr | benji: r=me, though i don't think we should merge this yet | 23:20 |
| benji | leonardr: k | 23:21 |
| === Ursinha-brb is now known as Ursinha | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!