| === lifeless changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-607935/+merge/34143] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | can I tempt anyone with a review? | 00:53 |
|---|---|---|
| lifeless | mwhudson: I know you have other stuff to do, but I'm hoping you'll be generous :) - its another small one, with -much less- hair than the other. | 00:54 |
| mwhudson | lifeless: sure, i can take a look | 00:55 |
| lifeless | mwhudson: thanks; its in topic | 00:58 |
| mwhudson | oh yeah, that thing | 00:59 |
| lifeless | mwhudson: so, what did you think? | 01:10 |
| mwhudson | lifeless: sorry, got a bit distracted, have meld up now :) | 01:11 |
| mwhudson | can you really not do remove_tracer(self) ? | 01:14 |
| * mwhudson has no idea | 01:14 | |
| lifeless | dunno, was just moving it so documented the limitation | 01:14 |
| mwhudson | ah ok | 01:14 |
| lifeless | it already existed if you passed in a function that itself did record_statements | 01:14 |
| mwhudson | ah ok | 01:15 |
| mwhudson | again :) | 01:15 |
| lifeless | mwhudson: did you alter https://launchpad.net/ubuntu/+assignments in your recent refactorings ? | 01:17 |
| mwhudson | oh man, lp.testing._webservice.QueryCollector? | 01:18 |
| mwhudson | what's that doing there | 01:18 |
| lifeless | mwhudson: existing ? | 01:18 |
| mwhudson | i just hate the way we accumulate multiple copies of things that do nearly the same thing | 01:18 |
| lifeless | right | 01:18 |
| lifeless | so QC was *in a doctest* a month ago | 01:18 |
| mwhudson | haha | 01:19 |
| lifeless | its different to record_statements though | 01:19 |
| mwhudson | so things are moving in the right direction | 01:19 |
| mwhudson | yeah, but in ways that shouldn't *really* matter, right? | 01:19 |
| lifeless | oh they should | 01:19 |
| lifeless | very much | 01:19 |
| mwhudson | not expecting you to fix it in this branch, just complaining out loud | 01:19 |
| lifeless | I think it would be a huge mistake to treat them as interchangable | 01:19 |
| mwhudson | now, more apropos: "Find the persons or teams by which person is subscribed." <- i don't understand this | 01:20 |
| lifeless | ok | 01:20 |
| lifeless | so a person can be subscribed to a bug directly or indirectly | 01:21 |
| lifeless | the bugtask browser code (for me specifically) generates 300 queries whenever I look at a bugtask:+index page. | 01:21 |
| lifeless | because it was answering the question 'what ways is robert subcribed' by python | 01:22 |
| lifeless | and its not interested in the BugSubscriptions | 01:22 |
| lifeless | its interested in the Person or Team records that are subscribed. | 01:22 |
| lifeless | I find making the one-line summary hard sometimes. | 01:23 |
| mwhudson | yeah, it can be difficult | 01:23 |
| lifeless | anyhow, this thing returns a result set | 01:24 |
| lifeless | which outputs IPerson | 01:24 |
| lifeless | such that it outputs all IPersons which cause person to be subscribed to the bug. | 01:24 |
| mwhudson | lifeless: that version makes more sense to me, perhaps | 01:25 |
| mwhudson | but it's hard to say if that's only because i read it after i already understood what it did | 01:25 |
| lifeless | 'return all IPersons which cause person to be subscribe to self' ? | 01:26 |
| mwhudson | lifeless: s/self/this bug/ would be more consistent with other docstrings, but otherwise +1 | 01:27 |
| lifeless | 78 chars. | 01:28 |
| lifeless | 79 with . | 01:28 |
| lifeless | :P | 01:28 |
| mwhudson | hahaha | 01:28 |
| * lifeless pissess of the narrow-screen crowd. | 01:28 | |
| mwhudson | ok, stick with your version then | 01:28 |
| lifeless | mwhudson: I think we should use the full 80 columns, but thats an entirely different discussion | 01:29 |
| mwhudson | yes | 01:29 |
| thumper | lifeless: want to review my tag cloud change? | 01:30 |
| mwhudson | lifeless: TestBugTaskView.initialize_view, did you consider using create_initialized_view ? | 01:30 |
| thumper | lifeless: it'll get rid of the timeouts | 01:30 |
| mwhudson | lifeless: it's in lp.testing.view iirc | 01:30 |
| mwhudson | lp.testing.views | 01:31 |
| mwhudson | lifeless: gosh the browser code you're changing is horrid | 01:33 |
| lifeless | thumper: sure | 01:34 |
| lifeless | mwhudson: I wanted to be crystal clear about what I'm exercising | 01:34 |
| lifeless | mwhudson: can switch it if you prefer | 01:34 |
| mwhudson | lifeless: hm, that's a fair point too | 01:35 |
| thumper | lifeless: https://code.edge.launchpad.net/~thumper/launchpad/different-cloud/+merge/34146 | 01:37 |
| lifeless | I'm just doing a task for lynne, brb | 01:37 |
| mwhudson | lifeless: factory.makeTeam takes a members=[] argument | 01:39 |
| mwhudson | would reduce some hair in tests | 01:39 |
| lifeless | mwhudson: thanks, I'll definitely do that in future, can tweak here if you like. | 01:44 |
| mwhudson | lifeless: i think it would make the tests significantly easier to follow | 01:44 |
| lifeless | will do | 01:44 |
| mwhudson | ta | 01:44 |
| mwhudson | lifeless: in the browser doctest, "# count with 2 teams" should say 4 shouldn't it? | 01:46 |
| lifeless | oh bugger, yes. | 01:46 |
| lifeless | s/2/many/ | 01:47 |
| mwhudson | :) | 01:47 |
| mwhudson | future proofing! | 01:47 |
| mwhudson | lifeless: in the same test class, how do you feel about s/initialize_view/record_view_initialization/ ? | 01:48 |
| lifeless | sure | 01:49 |
| mwhudson | otherwise i think i'm fine :) | 01:49 |
| lifeless | changes are done | 01:49 |
| mwhudson | can you push the branch again and i'll mark it so in the web | 01:49 |
| mwhudson | ah ok | 01:49 |
| lifeless | pushing in a second | 01:50 |
| * lifeless ignores the 79 limit issue | 01:53 | |
| lifeless | ok push running now | 01:54 |
| lifeless | mwhudson: you can clicky click now | 01:56 |
| lifeless | mwhudson: (though I don't know why you wait) | 01:56 |
| mwhudson | lifeless: it's been a somewhat distracted review from my end (my fault), i want to look over the diff again, may as well look over the one with the changes | 01:57 |
| lifeless | mwhudson: well its there | 01:58 |
| lifeless | thumper: nuke this: | 01:59 |
| lifeless | 653+ | 01:59 |
| lifeless | 654+def test_suite(): | 01:59 |
| lifeless | 655+ return unittest.TestLoader().loadTestsFromName(__name__) | 01:59 |
| mwhudson | lifeless: click clicked | 01:59 |
| mwhudson | +y | 01:59 |
| lifeless | thanks | 02:02 |
| thumper | lifeless: OK, should also remove it from the standard_test_template file then | 02:06 |
| thumper | because that is where it came from | 02:06 |
| lifeless | please do | 02:07 |
| lifeless | if we had gettext | 02:07 |
| lifeless | the pluralisation would be easier for you | 02:07 |
| lifeless | thumper: I know you didn't touch this | 02:08 |
| thumper | lifeless: tweaked and pushed | 02:08 |
| lifeless | but | 02:08 |
| lifeless | sorted( | 02:08 |
| lifeless | 76list(getUtility(IBranchCloud).getProductsWithInfo(num_products))) | 02:08 |
| lifeless | == | 02:08 |
| lifeless | sorted(getUtility(IBranchCloud).getProductsWithInfo(num_products)) | 02:08 |
| lifeless | (sorted takes an iterable) | 02:08 |
| thumper | yep | 02:08 |
| lifeless | is there some reason to have a list( there ? | 02:09 |
| thumper | nope | 02:09 |
| thumper | removed | 02:09 |
| lifeless | the slave store by default. | 02:10 |
| lifeless | is a lie | 02:10 |
| lifeless | 'the slave store.' | 02:10 |
| thumper | ok | 02:10 |
| lifeless | would be true | 02:10 |
| thumper | it used to be defaulted :) | 02:11 |
| lifeless | thats ok | 02:11 |
| lifeless | have you tried the resulting queries on staging ? | 02:11 |
| thumper | tweaked | 02:11 |
| thumper | yes | 02:11 |
| thumper | I have | 02:12 |
| thumper | and it's fast | 02:12 |
| lifeless | 269+ | 02:12 |
| lifeless | that new VWS should be removed; the auto formatter can do it for you I think | 02:12 |
| thumper | what is VWS? | 02:13 |
| lifeless | vertical white space | 02:13 |
| thumper | 269+ what? | 02:13 |
| thumper | I see it | 02:13 |
| thumper | are we running the autoformatter? | 02:14 |
| lifeless | not automatically | 02:14 |
| thumper | I thought we weren't | 02:14 |
| lifeless | but you can run it by hand AFAIK | 02:14 |
| thumper | but it makes my nice import lines messy | 02:14 |
| lifeless | its more stubborn than you | 02:15 |
| thumper | my making something like "from storm.locals import And, Or" take 4 lines | 02:15 |
| thumper | which I've been told is OK | 02:15 |
| lifeless | See, I think that should be fine on one line. | 02:15 |
| thumper | me too | 02:15 |
| thumper | so... | 02:15 |
| thumper | fix the autoformatter | 02:15 |
| lifeless | and the policy | 02:15 |
| thumper | agreed | 02:16 |
| thumper | so I can leave the VWS? | 02:16 |
| lifeless | oh the VWS is wrong regardless :) | 02:16 |
| thumper | why? | 02:16 |
| thumper | are we not separating canonical and lp? | 02:16 |
| lifeless | no, because they are both 'local' | 02:16 |
| lifeless | pep8 defines three classes of imports | 02:16 |
| lifeless | stdlib | 02:16 |
| lifeless | nonlocal | 02:16 |
| lifeless | local | 02:16 |
| thumper | ok | 02:16 |
| lifeless | in that order | 02:16 |
| thumper | fixed | 02:17 |
| lifeless | so an external canonical.landscape import, say, would be nonlocal. | 02:17 |
| lifeless | I doubt the autoformatter is sophisticated enough for that yet, but that would be the right thing to do | 02:17 |
| thumper | I've moved on | 02:18 |
| lifeless | reviewed +1 | 02:19 |
| lifeless | clicky clicky set your commit message etc | 02:19 |
| thumper | ta | 02:19 |
| lifeless | how many ms do you think it will take to render now ? | 02:24 |
| thumper | no freaking idea | 02:39 |
| thumper | although wallyworld_ says 42 | 02:39 |
| lifeless | lies | 02:49 |
| lifeless | Would be nice | 02:49 |
| lifeless | but lies. | 02:49 |
| === lifeless changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618367/+merge/34162] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | if anyone wants a break from coding and would like to review whats in the topic, that would be grand. | 07:11 |
| noodles775 | Can someone rs the following testfix, otherwise I'll just rs it myself :) https://code.edge.launchpad.net/~michael.nelson/launchpad/test-fix-db-buildbase/+merge/34166 | 08:04 |
| lifeless | done | 08:23 |
| noodles775 | Thanks lifeless it's on pqm already. | 08:23 |
| lifeless | speaking of buffer 65 | 08:30 |
| lifeless | do you have time to do a review perhaps ? | 08:30 |
| noodles775 | lifeless: I can, although gmb might be starting his OCR round now too? | 08:35 |
| lifeless | gmb: are you? | 08:35 |
| noodles775 | lifeless: looking now. | 08:46 |
| lifeless | thanks! | 08:48 |
| noodles775 | lifeless: have you seen the pre-iterator-hook option for DecoratedResultSet? It might allow you to prefetch the data for all results at once, rather than when each result item is accessed? | 09:16 |
| lifeless | noodles775: yes, it's not sufficient | 09:17 |
| noodles775 | OK | 09:17 |
| lifeless | because you need to discard the uninteresting columns so that you return Specification | 09:17 |
| lifeless | not (Specification, .., ..,..) | 09:17 |
| lifeless | in all other regards it would be equal | 09:17 |
| lifeless | (this patch prefetches all at once; its just patches them up row by row) | 09:18 |
| noodles775 | Yes, you would still do that with your decorator function, the pre-iter-hook just allows you to do your caching all at once. | 09:18 |
| noodles775 | Oh, does it? | 09:18 |
| * noodles775 looks again. | 09:18 | |
| lifeless | it does one query | 09:18 |
| lifeless | on Specification, Person(aliased assignee), Person(aliased approver), Person(aliased drafter), EmailAddress(aliased for assignee), Account(aliased for assignee), EmailAddress(aliased for approver ... | 09:19 |
| lifeless | the decorator does two things: | 09:19 |
| lifeless | - pulls out the Specification, so that it looks like we only queried Specifications to clients of the api | 09:20 |
| lifeless | - preloads the Person objects for assignee, approver and drafter for that row - no new queries are issued. | 09:20 |
| lifeless | [I has the tests to prove it] | 09:20 |
| noodles775 | Yes, I don't doubt that that is what its doing, what I do doubt is that it's doing the preloading once for all people in your result set, rather than for each person individually. | 09:22 |
| lifeless | noodles775: performance wise that doesn't matter though | 09:23 |
| noodles775 | but that may not be possible here. | 09:23 |
| noodles775 | Ah, ok. | 09:23 |
| lifeless | noodles775: I don't see why it would be of advantage (or disadvantage) | 09:23 |
| lifeless | because: | 09:24 |
| lifeless | - we have to have a row decorator to return just Specification | 09:24 |
| noodles775 | The advantage of the pre-iter-hook is that you can do one single query to pre-load all of your result set (or slice of your result set), rather than one query to pre-load each item in your result set. But as you say, it might not apply here. | 09:24 |
| lifeless | - doing it in pre_iter_hook would then save no function calls, but add one function call to use the pre_iter_hook | 09:24 |
| noodles775 | Yes, its about saving queries, not function calls. | 09:25 |
| lifeless | noodles775: if you're preloading via separate queries, I suspect you're doing it wrong :) | 09:25 |
| lifeless | noodles775: though I can imagine a use case when the query optimiser goes beserk | 09:25 |
| noodles775 | lifeless: Sorry, s/preloading/caching | 09:25 |
| noodles775 | ie. currently cache_validity is called when each item in your result set is accessed, which may be the best way, I just wasn't sure if you'd considered whether it was possible to do this once for the result set instead. | 09:26 |
| lifeless | sure, yes. | 09:27 |
| lifeless | note that cache_validity does no queries | 09:27 |
| noodles775 | Ah, right. I see... because its ownly using the preloaded data. Gotit. | 09:27 |
| lifeless | noodles775: thanks for doing this | 09:40 |
| noodles775 | no probs, almost done. Looks great, so far the only question I've got is whether you think the call-sites might be easier to read if _validity_queries returned a dict instead of a tuple? | 09:41 |
| lifeless | hmm, happy to change that. | 09:42 |
| lifeless | namedtuple would be ideal | 09:42 |
| lifeless | is that in 2.5 ? | 09:42 |
| noodles775 | Not sure, but yeah, that would be ideal. | 09:42 |
| lifeless | I'll do a dict or namedtuple | 09:45 |
| noodles775 | Great, r=me (I had one or two other questions). Thanks lifeless. | 09:49 |
| lifeless | cool | 09:54 |
| lifeless | _ is a convention in python for 'ignored' | 09:54 |
| lifeless | or unused | 09:54 |
| lifeless | namedtuple is 2.6 | 09:56 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618367/+merge/34162] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | lifeless, Is the branch in the queue the one that noodles775 just reviewed? | 09:56 |
| noodles775 | Yeah, among other things (translatable strings, last result on console etc.). | 09:56 |
| noodles775 | gmb: yep. | 09:57 |
| gmb | Righty. | 09:57 |
| lifeless | noodles775: I like having it - its much more pithy than ignored_because_we_have_to_have_a_name_expr_here in range(...): | 09:58 |
| lifeless | noodles775: Its the only single codepoint variable name I use ;) | 09:59 |
| noodles775 | lifeless: fine by me, might be worth adding to the style guide. | 09:59 |
| lifeless | noodles775: http://pastebin.com/Q2dAz2ts | 10:02 |
| lifeless | noodles775: is my proposed diff | 10:02 |
| lifeless | please let me know if you think it is an improvement | 10:02 |
| lifeless | lastly, _webservice for QueryCollector - its not exported from testing/__init__.py; it seems a bit inconsistent to me to reexport there and not elsewhere; we probably want to keep iterating and shuffling that stuff around anyhow... | 10:03 |
| noodles775 | Yep. | 10:03 |
| noodles775 | lifeless: and yes, I think the dict references are a bit more obvious than tuple indexes. Thanks! | 10:05 |
| lifeless | my pleasure | 10:05 |
| lifeless | thanks heaps for the review | 10:05 |
| noodles775 | np. | 10:05 |
| lifeless | I shall push that change and send to ec2 | 10:05 |
| noodles775 | lifeless: erm, don't you need to change the callsites too? | 10:05 |
| noodles775 | ah, that is the only one? | 10:06 |
| lifeless | noodles775: two | 10:06 |
| lifeless | one in specification, one in person | 10:06 |
| noodles775 | yep, never mind me. | 10:06 |
| * noodles775 goes for a cuppa. | 10:06 | |
| wgrant | gmb: I have a massive (~30 lines) branch for you, if you've time. | 10:29 |
| gmb | wgrant, Unless you're out by two orders of magnitude, in your estimation, I think I can handle that :) | 10:31 |
| lifeless | +1 | 10:31 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| wgrant | gmb: https://code.edge.launchpad.net/~wgrant/launchpad/bug-510180/+merge/34175 | 10:33 |
| gmb | wgrant, Nice and subtle. I like it. r=me | 10:33 |
| wgrant | gmb: Heh. Thanks. Can you throw it into ec2? | 10:33 |
| gmb | wgrant, Will do. | 10:34 |
| wgrant | Thanks. | 10:34 |
| === 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_ is now known as henninge | ||
| === matsubara-afk is now known as matsubara | ||
| === benji___ is now known as benji | ||
| bac | hi gmb can i get a review? https://code.edge.launchpad.net/~bac/launchpad/bug-39212/+merge/34196 | 14:24 |
| gmb | bac, Sure. | 14:24 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | thanks | 14:25 |
| === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | bac, What's with the wacky indenting around line 93-96 of the diff? | 14:31 |
| bac | gmb: dunno. let me look | 14:31 |
| bac | gmb: dewhacked | 14:33 |
| gmb | bac, Eyefankyoo | 14:33 |
| gmb | bac, ALso, line 109 needs to be indented by one more stop to be consistent. | 14:34 |
| bac | gmb: i am going to open a but about URIWidget not respecting displayWidth, too. | 14:34 |
| bac | gmb, yep, i caught that one too | 14:34 |
| gmb | bac, Thanks x2 | 14:34 |
| bac | i think we have css that is harmful and should be removed, but i'm not certain of all of the areas affected. | 14:35 |
| gmb | Fair neough. | 14:35 |
| * gmb switches lcoations; brb | 14:35 | |
| gmb | bac, I thought that ReST headers just had to have the underline rather than the overline, too. | 14:41 |
| gmb | e.g. | 14:41 |
| gmb | Person's wikinames | 14:41 |
| gmb | =============== | 14:41 |
| gmb | (etc.) | 14:41 |
| bac | gmb: really? i've often seen over/under | 14:41 |
| gmb | bac, In that case, we have a problem with consistency | 14:41 |
| bac | gmb: if you know better that's one less line... | 14:41 |
| bac | gmb: duh | 14:42 |
| gmb | (Not least because I do underline only :)) | 14:42 |
| * gmb checks TestsStyleGuide | 14:42 | |
| gmb | bac, The example in TestsStyleGuide uses underline only, so let's standardise on that. | 14:43 |
| bac | gmb: will do | 14:43 |
| gmb | Thanks. | 14:43 |
| gmb | bac, r=me with the above changes. | 14:51 |
| === 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 | ||
| === Ursinha is now known as Ursinha-afk | ||
| EdwinGrubbs | gmb: can you review lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2 | 15:39 |
| EdwinGrubbs | oops, I meant to paste https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout-part2/+merge/34159 | 15:40 |
| gmb | EdwinGrubbs, Sure | 15:40 |
| === salgado is now known as salgado-lunch | ||
| gmb | EdwinGrubbs, Those two tests are *awfully* similar. Any chance they could be unified in their own TestCase (e.g. TestFooBatchNavigators) | 15:47 |
| EdwinGrubbs | gmb: sure | 15:48 |
| gmb | EdwinGrubbs, Cool. | 15:48 |
| gmb | EdwinGrubbs, Other than that, r=me. | 15:48 |
| === gmb 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 | ||
| === 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 | ||
| EdwinGrubbs | thanks | 16:00 |
| === deryck is now known as deryck[lunch] | ||
| noodles775 | Hi abentley ! Will you have a chance view the changes I did in response to your review yesterday, or should I ask Leonard to take a look? | 16:19 |
| noodles775 | https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-model/+merge/34086 | 16:20 |
| abentley | noodles775, I can have a look. | 16:20 |
| noodles775 | Great, thanks. | 16:20 |
| abentley | noodles775, re: getComments, I don't understand how implementing it directly on DistroSeriesDifference would be programming to interfaces, not implementations. | 16:40 |
| noodles775 | abentley: It wouldn't. It would be programming to implementations, rather than programming to interfaces wouldn't it? | 16:41 |
| noodles775 | ie. it would be using the comment model object etc., where as at the moment it only deals with interfaces. But I don't mind changing it. | 16:42 |
| abentley | noodles775, sorry, I still don't understand why moving it there would be the wrong thing. | 16:42 |
| noodles775 | abentley: It wouldn't be wrong, I just thought it was *better* to separate out so that the DSDifference model doesn't need to know anything about the comment model. | 16:43 |
| abentley | You just wind up calling a method on DistroSeriesDifferenceCommentSource that makes very implementation-specific assumption about DistrosSeriesDifference | 16:43 |
| abentley | The implementation of getComments *must* deal with the implementation of how those comments are stored. The best encapsulation is to make it an instance method on DistroSeriesDifference. | 16:45 |
| noodles775 | Calling a method on a utility that implements an interface, yes. And the comment model needs to know about the implementation of DSDifference, as its implementation references it. But as I said, I don't feel strongly about it. | 16:45 |
| abentley | noodles775, my own rule is to be suspicious of staticmethods, because they are often cleaner as instance methods. | 16:48 |
| abentley | noodles775, I don't think your rule applies to objects that implement an interface. | 16:48 |
| noodles775 | oh, I only used a static method because that's what most of the classes in lp.code do. | 16:48 |
| noodles775 | See lp.code.models.sourcepackagerecipe.SourcePackageRecipe.new, for example. | 16:49 |
| abentley | noodles775, I'm glad you did. Lots of people go around making "instance" methods that are really staticmethods, and I'm even more suspicious of those. | 16:49 |
| noodles775 | OK, so you'd like me to remove DSDifferenceComment.getForDifference() and put that functionality directly in the existing DSDifference.getComments() right? | 16:51 |
| abentley | noodles775, right. | 16:51 |
| abentley | noodles775, An example of an "instance" method that is really a staticmethod is BranchSet.getRecentlyChangedBranches() | 16:52 |
| noodles775 | abentley: pushing http://pastebin.ubuntu.com/486363/ | 16:55 |
| noodles775 | which passes with: bin/test -vvm test_distroseriesdifference -m test_distroseriesdifferencecomment | 16:56 |
| abentley | noodles775, r=me. Thanks for your changes. | 16:57 |
| noodles775 | np, thanks for the review :) | 16:57 |
| === deryck[lunch] is now known as deryck | ||
| === benji is now known as benji-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === benji-lunch is now known as benji | ||
| === matsubara-lunch is now known as matsubara | ||
| bac | hi leonardr ... has the API infrastructure changed with respect to batching lately? We're seeing OOPSes on getting all of the members of a very large team. | 18:57 |
| leonardr | bac, yes, see bug 627442, caused by bug 620508, recently triggered and incompletely worked around | 18:58 |
| _mup_ | Bug #627442: lazr.restful and batching cause a FeatureError <oops> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/627442> | 18:58 |
| _mup_ | Bug #620508: Slicing a ResultSet breaks subsequent len() calls. <Storm:New> <https://launchpad.net/bugs/620508> | 18:58 |
| leonardr | gary and benji are working on it | 18:58 |
| bac | leonardr: yep, that's the one! thanks. | 18:59 |
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| === jcsackett changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | leonardr: https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-official_rosetta/+merge/34229 | 19:20 |
| jcsackett | linting on this one is a bit odd, b/c it hit about a zillion files and the review would be unreadable with the fixes; i'll hit everything from make lint, but didn't push it up for now, so the MP and diff is legible. | 19:21 |
| leonardr | jcsackett, ok | 19:40 |
| leonardr | salgado, can i ask you to take a look at https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2 ? | 19:42 |
| leonardr | just the last two revisions | 19:42 |
| leonardr | abently isn't around, but it's all oauth stuff so you should be able to do it | 19:43 |
| salgado | leonardr, r11454 and r11455? | 19:43 |
| leonardr | salgado: actually, it looks like it's everything from 11452..11455 | 19:47 |
| salgado | leonardr, it'd be nice if you could generate an incremental diff for me to look, then | 19:47 |
| leonardr | salgado: sure | 19:48 |
| === 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 | ||
| === jcsackett_ is now known as jcsackett | ||
| leonardr | salgado, http://pastebin.ubuntu.com/486435/ | 19:51 |
| leonardr | i converted a pagetest to a unit test, fixed a bunch of indentation, and did the interface switch i showed you earlier | 19:52 |
| leonardr | jcsackett, basic question: what is the difference between official_rosetta and translations_usage? | 20:01 |
| jcsackett_ | leonardr: official_rosetta is a bool, translations_usage is an enum letting us know more about how a service is used. | 20:04 |
| jcsackett_ | We're trying to phase out the official_ bools because they don't really provide enough data. | 20:04 |
| === jcsackett_ is now known as jcsackett | ||
| leonardr | so 'official' means that launchpad is the official handler of a project's translations | 20:04 |
| jcsackett | Right, which isn't aleays enough data. For example in project configs we can't tell that translations aren't even being done, just that no translations are on LP. | 20:06 |
| leonardr | jcsackett, you should file a bug to track the work done to fix the xxx on line 422, and mention that bug in the xxx | 20:17 |
| jcsackett | leonardr ok. | 20:18 |
| leonardr | you should do the same for the xxx on line 453 | 20:18 |
| jcsackett | Got it. | 20:18 |
| jcsackett | There's a repeated xxx, I'll update them all. | 20:21 |
| leonardr | jcsackett, r=me with the bugs filed | 20: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 | ||
| lifeless | leonardr: hi | 20:39 |
| lifeless | leonardr: I have a branch which was oked., but ec2 showed me the error of my code | 20:40 |
| lifeless | leonardr: I'd like you to comment on the last commit (only) on it, if you could | 20:40 |
| leonardr | lifeless, sure | 20:40 |
| lifeless | http://bazaar.launchpad.net/~lifeless/launchpad/bug-618367/revision/11477 | 20:40 |
| lifeless | is the commit | 20:40 |
| lifeless | https://code.launchpad.net/~lifeless/launchpad/bug-618367/+merge/34162 | 20:40 |
| lifeless | is the whole MP, if you want context | 20:40 |
| lifeless | basically, all I do in the commit is move checks for 'valid person' from WHERE to the JOIN | 20:40 |
| jcsackett | leonardr: Thanks. | 20:41 |
| === jcsackett_ is now known as jcsackett | ||
| leonardr | lifeless, what comes out of Person._validity_queries? | 20:46 |
| lifeless | leonardr: a dict | 20:46 |
| lifeless | leonardr: used in building queries | 20:46 |
| lifeless | Person._all_members uses it, I refactored the code from Person._all_members to make this bit of it usable by specifications | 20:47 |
| lifeless | leonardr: I've shrunk the dict | 20:47 |
| lifeless | because it now did not need to return WHERE clauses at all. | 20:47 |
| leonardr | lifeless: ok, i see where you're moving stuff from the WHERE to the JOIN, and i see how someone with only non-preferred emails would be excluded | 20:50 |
| leonardr | you're also changing the account_table join so that it only picks up active accounts. is that right? | 20:50 |
| lifeless | same bug | 20:51 |
| lifeless | if you had an account that wasn't active, you'd be excluded rather than being included but marked invalid | 20:51 |
| lifeless | sorry, the *row* would be excluded | 20:51 |
| lifeless | which drops the specification out | 20:51 |
| lifeless | in exactly the same way; the account row would match the old join, making account.status non-null, but then the where clause would exclude the whole row. | 20:52 |
| leonardr | ok, i understand. and oyu took the code out of specificatoion.py because _validity_queries no longer returns any constraints | 20:54 |
| leonardr | r=me | 20:54 |
| lifeless | right | 20:56 |
| lifeless | thanks | 20:56 |
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === 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 | ||
| === Ursinha is now known as Ursinha-afk | ||
| === matsubara is now known as matsubara-afk | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!