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