| thumper | lifeless: ping | 05:05 |
|---|---|---|
| thumper | mwhudson: ping | 05:06 |
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/better-errors-for-translate-path/+merge/38178 | 05:06 |
| thumper | jtv: ping? | 05:06 |
| jtv | thumper: pong! | 05:06 |
| jtv | need a review? | 05:07 |
| thumper | please | 05:07 |
| thumper | it is very simple | 05:07 |
| jtv | I know, I've done it before thanks | 05:07 |
| * thumper pokes jtv | 05:07 | |
| * jtv does not like that | 05:07 | |
| * thumper pokes jtv | 05:07 | |
| * thumper sniggers | 05:07 | |
| jtv | Good thing I just cleaned out the pigsty in the kitchen and got the coffee brewing. I'm in a good mood. | 05:07 |
| thumper | good | 05:08 |
| jtv | Yes, good. | 05:09 |
| jtv | Wow, that's a long opening sentence in the MP description. | 05:09 |
| jtv | thumper: in test_translatePath_branch_alias_no_linked_sourcepackage_branch, isn't the path you're constructing a sourcepackage path? The comment says it's a distroseriessourcepackage. | 05:17 |
| jtv | Sorry, I got the terminology all wrong there. | 05:17 |
| thumper | technically | 05:17 |
| jtv | I mean: aren't you constructing a distribution source package path? | 05:17 |
| thumper | yes | 05:17 |
| thumper | I am | 05:17 |
| lifeless | thumper: I've rc'd it for you | 05:17 |
| jtv | A distroseries source package is actually a source package, isn't it? | 05:17 |
| thumper | lifeless: ta | 05:18 |
| lifeless | thumper: but please let edwin know directly. | 05:18 |
| jtv | hi lifeless | 05:18 |
| lifeless | hi jtv | 05:18 |
| thumper | jtv: not really | 05:18 |
| thumper | jtv: but the linking is at the source package leve | 05:18 |
| thumper | l | 05:18 |
| thumper | a branch linked for a distribution source package is the source package linked to the active distro series | 05:18 |
| jtv | I see | 05:20 |
| jtv | thumper: another thing… I can't make head or tails of the comment in test_translatePath_branch_alias_invalid_product_name | 05:20 |
| * thumper hangs head | 05:21 | |
| thumper | sorry | 05:21 |
| thumper | forgot about finishing that | 05:21 |
| jtv | Ha! Got you. | 05:22 |
| thumper | finished off and pushed | 05:23 |
| === jtv is now known as jtv-afk | ||
| === jtv-afk is now known as jtv | ||
| === jtv is now known as jtv-eat | ||
| === jtv-eat is now known as jtv | ||
| === StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | hi jtv, would you have time for a translations review? | 10:03 |
| jtv | bac: I would | 10:04 |
| jtv | fork it over | 10:04 |
| bac | jtv: thanks! https://code.edge.launchpad.net/~bac/launchpad/bug-652280-pg-trans/+merge/38195 | 10:04 |
| jtv | bac: how's the heat? | 10:04 |
| bac | jtv: 84F ... nothing to complain about | 10:04 |
| bac | jtv: the smoke and smog -- those i'll complain about | 10:04 |
| jtv | only 29°C? wow, nice & cool | 10:08 |
| jtv | or as they would say here, collllddd! | 10:08 |
| jtv | It's also about 302 Kelvin | 10:08 |
| jtv | apparently | 10:08 |
| jtv | bac: with all the unexplained parameters to create_initialized_view that are just passed on to create_view, I'm beginning to wonder if it wouldn't make more sense to accept and pass on **kwargs. | 10:10 |
| jtv | (Had somebody done that before, you wouldn't have needed to touch it now) | 10:10 |
| bac | jtv: actually i had to do it in another branch too that is stalled... | 10:11 |
| jtv | bac: so what say you? | 10:11 |
| bac | jtv: sure, i'll do that | 10:12 |
| jtv | Excellent. | 10:12 |
| bac | jtv: if you have any insight into my other problem i'd like to chat about it. it is resolved with a fine work-around but i'm bugged that getting and rendering a view doesn't work | 10:13 |
| jtv | bac: I haven't gotten to that bit yet, but I've been revisiting my modest recollections of how others did this. | 10:14 |
| jtv | Something else again: in project.py, has_translatables: | 10:14 |
| jtv | don't use "query_result.count() > 0" to establish existence. Use "not query_result.is_empty()". | 10:15 |
| bac | jtv: yes, of course | 10:15 |
| jtv | (Storm's implementation of that is still not optimal, but at least this way you don't stop it from becoming optimal) | 10:15 |
| jtv | bac: has the requirement of dromedaryCase in method names finally been abolished? | 10:17 |
| bac | jtv: no, i just probably munged something. where? | 10:17 |
| jtv | The helper methods in TestRobotsMixin. | 10:18 |
| jtv | And the rest of the test. | 10:20 |
| bac | jtv: yeah, i was just on crack | 10:24 |
| jtv | oh that's alright then (!) | 10:24 |
| jtv | bac: just pulled your branch so I can play along | 10:30 |
| bac | jtv: wrt has_translatables on the view, i just saw there is the same method on the context so i can just delete it and the view tests. yay. | 10:33 |
| jtv | bac: but you deliberately replaced references to the context object in the TAL with references to this new view property, and I thought that was actually a good thing. | 10:34 |
| bac | jtv: no i didn't | 10:35 |
| bac | i replaced context/translatables, which is the resultset | 10:35 |
| bac | i should've used context/has_translatable | 10:35 |
| jtv | ahhh | 10:35 |
| bac | jtv: of course the has_translatable on the model needs to be changed to use is_empty, so i don't escape completely! | 10:36 |
| jtv | :) | 10:36 |
| jtv | bac: what surprises me is that if I replace the body of get_rendered_contents with just "return self.get_view()()" I only see one test fail. | 10:36 |
| bac | yes | 10:37 |
| bac | jtv:and that is for distroseries, yes? | 10:37 |
| bac | and it fails on 'templates', no? | 10:37 |
| jtv | bac: no, it's TestRobotsProductSeries.test_LAUNCHPAD_does_not_block_robots | 10:37 |
| jtv | KeyError: 'imports' | 10:38 |
| bac | jtv: you would see another failure except i inadvertantly included an extra get_rendered_contents in the test for distroseries | 10:40 |
| bac | jtv: delete it and you'll see another failure | 10:40 |
| bac | jtv: but i'm glad you just pointed out the redundancy | 10:40 |
| jtv | well, "stumbled into it without even realizing it afterwards" would be more appropriate | 10:41 |
| jtv | So now test_LAUNCHPAD_does_not_block_robots fails for both ProductSeries and DistroSeries, though not for ProjectGroup. | 10:42 |
| jtv | (Is there one for ProjectGroup?) | 10:42 |
| bac | jtv: a failure? no. likely the page template doesn't use a menu for PG | 10:43 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: StevenK || queue: [StevenK*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | bac: I wonder if it's this line… | 10:46 |
| jtv | lib/lp/translations/templates/person-navlinks.pt: <a tal:replace="structure view/menu:navigation/imports/render"/> | 10:46 |
| bac | yes. yes that is it | 10:46 |
| bac | for distroseries it is the one for 'templates' | 10:46 |
| jtv | bac: that makes me wonder… why is that menu even being referenced for an anonymous access? —this is anonymous access, isn't it? | 10:49 |
| bac | yes, but i don't think it is relevant | 10:49 |
| bac | in the web UI those links are visible to the anon user | 10:50 |
| bac | again, i think the problem is that the view is registered in the zcml on a layer but not the facet | 10:50 |
| bac | and it could be the redirection view stuff y'all do | 10:50 |
| jtv | ah | 10:51 |
| bac | jtv: hey, would you mind looking at sqlobject.py's is_empty()? | 10:55 |
| bac | i mean in storm | 10:55 |
| jtv | bac: not at all, hang on | 10:55 |
| jtv | bac: that looks a bit like a bug doesn't it | 10:57 |
| bac | yeah, it it really messes up my tests | 10:57 |
| jtv | def is_empty(self): result_set = foo() ; return not result_set.is_empty() | 10:57 |
| jtv | bugger with bells on | 10:57 |
| jtv | that's pretty serious | 10:58 |
| bac | so, are we not using it anywhere? i guess all the call sites using is_empty are pure storm | 10:58 |
| jtv | I guess. We've been using workarounds in Storm, and cast-to-bool in SQLObject. | 10:58 |
| * jtv strides off towards #storm | 10:59 | |
| jtv | bac: I'd file a storm bug for this if I were you. | 11:01 |
| bac | i would if i could get to LP.net | 11:01 |
| jtv | !? | 11:01 |
| jtv | same problem as spiv? | 11:02 |
| bac | dunno. is spiv having a problem? | 11:02 |
| bac | it worked moments ago so i expect it is transient | 11:02 |
| jtv | He was having problems with SSL connections. | 11:02 |
| bac | hmm, dns lookup error | 11:02 |
| bac | and we're back | 11:03 |
| jtv | …except I'm not getting a whois record for code.launchpad.net | 11:04 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| allenap | gmb: Fancy another one on the stack? Also, can you do UI reviews? | 11:19 |
| bac | jtv:bug 659078 | 11:21 |
| _mup_ | Bug #659078: sqlobject is_empty and __nonzero__ are incorrect <Storm:New> <https://launchpad.net/bugs/659078> | 11:21 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | allenap: in order: Yes and No | 11:21 |
| === allenap changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: allenap || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | allenap: Er... | 11:21 |
| bac | jtv: in light of this bug, i'll put an XXX in the spot i was going to use is_empty until it is resolved. | 11:22 |
| allenap | gmb: Oops | 11:22 |
| === allenap changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | :) | 11:22 |
| gmb | allenap: I'm assuming its the one on +activereviews already | 11:22 |
| gmb | bug 656823 | 11:22 |
| allenap | gmb: Yip. | 11:22 |
| _mup_ | Bug #656823: Subscribing to a search lacks a UI <story-subscribe-to-search> <Launchpad Bugs:In Progress by allenap> <https://launchpad.net/bugs/656823> | 11:22 |
| gmb | Cool | 11:22 |
| jtv | jtv: yes, makes sense | 11:22 |
| bac | jtv: in create_initialized_view, some of the named parameters are actually used in the method (form and method) so they can't really be grouped in with kwargs. unfortunately they are in the middle of the argument list. | 11:34 |
| jtv | bac: would it be too nasty to read them as kwargs.get(parameter, default)? | 11:36 |
| bac | jtv: also create_view already takes a kwargs | 11:36 |
| bac | jtv: my inquisitiveness into how that would work is trumped by my need to go get dinner. let's talk about it tomorrow. | 11:38 |
| jtv | bac: ok | 11:38 |
| jtv | I'll just add my comments to the MP. | 11:39 |
| bac | thanks jtv | 11:43 |
| gmb | allenap: Code looks good to me. | 11:43 |
| gmb | It'll be interesting to see if and how we eventually integrate that with +subscriptions... | 11:43 |
| === 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 | ||
| allenap | gmb: Cheers. Is there a plan somewhere for what +subscriptions will evolve into? | 11:44 |
| StevenK | gmb: I've commented on https://code.edge.launchpad.net/~stevenk/launchpad/db-add-derivedistroseries-api/+merge/38189 ; I'm happy with everything else you pointed out. | 11:44 |
| gmb | allenap: There's a mockup. Not sure that constitutes a plan. | 11:49 |
| gmb | StevenK: Looking now, thanks. | 11:49 |
| gmb | StevenK: Okay, I've replied. I suggest that you just expand the loop to a series of `if not <param>: raise DerivationError("Useful message")` statements. | 11:53 |
| * gmb -> lunch and errands | 12:31 | |
| === 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 | ||
| === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jcsackett changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: lunch || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | MP for my review is here: https://code.edge.launchpad.net/~jcsackett/launchpad/code-configuration-652142/+merge/38015 | 14:07 |
| jcsackett | no huge rush, just making sure it's in the queue this morning. :-) | 14:07 |
| leonardr | jcsackett, i'll take a look | 14:07 |
| jcsackett | leonardr, thanks. | 14:07 |
| === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: lunch, jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | jcsackett, r=me | 14:16 |
| === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: lunch, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | leonardr: thanks. | 14:16 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | Ooops. | 15:53 |
| === deryck is now known as deryck[lunch] | ||
| === gmb 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 | ||
| gary_poster | leonardr: care to eyeball this to make sure I haven't done something stupid somehow in what should be the dumbest commit ever? https://code.edge.launchpad.net/~gary/launchpad/production-devel-test/+merge/38239 | 16:46 |
| leonardr | sure | 16:46 |
| gary_poster | thanks | 16:46 |
| leonardr | gary, r=me | 16:50 |
| gary_poster | cool thanks | 16:50 |
| gary_poster | leonardr: could you actually approve it in the MP? That will make it easier to land | 16:54 |
| leonardr | gary: sorry! | 16:54 |
| gary_poster | np | 16:54 |
| leonardr | done | 16:55 |
| === deryck[lunch] is now known as deryck | ||
| gary_poster | thanks | 17:05 |
| gary_poster | leonardr: two more, with low priority because I won't land them till after the release. | 17:08 |
| gary_poster | (1) https://code.edge.launchpad.net/~gary/launchpad/bug656213/+merge/38242 stub has already approved this one outside of the MP. | 17:08 |
| gary_poster | (2) https://code.edge.launchpad.net/~gary/launchpad/bug650343-2/+merge/38224 is big but stupid--a few moved files and making lint happy, in response to a review of an earlier branch. | 17:08 |
| leonardr | ok, taking a look | 17:08 |
| gary_poster | thanks | 17:08 |
| === benji is now known as benji-lunch | ||
| leonardr | gary, line 284-ish of your second branch | 17:42 |
| leonardr | you erase the distinction between doctests with logging and doctests without. that's your intention? | 17:43 |
| gary_poster | leonardr: yes, because we don't have anymore in that class | 17:43 |
| leonardr | ok | 17:43 |
| leonardr | gary: line 562: "no signature is treated" is ambiguous, how about "a message with no signature" | 17:44 |
| leonardr | line 675, "message message" | 17:45 |
| leonardr | er, 576 | 17:45 |
| gary_poster | leonardr agree, good catch, for both. | 17:45 |
| leonardr | gary, can you explain your change to the security policy around line 611? are you running the tests in a different layer to save time? | 17:47 |
| leonardr | (ie. the LaunchpadZopelessLayer) | 17:49 |
| gary_poster | leonardr: I'm duplicating the way this test was run before, when it was a part of that "doctests_with_logging" code. It seemed that this was reasonable before, and I didn't think about it much beyond that TBH. | 17:50 |
| gary_poster | Using the real Launchpad security policy rather than the permissive one that the zopeless layer usually uses is a Good Thing generally, but that doesn't really explain what is going on here. | 17:50 |
| leonardr | gary: maybe you copied that code from test_system_documentation.py? | 17:52 |
| gary_poster | that sounds right; verifying | 17:53 |
| gary_poster | leonardr: yes, see testSetUp and testTearDown in ProcessMailLayer in est_system_documentation.py | 17:54 |
| gary_poster | test_system_documentation.py | 17:54 |
| leonardr | gary: is it worth refactoring that code? | 17:57 |
| gary_poster | leonardr: you mean to not dupe between canonical/launchpad and lp/services? I briefly considered it. I rejected it because (1) the canonical/launchpad code is supposed to be all moved eventually, I assume, and I don't know how things will resolve then; and (2) it was so little code that it didn't seem worth it to me. If you disagree, happy to put it in. | 18:00 |
| gary_poster | If you want me to put it in and have any ideas other than two teeny tiny functions for set up and tear down that are used by the two respective bits of code, lemme know. | 18:00 |
| leonardr | your #1 reason convinces me | 18:01 |
| leonardr | second branch approved | 18:02 |
| === benji-lunch is now known as benji | ||
| gary_poster | thank you | 18:04 |
| leonardr | trying to wrap my head around the other one | 18:04 |
| leonardr | gary: ok, the thing i don't understand about the other branch is why you don't set LPCONFIG when the instance name is 'development' | 18:13 |
| gary_poster | leonardr: because it is already considered the default in lincanonical/config/__init__.py ``DEFAULT_CONFIG = 'development'``) | 18:14 |
| leonardr | gary: ok, so avoiding that case is how you avoid setting LPCONFIG all the time | 18:16 |
| gary_poster | well--I'd say, leveraging that behavior is how I avoid setting LPCONFIG all the time, but maybe that's the same thing | 18:16 |
| leonardr | gary: ok, r=me on that one | 18:20 |
| gary_poster | cool, thanks again leonardr | 18:20 |
| === gary_poster is now known as gary-lunch | ||
| === fjlacoste is now known as flacoste | ||
| === gary-lunch is now known as gary_poster | ||
| bdmurray | leonardr: would you mind reviewing https://code.edge.launchpad.net/~brian-murray/launchpad/person-subscription-story/+merge/37920 | 20:21 |
| leonardr | bdmurray, sure | 20:23 |
| === Ursinha is now known as Ursinha-afk | ||
| leonardr | bdmurray, you talk about 'webster' before that user exists. you should mention that you're creating that user | 20:31 |
| leonardr | can you explain where the 243656 comes from on line 69? | 20:31 |
| leonardr | you're also mentioning bug 16 in the text, though it looks like you removed it from the code | 20:32 |
| _mup_ | Bug #16: "Swedish" and "Swedish (Sweden)" should be the same language <Launchpad Translations:Fix Released by daf> <https://launchpad.net/bugs/16> | 20:32 |
| bdmurray | leonardr: okay re: creating webster. 243656 is the subscriber id. and got it re # 16. | 20:38 |
| leonardr | bdmurray, that subscriber id seems like the kind of thing we're trying to remove from tests? | 20:42 |
| bdmurray | leonardr: yeah, I'm working on that now | 20:43 |
| leonardr | ok | 20:43 |
| leonardr | bdmurray: review sent | 20:47 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!