[00:53] <lifeless> can I tempt anyone with a review?
[00:54] <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:55] <mwhudson> lifeless: sure, i can take a look
[00:58] <lifeless> mwhudson: thanks; its in topic
[00:59] <mwhudson> oh yeah, that thing
[01:10] <lifeless> mwhudson: so, what did you think?
[01:11] <mwhudson> lifeless: sorry, got a bit distracted, have meld up now :)
[01:14] <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:15] <mwhudson> ah ok
[01:15] <mwhudson> again :)
[01:17] <lifeless> mwhudson: did you alter https://launchpad.net/ubuntu/+assignments in your recent refactorings ?
[01:18] <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:19] <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:20] <mwhudson> now, more apropos: "Find the persons or teams by which person is subscribed." <- i don't understand this
[01:20] <lifeless> ok
[01:21] <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:22] <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:23] <lifeless> I find making the one-line summary hard sometimes.
[01:23] <mwhudson> yeah, it can be difficult
[01:24] <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:25] <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:26] <lifeless> 'return all IPersons which cause person to be subscribe to self' ?
[01:27] <mwhudson> lifeless: s/self/this bug/ would be more consistent with other docstrings, but otherwise +1
[01:28] <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:29] <lifeless> mwhudson: I think we should use the full 80 columns, but thats an entirely different discussion
[01:29] <mwhudson> yes
[01:30] <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:31] <mwhudson> lp.testing.views
[01:33] <mwhudson> lifeless: gosh the browser code you're changing is horrid
[01:34] <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:35] <mwhudson> lifeless: hm, that's a fair point too
[01:37] <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:39] <mwhudson> lifeless: factory.makeTeam takes a members=[] argument
[01:39] <mwhudson> would reduce some hair in tests
[01:44] <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:46] <mwhudson> lifeless: in the browser doctest, "# count with 2 teams" should say 4 shouldn't it?
[01:46] <lifeless> oh bugger, yes.
[01:47] <lifeless> s/2/many/
[01:47] <mwhudson> :)
[01:47] <mwhudson> future proofing!
[01:48] <mwhudson> lifeless: in the same test class, how do you feel about s/initialize_view/record_view_initialization/ ?
[01:49] <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:50] <lifeless> pushing in a second
[01:53]  * lifeless ignores the 79 limit issue
[01:54] <lifeless> ok push running now
[01:56] <lifeless> mwhudson: you can clicky click now
[01:56] <lifeless> mwhudson: (though I don't know why you wait)
[01:57] <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:58] <lifeless> mwhudson: well its there
[01:59] <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
[02:02] <lifeless> thanks
[02:06] <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:07] <lifeless> please do
[02:07] <lifeless> if we had gettext
[02:07] <lifeless> the pluralisation would be easier for you
[02:08] <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> 76	list(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:09] <lifeless> is there some reason to have a list( there ?
[02:09] <thumper> nope
[02:09] <thumper> removed
[02:10] <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:11] <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:12] <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:13] <thumper> what is VWS?
[02:13] <lifeless> vertical white space
[02:13] <thumper> 269+ what?
[02:13] <thumper> I see it
[02:14] <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:15] <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:16] <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:17] <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:18] <thumper> I've moved on
[02:19] <lifeless> reviewed +1
[02:19] <lifeless> clicky clicky set your commit message etc
[02:19] <thumper> ta
[02:24] <lifeless> how many ms do you think it will take to render now ?
[02:39] <thumper> no freaking idea
[02:39] <thumper> although wallyworld_ says 42
[02:49] <lifeless> lies
[02:49] <lifeless> Would be nice
[02:49] <lifeless> but lies.
[07:11] <lifeless> if anyone wants a break from coding and would like to review whats in the topic, that would be grand.
[08:04] <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:23] <lifeless> done
[08:23] <noodles775> Thanks lifeless it's on pqm already.
[08:30] <lifeless> speaking of buffer 65
[08:30] <lifeless> do you have time to do a review perhaps ?
[08:35] <noodles775> lifeless: I can, although gmb might be starting his OCR round now too?
[08:35] <lifeless> gmb: are you?
[08:46] <noodles775> lifeless: looking now.
[08:48] <lifeless> thanks!
[09:16] <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:17] <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:18] <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:19] <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:20] <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:22] <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:23] <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:24] <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:25] <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:26] <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:27] <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:40] <lifeless> noodles775: thanks for doing this
[09:41] <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:42] <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:45] <lifeless> I'll do a dict or namedtuple
[09:49] <noodles775> Great, r=me (I had one or two other questions). Thanks lifeless.
[09:54] <lifeless> cool
[09:54] <lifeless> _ is a convention in python for 'ignored'
[09:54] <lifeless> or unused
[09:56] <lifeless> namedtuple is 2.6
[09:56] <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:57] <noodles775> gmb: yep.
[09:57] <gmb> Righty.
[09:58] <lifeless> noodles775: I like having it - its much more pithy than ignored_because_we_have_to_have_a_name_expr_here in range(...):
[09:59] <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.
[10:02] <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:03] <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:05] <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:06] <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:29] <wgrant> gmb: I have a massive (~30 lines) branch for you, if you've time.
[10:31] <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:33] <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:34] <gmb> wgrant, Will do.
[10:34] <wgrant> Thanks.
[14:24] <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:25] <bac> thanks
[14:31] <gmb> bac, What's with the wacky indenting around line 93-96 of the diff?
[14:31] <bac> gmb: dunno.  let me look
[14:33] <bac> gmb: dewhacked
[14:33] <gmb> bac, Eyefankyoo
[14:34] <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:35] <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:41] <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:42] <bac> gmb: duh
[14:42] <gmb> (Not least because I do underline only :))
[14:42]  * gmb checks TestsStyleGuide
[14:43] <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:51] <gmb> bac, r=me with the above changes.
[15:39] <EdwinGrubbs> gmb: can you review lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2
[15:40] <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:47] <gmb> EdwinGrubbs, Those two tests are *awfully* similar. Any chance they could be unified in their own TestCase (e.g. TestFooBatchNavigators)
[15:48] <EdwinGrubbs> gmb: sure
[15:48] <gmb> EdwinGrubbs, Cool.
[15:48] <gmb> EdwinGrubbs, Other than that, r=me.
[16:00] <EdwinGrubbs> thanks
[16:19] <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:20] <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:40] <abentley> noodles775, re: getComments, I don't understand how implementing it directly on DistroSeriesDifference would be programming to interfaces, not implementations.
[16:41] <noodles775> abentley: It wouldn't. It would be programming to implementations, rather than programming to interfaces wouldn't it?
[16:42] <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:43] <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:45] <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:48] <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:49] <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:51] <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:52] <abentley> noodles775, An example of an "instance" method that is really a staticmethod is BranchSet.getRecentlyChangedBranches()
[16:55] <noodles775> abentley: pushing http://pastebin.ubuntu.com/486363/
[16:56] <noodles775> which passes with: bin/test -vvm test_distroseriesdifference -m test_distroseriesdifferencecomment
[16:57] <abentley> noodles775, r=me.  Thanks for your changes.
[16:57] <noodles775> np, thanks for the review :)
[18:57] <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:58] <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:59] <bac> leonardr: yep, that's the one!  thanks.
[19:20] <jcsackett> leonardr: https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-official_rosetta/+merge/34229
[19:21] <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:40] <leonardr> jcsackett, ok
[19:42] <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:43] <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:47] <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:48] <leonardr> salgado: sure
[19:51] <leonardr> salgado, http://pastebin.ubuntu.com/486435/
[19:52] <leonardr> i converted a pagetest to a unit test, fixed a bunch of indentation, and did the interface switch i showed you earlier
[20:01] <leonardr> jcsackett, basic question: what is the difference between official_rosetta and translations_usage?
[20:04] <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] <leonardr> so 'official' means that launchpad is the official handler of a project's translations
[20:06] <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:17] <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:18] <jcsackett>  leonardr ok.
[20:18] <leonardr> you should do the same for the xxx on line 453
[20:18] <jcsackett> Got it.
[20:21] <jcsackett> There's a repeated xxx, I'll update them all.
[20:37] <leonardr> jcsackett, r=me with the bugs filed
[20:39] <lifeless> leonardr: hi
[20:40] <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:41] <jcsackett> leonardr: Thanks.
[20:46] <leonardr> lifeless, what comes out of Person._validity_queries?
[20:46] <lifeless> leonardr: a dict
[20:46] <lifeless> leonardr: used in building queries
[20:47] <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:50] <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:51] <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:52] <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:54] <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:56] <lifeless> right
[20:56] <lifeless> thanks