#launchpad-reviews 2010-08-30
<jelmer> lifeless: Sorry, I got distracted and forgot about the review. :-/
<lifeless> thats alright
<noodles775> Thanks for the db-review lifeless, I've just responded and will be around if you want to chat about it.
<lifeless> I'm hereish, I'll be in and out - its getting late here.
<noodles775> Yeah, I replied to the MP so you could leave it 'til tomorrow if you wish :)
<noodles775> stub: I've responded to your review (already approved) with an incremental to use the Message model for comments on DistroSeriesDifference when you've a chance to look over it: https://code.launchpad.net/~michael.nelson/launchpad/distro-series-difference-schema/+merge/33515
<lifeless> ok, I'll need to look closely at the model which I haven't done yet (as I mentioned)
<lifeless> please do drop the latest_ prefix; I'll get a more complete review for you tomorrow.
<stub> noodles775: Any reason that two DistroSeriesDifference rows can share a Message?
<lifeless> ish.
<noodles775> stub: Right - my mistake. I think I was originally thinking I'd need extra meta-data per comment (ie. which versions the comment was made with), but ended up removing it. So I'll just remove that second create table?
<stub> eh? I was wondering why you made (distroseriesdifference, message) UNIQUE. Making just message UNIQUE would make more sense.
<stub> So a Message is linked to no more than one distroseriesdifference.
 * noodles775 needs coffee.
<noodles775> Yes.
* StevenK changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> henninge: O hai! Review my branch? I have cookies ....
<henninge> ... COOKIES ....
<henninge> StevenK: is it "ifp-use-istore" ?
<StevenK> henninge: Indeed
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> StevenK: in progress ...
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> StevenK: looks good but you are introducing extra new lines in the import section that should not be there.
<StevenK> henninge: I like to seperate the canonical and lp sections, I should stop doing that?
<henninge> Running "utilities/format-imports <path1> <path2>" will fixt that for you.
<henninge> StevenK: yes as we'd like to standardize on what the script does.
<henninge> The script could, of course, be changed to enforce your way if that was the consensus ... ;)
<StevenK> henninge: Script ran, changes pushed
<henninge> StevenK: r=me ;-) Thanks
<StevenK> henninge: Thanks!
<henninge> StevenK: ah see, the script also found some other glitches.
<StevenK> henninge: I did notice, yes :-)
 * wgrant looks for more support for putting it in 'make lint' or similar.
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* benji changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> henninge: I have a small one for you: https://code.edge.launchpad.net/~benji/launchpad/bug-622765/+merge/34077
 * henninge looks
<henninge> benji: sure, I'll do it ;-)
<benji> heh :)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || reviewing: benji, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> benji: r=me with a few comments ;-)
* henninge changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> cool, taking a look
<benji> henninge: I don't see comments on the MP; are they somewhere else?
<henninge> hm, should be. Did it by email, let me chek.
<henninge> c
<benji> maybe you got cought in the recent edge flappage
<henninge> benji: it just came in! ;-)
<benji> yep, I just got it too; I guess I need to be more patient
<noodles775> henninge: I'll add one to the queue, but understand that you'll probably not get to it before finishing :)
* noodles775 changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Ah, you've finished :)
<henninge> noodles775: I have already finished but abentley is here, too. ;-)
<henninge> Hi abentley! ;)
<noodles775> abentley_: when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-model/+merge/34086
<henninge> noodles775: or was this an UI review?
<henninge> probably not
<noodles775> Nope.
<henninge> benji: hope you can handle my review because I have to go now. ;-)
<benji> henninge: I had one minor rebuttle.  Shall I redirect it to abentley?
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* benji changed the topic of #launchpad-reviews to: On call: abentley || reviewing: noodles775 || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> abentley: you get to channel henning: please see my last message at https://code.launchpad.net/~benji/launchpad/bug-622765/+merge/34077/+index thanks
<abentley> benji, it sounds like all you care about is the return value-- the parsed_lines value.  Is that right?
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> abentley: not quite; if parse_file respects logparser_max_parsed_lines then it will return 2 (the maximum in effect during the test run), I want to be sure that it both returns 2 (IOW, it hit the max) and show that it did not parse line 2 of the input file because that shows that it took the passed in value of lines_parsed into account when checking to see if the max has been reached
<benji> it strikes me that (a variation) on the above would be a good comment to add just before the asserts to explain them
<abentley> benji, then perhaps you should test that the parsed value of 2 is not in downloads.items()
<abentley> benji, it's better to not need a comment if you can avoid it.
<benji> I attempted to show that it wasn't parsed by asserting tha downloads.items() is empty.
<benji> (maybe I misunderstood you)
 * benji thinks he should have mentioned abentley's name.
<abentley> benji, I realize that.
<abentley> benji, I'm saying it's not clear what you're trying to demonstrate.
<abentley> benji, If you're trying to demonstrate that the parsed value of line 2 isn't in download.items(), you can express that pretty directly in Python.
<abentley> benji, self.assertTrue(line_2 not in download.items())
<benji> If you would prefer that, I'll spell it that way.  Me, when I see something like that I think "well, there are lots of values that aren't there, I wonder what is actually there and why that particular value not being there is important"; I don't feel like fighting over it though
<benji> abentley: wait, maybe I do feel like fighting ;)  If we assert that it isn't equal to something, then that test is unlikely to catch a bug that would make it parse more (say the test data changes and the negative assertion isn't updated or there is a bug in the negative assertion from the get-go (I typo it or something); therefore the assertion that the parsed data is empty is superior
<abentley> benji, whatever you want to test, test that.  Don't test other things.  But if you want to test that that what gets parsed is what you expect, I think it would be much clearer to not have a 404 in the list.
<benji> abentley: I can agree with that.  The 404 being the first thing is a mistake.  It should be something that generates a value in downloads.  I'll change the test accordingly.
<abentley> benji, works for me.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> abentley: it looks like henning forgot step 2: the Status is still "Needs review"
<benji> (https://code.launchpad.net/~benji/launchpad/bug-622765/+merge/34077/+index)
<abentley> benji, if you're satisfied that it's been approved, feel free to set it yourself.
<benji> ok; I didn't know if that was kosher or not
<benji> done, thanks
<leonardr> abentley, i think https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123 is ready for review
* abentley 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
<abentley> leonardr, your doctest looks like a unit test, not documentation.  Could you please rewrite them as unit tests?
<leonardr> webservice/oauth.txt?
<abentley> I was thinking of lib/canonical/launchpad/doc/oauth.txt and webservice/oauth.txt both look like their main goal is to provide test coverage.
<leonardr> i can turn webservice/oauth.txt into a unit test but i'd rather not have to convert doc/oauth.txt just now--i just made some minor changes to get it to work with the permission system
<abentley> leonardr, you don't have to change the existing code, but I'd like to see lines 61-84 of the patch as a unit test.
<abentley> leonardr, could you explain why these removeSecurityProxies are needed?
<leonardr> abentley: sure
<leonardr> we use OAuthAccessToken objects two different ways in the tests
<leonardr> we use them to demonstrate the server-side behavior when someone tries to touch an OAuthAccessToken
<leonardr> in that case the security proxy is left in place, and part of its job is to examine the request in which the server-side behavior is taking place
<leonardr> but, we also use an OAuthAccessToken object as a convenient repository for OAuth authentication information when we create *client-side* objects like LaunchpadWebServiceCaller and launchpadlib Launchpad objects
<leonardr> In those cases, we would like to look at OAuthAccessToken's data members without having the security check run, because the security check assumes there is a current request, and we're trying to _create_ a request
<leonardr> a similar situation occurs when we look at the incoming request and try to determine if it's signed correctly
<leonardr> we need to access OauthAccessToken's data members without the security proxy, because the security proxy assumes what we're trying to determine--that the request has been identified with a particular OAuth token
<abentley> leonardr, so there's a pretty big difference between the active OathAccessToken, which we must be able to see, and arbitrary OauthAccessTokens which happen to be in the database?
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> well, there are situations where you must be able to see access tokens from the database--that's the point of this branch
<abentley> leonardr, right.  I meant that for the current one, access is mandatory, but for others, it depends on your permissions.
<leonardr> sorry, i still don't understand
<leonardr> the server gets some string data from the client
<leonardr> which identifies an oauth access token
<leonardr> as part of verifying whether the client is really authorized to use this access token, we need to strip the security proxy and look at its data members
<leonardr> after the access token has been identified, all future attempts to look at _any_ access token are subject to the security proxy rules
<abentley> leonardr, could we make this verification internal to OauthAccessToken?  Then we wouldn't need external access to its protected members.
<leonardr> salgado, is this a good idea? take the code in servers.py, that calls checkNonceAndTimestamp, and everything else up to the end of the method? basically a OAuthAccessToken.getPrincipal method which we can leave unprotected?
<abentley> leonardr, sorry to be more anal than usual, but you did ask for a review of the use of removeSecurityProxy, so I'm trying to make sure I understand the issues.
<leonardr> sure
<abentley> the best case is to avoid using it in production code, but I can see that bootstrapping the security foo can be a problem.
<abentley> leonardr, I'm noticing that the line wrapping doesn't match our usual style.  Usually when we wrap, we put a newline after the opening parenthesis, then an indent, then the arguments.
<leonardr> all right, i'll fix it
<abentley> leonardr, see "Multiline function calls" in https://dev.launchpad.net/PythonStyleGuide#Whitespace%20and%20Wrapping
<abentley> leonardr, LaunchpadWebServiceCaller is not exposed in the API, right?
<leonardr> i don't understand the question. when you say 'api' do you mean the web service or some internal python api?
<leonardr> LWSC is a test class defined in testing/pages.py. it is not available except when the test suite is running
<abentley> leonardr, I mean the web service.
<leonardr> no, LWSC is a test class that makes HTTP requests _to_ the web service
<leonardr> it is not available _through_ the web service
<abentley> leonardr, so it seems like the risk of assigning the token to LWSC.access_token is minimal.
<leonardr> right. this is the 'client-side' code that uses an OAuthAccessToken as a data repository. but if you want it's easy to turn the token object into a dict or something to reduce the risk
<leonardr> of carrying around an unprotected object
<abentley> leonardr, that's okay, it looks like test code anyhow.
<abentley> leonardr, I've given it a "needs fixing".  I'll be happy to re-review.  https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123
<leonardr> ok
<leonardr> i'll work on it tomorrow. thanks
* abentley changed the topic of #launchpad-reviews to: On call: - || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley 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
#launchpad-reviews 2010-08-31
* 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?
<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.
<mwhudson> lifeless: sure, i can take a look
<lifeless> mwhudson: thanks; its in topic
<mwhudson> oh yeah, that thing
<lifeless> mwhudson: so, what did you think?
<mwhudson> lifeless: sorry, got a bit distracted, have meld up now :)
<mwhudson> can you really not do remove_tracer(self) ?
 * mwhudson has no idea
<lifeless> dunno, was just moving it so documented the limitation
<mwhudson> ah ok
<lifeless> it already existed if you passed in a function that itself did record_statements
<mwhudson> ah ok
<mwhudson> again :)
<lifeless> mwhudson: did you alter https://launchpad.net/ubuntu/+assignments in your recent refactorings ?
<mwhudson> oh man, lp.testing._webservice.QueryCollector?
<mwhudson> what's that doing there
<lifeless> mwhudson: existing ?
<mwhudson> i just hate the way we accumulate multiple copies of things that do nearly the same thing
<lifeless> right
<lifeless> so QC was *in a doctest* a month ago
<mwhudson> haha
<lifeless> its different to record_statements though
<mwhudson> so things are moving in the right direction
<mwhudson> yeah, but in ways that shouldn't *really* matter, right?
<lifeless> oh they should
<lifeless> very much
<mwhudson> not expecting you to fix it in this branch, just complaining out loud
<lifeless> I think it would be a huge mistake to treat them as interchangable
<mwhudson> now, more apropos: "Find the persons or teams by which person is subscribed." <- i don't understand this
<lifeless> ok
<lifeless> so a person can be subscribed to a bug directly or indirectly
<lifeless> the bugtask browser code (for me specifically) generates 300 queries whenever I look at a bugtask:+index page.
<lifeless> because it was answering the question 'what ways is robert subcribed' by python
<lifeless> and its not interested in the BugSubscriptions
<lifeless> its interested in the Person or Team records that are subscribed.
<lifeless> I find making the one-line summary hard sometimes.
<mwhudson> yeah, it can be difficult
<lifeless> anyhow, this thing returns a result set
<lifeless> which outputs IPerson
<lifeless> such that it outputs all IPersons which cause person to be subscribed to the bug.
<mwhudson> lifeless: that version makes more sense to me, perhaps
<mwhudson> but it's hard to say if that's only because i read it after i already understood what it did
<lifeless> 'return all IPersons which cause person to be subscribe to self' ?
<mwhudson> lifeless: s/self/this bug/ would be more consistent with other docstrings, but otherwise +1
<lifeless> 78 chars.
<lifeless> 79 with .
<lifeless> :P
<mwhudson> hahaha
 * lifeless pissess of the narrow-screen crowd.
<mwhudson> ok, stick with your version then
<lifeless> mwhudson: I think we should use the full 80 columns, but thats an entirely different discussion
<mwhudson> yes
<thumper> lifeless: want to review my tag cloud change?
<mwhudson> lifeless: TestBugTaskView.initialize_view, did you consider using create_initialized_view ?
<thumper> lifeless: it'll get rid of the timeouts
<mwhudson> lifeless: it's in lp.testing.view iirc
<mwhudson> lp.testing.views
<mwhudson> lifeless: gosh the browser code you're changing is horrid
<lifeless> thumper: sure
<lifeless> mwhudson: I wanted to be crystal clear about what I'm exercising
<lifeless> mwhudson: can switch it if you prefer
<mwhudson> lifeless: hm, that's a fair point too
<thumper> lifeless: https://code.edge.launchpad.net/~thumper/launchpad/different-cloud/+merge/34146
<lifeless> I'm just doing a task for lynne, brb
<mwhudson> lifeless: factory.makeTeam takes a members=[] argument
<mwhudson> would reduce some hair in tests
<lifeless> mwhudson: thanks, I'll definitely do that in future, can tweak here if you like.
<mwhudson> lifeless: i think it would make the tests significantly easier to follow
<lifeless> will do
<mwhudson> ta
<mwhudson> lifeless: in the browser doctest, "# count with 2 teams" should say 4 shouldn't it?
<lifeless> oh bugger, yes.
<lifeless> s/2/many/
<mwhudson> :)
<mwhudson> future proofing!
<mwhudson> lifeless: in the same test class, how do you feel about s/initialize_view/record_view_initialization/ ?
<lifeless> sure
<mwhudson> otherwise i think i'm fine :)
<lifeless> changes are done
<mwhudson> can you push the branch again and i'll mark it so in the web
<mwhudson> ah ok
<lifeless> pushing in a second
 * lifeless ignores the 79 limit issue
<lifeless> ok push running now
<lifeless> mwhudson: you can clicky click now
<lifeless> mwhudson: (though I don't know why you wait)
<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
<lifeless> mwhudson: well its there
<lifeless> thumper: nuke this:
<lifeless> 653	+
<lifeless> 654	+def test_suite():
<lifeless> 655	+ return unittest.TestLoader().loadTestsFromName(__name__)
<mwhudson> lifeless: click clicked
<mwhudson> +y
<lifeless> thanks
<thumper> lifeless: OK, should also remove it from the standard_test_template file then
<thumper> because that is where it came from
<lifeless> please do
<lifeless> if we had gettext
<lifeless> the pluralisation would be easier for you
<lifeless> thumper: I know you didn't touch this
<thumper> lifeless: tweaked and pushed
<lifeless> but
<lifeless> sorted(
<lifeless> 76	list(getUtility(IBranchCloud).getProductsWithInfo(num_products)))
<lifeless> ==
<lifeless> sorted(getUtility(IBranchCloud).getProductsWithInfo(num_products))
<lifeless> (sorted takes an iterable)
<thumper> yep
<lifeless> is there some reason to have a list( there ?
<thumper> nope
<thumper> removed
<lifeless> the slave store by default.
<lifeless> is a lie
<lifeless> 'the slave store.'
<thumper> ok
<lifeless> would be true
<thumper> it used to be defaulted :)
<lifeless> thats ok
<lifeless> have you tried the resulting queries on staging ?
<thumper> tweaked
<thumper> yes
<thumper> I have
<thumper> and it's fast
<lifeless> 269+
<lifeless> that new VWS should be removed; the auto formatter can do it for you I think
<thumper> what is VWS?
<lifeless> vertical white space
<thumper> 269+ what?
<thumper> I see it
<thumper> are we running the autoformatter?
<lifeless> not automatically
<thumper> I thought we weren't
<lifeless> but you can run it by hand AFAIK
<thumper> but it makes my nice import lines messy
<lifeless> its more stubborn than you
<thumper> my making something like "from storm.locals import And, Or" take 4 lines
<thumper> which I've been told is OK
<lifeless> See, I think that should be fine on one line.
<thumper> me too
<thumper> so...
<thumper> fix the autoformatter
<lifeless> and the policy
<thumper> agreed
<thumper> so I can leave the VWS?
<lifeless> oh the VWS is wrong regardless :)
<thumper> why?
<thumper> are we not separating canonical and lp?
<lifeless> no, because they are both 'local'
<lifeless> pep8 defines three classes of imports
<lifeless> stdlib
<lifeless> nonlocal
<lifeless> local
<thumper> ok
<lifeless> in that order
<thumper> fixed
<lifeless> so an external canonical.landscape import, say, would be nonlocal.
<lifeless> I doubt the autoformatter is sophisticated enough for that yet, but that would be the right thing to do
<thumper> I've moved on
<lifeless> reviewed +1
<lifeless> clicky clicky set your commit message etc
<thumper> ta
<lifeless> how many ms do you think it will take to render now ?
<thumper> no freaking idea
<thumper> although wallyworld_ says 42
<lifeless> lies
<lifeless> Would be nice
<lifeless> 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
<lifeless> if anyone wants a break from coding and would like to review whats in the topic, that would be grand.
<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
<lifeless> done
<noodles775> Thanks lifeless it's on pqm already.
<lifeless> speaking of buffer 65
<lifeless> do you have time to do a review perhaps ?
<noodles775> lifeless: I can, although gmb might be starting his OCR round now too?
<lifeless> gmb: are you?
<noodles775> lifeless: looking now.
<lifeless> thanks!
<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?
<lifeless> noodles775: yes, it's not sufficient
<noodles775> OK
<lifeless> because you need to discard the uninteresting columns so that you return Specification
<lifeless> not (Specification, .., ..,..)
<lifeless> in all other regards it would be equal
<lifeless> (this patch prefetches all at once; its just patches them up row by row)
<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.
<noodles775> Oh, does it?
 * noodles775 looks again.
<lifeless> it does one query
<lifeless> on Specification, Person(aliased assignee), Person(aliased approver), Person(aliased drafter), EmailAddress(aliased for assignee), Account(aliased for assignee), EmailAddress(aliased for approver ...
<lifeless> the decorator does two things:
<lifeless>  - pulls out the Specification, so that it looks like we only queried Specifications to clients of the api
<lifeless>  - preloads the Person objects for assignee, approver and drafter for that row - no new queries are issued.
<lifeless> [I has the tests to prove it]
<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.
<lifeless> noodles775: performance wise that doesn't matter though
<noodles775> but that may not be possible here.
<noodles775> Ah, ok.
<lifeless> noodles775: I don't see why it would be of advantage (or disadvantage)
<lifeless> because:
<lifeless>  - we have to have a row decorator to return just Specification
<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.
<lifeless>  - doing it in pre_iter_hook would then save no function calls, but add one function call to use the pre_iter_hook
<noodles775> Yes, its about saving queries, not function calls.
<lifeless> noodles775: if you're preloading via separate queries, I suspect you're doing it wrong :)
<lifeless> noodles775: though I can imagine a use case when the query optimiser goes beserk
<noodles775> lifeless: Sorry, s/preloading/caching
<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.
<lifeless> sure, yes.
<lifeless> note that cache_validity does no queries
<noodles775> Ah, right. I see... because its ownly using the preloaded data. Gotit.
<lifeless> noodles775: thanks for doing this
<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?
<lifeless> hmm, happy to change that.
<lifeless> namedtuple would be ideal
<lifeless> is that in 2.5 ?
<noodles775> Not sure, but yeah, that would be ideal.
<lifeless> I'll do a dict or namedtuple
<noodles775> Great, r=me (I had one or two other questions). Thanks lifeless.
<lifeless> cool
<lifeless> _ is a convention in python for 'ignored'
<lifeless> or unused
<lifeless> 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
<gmb> lifeless, Is the branch in the queue the one that noodles775 just reviewed?
<noodles775> Yeah, among other things (translatable strings, last result on console etc.).
<noodles775> gmb: yep.
<gmb> Righty.
<lifeless> noodles775: I like having it - its much more pithy than ignored_because_we_have_to_have_a_name_expr_here in range(...):
<lifeless> noodles775: Its the only single codepoint variable name I use ;)
<noodles775> lifeless: fine by me, might be worth adding to the style guide.
<lifeless> noodles775: http://pastebin.com/Q2dAz2ts
<lifeless> noodles775: is my proposed diff
<lifeless> please let me know if you think it is an improvement
<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...
<noodles775> Yep.
<noodles775> lifeless: and yes, I think the dict references are a bit more obvious than tuple indexes. Thanks!
<lifeless> my pleasure
<lifeless> thanks heaps for the review
<noodles775> np.
<lifeless> I shall push that change and send to ec2
<noodles775> lifeless: erm, don't you need to change the callsites too?
<noodles775> ah, that is the only one?
<lifeless> noodles775: two
<lifeless> one in specification, one in person
<noodles775> yep, never mind me.
 * noodles775 goes for a cuppa.
<wgrant> gmb: I have a massive (~30 lines) branch for you, if you've time.
<gmb> wgrant, Unless you're out by two orders of magnitude, in your estimation, I think I can handle that :)
<lifeless> +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
<wgrant> gmb: https://code.edge.launchpad.net/~wgrant/launchpad/bug-510180/+merge/34175
<gmb> wgrant, Nice and subtle. I like it. r=me
<wgrant> gmb: Heh. Thanks. Can you throw it into ec2?
<gmb> wgrant, Will do.
<wgrant> 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
<bac> hi gmb can i get a review?  https://code.edge.launchpad.net/~bac/launchpad/bug-39212/+merge/34196
<gmb> 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
<bac> 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
<gmb> bac, What's with the wacky indenting around line 93-96 of the diff?
<bac> gmb: dunno.  let me look
<bac> gmb: dewhacked
<gmb> bac, Eyefankyoo
<gmb> bac, ALso, line 109 needs to be indented by one more stop to be consistent.
<bac> gmb: i am going to open a but about URIWidget not respecting displayWidth, too.
<bac> gmb, yep, i caught that one too
<gmb> bac, Thanks x2
<bac> i think we have css that is harmful and should be removed, but i'm not certain of all of the areas affected.
<gmb> Fair neough.
 * gmb switches lcoations; brb
<gmb> bac, I thought that ReST headers just had to have the underline rather than the overline, too.
<gmb> e.g.
<gmb> Person's wikinames
<gmb> ===============
<gmb> (etc.)
<bac> gmb: really?  i've often seen over/under
<gmb> bac, In that case, we have a problem with consistency
<bac> gmb: if you know better that's one less line...
<bac> gmb: duh
<gmb> (Not least because I do underline only :))
 * gmb checks TestsStyleGuide
<gmb> bac, The example in TestsStyleGuide uses underline only, so let's standardise on that.
<bac> gmb: will do
<gmb> Thanks.
<gmb> 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
<EdwinGrubbs> gmb: can you review lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2
<EdwinGrubbs> oops, I meant to paste https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout-part2/+merge/34159
<gmb> EdwinGrubbs, Sure
<gmb> EdwinGrubbs, Those two tests are *awfully* similar. Any chance they could be unified in their own TestCase (e.g. TestFooBatchNavigators)
<EdwinGrubbs> gmb: sure
<gmb> EdwinGrubbs, Cool.
<gmb> 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
<EdwinGrubbs> thanks
<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?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-model/+merge/34086
<abentley> noodles775, I can have a look.
<noodles775> Great, thanks.
<abentley> noodles775, re: getComments, I don't understand how implementing it directly on DistroSeriesDifference would be programming to interfaces, not implementations.
<noodles775> abentley: It wouldn't. It would be programming to implementations, rather than programming to interfaces wouldn't it?
<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.
<abentley> noodles775, sorry, I still don't understand why moving it there would be the wrong thing.
<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.
<abentley> You just wind up calling a method on DistroSeriesDifferenceCommentSource that makes very implementation-specific assumption about DistrosSeriesDifference
<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.
<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.
<abentley> noodles775, my own rule is to be suspicious of staticmethods, because they are often cleaner as instance methods.
<abentley> noodles775, I don't think your rule applies to objects that implement an interface.
<noodles775> oh, I only used a static method because that's what most of the classes in lp.code do.
<noodles775> See lp.code.models.sourcepackagerecipe.SourcePackageRecipe.new, for example.
<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.
<noodles775> OK, so you'd like me to remove DSDifferenceComment.getForDifference() and put that functionality directly in the existing DSDifference.getComments() right?
<abentley> noodles775, right.
<abentley> noodles775, An example of an "instance" method that is really a staticmethod is BranchSet.getRecentlyChangedBranches()
<noodles775> abentley: pushing http://pastebin.ubuntu.com/486363/
<noodles775> which passes with: bin/test -vvm test_distroseriesdifference -m test_distroseriesdifferencecomment
<abentley> noodles775, r=me.  Thanks for your changes.
<noodles775> np, thanks for the review :)
<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.
<leonardr> bac, yes, see bug 627442, caused by bug 620508, recently triggered and incompletely worked around
<_mup_> Bug #627442: lazr.restful and batching cause a FeatureError <oops> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/627442>
<_mup_> Bug #620508: Slicing a ResultSet breaks subsequent len() calls. <Storm:New> <https://launchpad.net/bugs/620508>
<leonardr> gary and benji are working on it
<bac> leonardr: yep, that's the one!  thanks.
* 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
<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.
<leonardr> jcsackett, ok
<leonardr> salgado, can i ask you to take a look at https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2 ?
<leonardr> just the last two revisions
<leonardr> abently isn't around, but it's all oauth stuff so you should be able to do it
<salgado> leonardr, r11454 and r11455?
<leonardr> salgado: actually, it looks like it's everything from 11452..11455
<salgado> leonardr, it'd be nice if you could generate an incremental diff for me to look, then
<leonardr> 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
<leonardr> salgado, http://pastebin.ubuntu.com/486435/
<leonardr> i converted a pagetest to a unit test, fixed a bunch of indentation, and did the interface switch i showed you earlier
<leonardr> jcsackett, basic question: what is the difference between official_rosetta and translations_usage?
<jcsackett_> leonardr: official_rosetta is a bool, translations_usage is an enum letting us know more about how a service is used.
<jcsackett_> We're trying to phase out the official_ bools because they don't really provide enough data.
<leonardr> so 'official' means that launchpad is the official handler of a project's translations
<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.
<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
<jcsackett>  leonardr ok.
<leonardr> you should do the same for the xxx on line 453
<jcsackett> Got it.
<jcsackett> There's a repeated xxx, I'll update them all.
<leonardr> 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
<lifeless> leonardr: hi
<lifeless> leonardr: I have a branch which was oked., but ec2 showed me the error of my code
<lifeless> leonardr: I'd like you to comment on the last commit (only) on it, if you could
<leonardr> lifeless, sure
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/bug-618367/revision/11477
<lifeless> is the commit
<lifeless> https://code.launchpad.net/~lifeless/launchpad/bug-618367/+merge/34162
<lifeless> is the whole MP, if you want context
<lifeless> basically, all I do in the commit is move checks for 'valid person' from WHERE to the JOIN
<jcsackett> leonardr: Thanks.
<leonardr> lifeless, what comes out of Person._validity_queries?
<lifeless> leonardr: a dict
<lifeless> leonardr: used in building queries
<lifeless> Person._all_members uses it, I refactored the code from Person._all_members to make this bit of it usable by specifications
<lifeless> leonardr: I've shrunk the dict
<lifeless> because it now did not need to return WHERE clauses at all.
<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
<leonardr> you're also changing the account_table join so that it only picks up active accounts. is that right?
<lifeless> same bug
<lifeless> if you had an account that wasn't active, you'd be excluded rather than being included but marked invalid
<lifeless> sorry, the *row* would be excluded
<lifeless> which drops the specification out
<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.
<leonardr> ok, i understand. and oyu took the code out of specificatoion.py because _validity_queries no longer returns any constraints
<leonardr> r=me
<lifeless> right
<lifeless> thanks
* 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
#launchpad-reviews 2010-09-01
* StevenK changed the topic of #launchpad-reviews to:  On call: StevenK || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> StevenK: we can talk in public you know :)
<StevenK> We could ... people might talk, then :-P
<lifeless> or contribute
<StevenK> thumper: I've addressed almost all of your concerns for db-add-ifp-job, I'm just having trouble writing InitialiseDistroSeriesJob.create() since DistributionJobDerived.create() has been binned, and could use some pointers. Or code. :-)
<thumper> ok
<thumper> what isn't working?
<StevenK> thumper: I'm not at all certain what create() should be returning -- and to answer one of your questions on the MP, the code in .create() was checking jobs so that if a job for a specific distroseries was already in the queue, a second wouldn't be created
<thumper> why would you be calling it twice?
<thumper> I suppose it is bad to try twice
<lifeless> race conditions
<thumper> the problem with that is that there is go guarantee that even with checking that you won't get two
<StevenK> Because InitialiseDistroSeries is one-shot. You take a empty distroseries and copy a lot of stuff
<thumper> s/go/no/
<thumper> if you have two requests going to two different app servers at the same time
<thumper> both will look and not see the job there
<thumper> and both will create it
<thumper> if you have to be sure, you should add a database constraint
<thumper> with a unique partial index
<StevenK> Okay, I'll keep that in mind
<thumper> you want the create method to return the new distibutionjob
<StevenK> thumper: Doing that causes other tests to fail since DistributionJob doesn't have a run() method, but InitialiseDistroSeriesJob does
<thumper> StevenK: take a look at model/branchjob.py line 271
<StevenK> thumper: What should I RTFM for to add the database constraint?
<thumper> StevenK: it is the create index syntax that has a where clause (I think)
<lifeless> or you can just handle the job existing twice when it executes
<thumper> lifeless: partial index would arguably be better
<lifeless> thumper: why?
<thumper> because it better represents the reality of the situation that there should be at most one
<lifeless> so, a partial index means that a collision in the web ui will result in a collision error
<lifeless> which is fugly
<lifeless> and one that is caught outside the template code so can't be made to look good
<thumper> ahm...
<thumper> no
<lifeless> secondly, its trading consistency for performance, its harder do queue things the more constraints you want up front
<thumper> it means the second attempt will get retried
<thumper> which can then find an existing one
<thumper> and say "dumb ass, don't do that"
<lifeless> really?
<thumper> yes
<lifeless> thats ... interesting
<lifeless> I sure hope we don't do that on apport uploads
<lifeless> anyhow
<thumper> the db constraint error causes the request to be retried
<thumper> I dunno about the apport uploads
<lifeless> its then causing more work :)
<lifeless> of a different sort, in a different context.
<lifeless> I don't mind which way you do it.
<thumper> heh
<thumper> you gotta handle it somewhere
<lifeless> If/when we move to rabbit it will *have* to be done the way I'm describing.
<lifeless> But you're welcome to do whatever way makes more sense to you know.
<thumper> well...
<thumper> not necessarily
<thumper> we don't want to fire jobs to rabbit for a transaction that'll be aborted
<lifeless> why not?
<thumper> so it will have to hook into the transaction somewhere
<thumper> isn't that obvious?
<lifeless> Nope.
<thumper> how about the data not being there?
<thumper> inconsistent expected stat
<thumper> e
<StevenK> thumper: So I'm expecting the test to now raise an IntegrityError if I've added the constraint right?
<thumper> StevenK: depends how you've done it
<lifeless> thumper: what data wouldn't be where? and what sort of abort are you expecting?
<thumper> lifeless: I'm giving examples of why I think it is a bad idea to fire events for something that'll be aborted
<thumper> I don't have concrete data
<lifeless> 2PC is pretty nasty perf wise; its almost certainly better to fire-and-check-when-executing.
<lifeless> BASE rather than ACID
<thumper> what 2pc?
<thumper> we don't have two phase
<lifeless> exactly
 * thumper walks away
<lifeless> thumper: I think we're miscommunicating or something.
<lifeless> thumper: as I said, right now, whatever makes sense, great.
* StevenK changed the topic of #launchpad-reviews to: On call: StevenK || reviewing: mmm, lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> thumper: sorry about before, I was needlessly antagonistic; I think I need another neurofen.
<lifeless> (but we're out)
<thumper> lifeless: np
* jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> StevenK: / jtv: who wants this one ?
<jtv> lifeless: I'll take it
<jtv> â¦whatever it is
<jtv> â¦assuming it's a Launchpad branch to be reviewed, of under 800 lines of diff.
<lifeless> hah
<lifeless> well
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/34272
<lifeless> diff coming up now
<jtv> Why do the active-reviews listings just have to get worse and worse?
<lifeless> so the theme of this patch
<lifeless> and I don't know how big it is
<lifeless> is a prepatory patch to logging:
<lifeless>  - resultset iteration
<lifeless>  - email sending
<jtv> diff's here
<lifeless>  - memcached time
<lifeless>  - rabbit in future
<lifeless> 713 lines. whew :P
<lifeless> there is one particular bit of hair.
<lifeless> I wanted to use request annotations
<jtv> That's okayâI submitted a 795-line one the other day
<lifeless> but we can't for anything that scripts use.
<jtv> Have you looked at how we do this for oopses in scripts?
<lifeless> so this has that code disabled, references to the bug about this, and instead works in with the existing thing in adapter.py
<lifeless> jtv: I spent two and a half days trying to make it work
<jtv> lifeless: it may be lack of context, but I don't understand the comment in line 65 of the diff.
<jtv> # Requires poking at the API; default is to Just Work.
<lifeless> its context
<lifeless> the new API creates a Timeline on demand
<lifeless> which is particularly well suited for request annotations
<lifeless> which we will eventually make work in scripts
<lifeless> but
<lifeless> checkwatches wants a limitedlist rather than grabbing all the queries
<lifeless> which is odd, but to keep things scoped I just put in compat code
<lifeless> I can expand on that, or remove the comment if you like.
<lifeless> I wanted it to be clear why there isn't an else clause.
<jtv> lifeless: be aware that you're at least 2Â½ days ahead of me in this; I'm having a very hard time following you.
<lifeless> jtv: I started this last tuesday - so 8 days ahead :)
<lifeless> jtv: I suggest starting with lib/lp/services/timeline/
<jtv> Okay, 8 days plus the context I'm not seeing in the diff.  In other words: I haven't a frakking clue what you're on about!
<lifeless> you'll seem some ugliness about _local
<lifeless> but ignore that; the module and its tests should tell you what its on about
<jtv> lifeless: lib/lp/services/timeline/__init__.py, diff line 355: "Because this part of [...],"âmissing "is" I think.
<lifeless> yes, will add
<jtv> lifeless: in requesttimeline.py, diff line 366: if the Timeline in that docstring is a class name, please backquote.
<lifeless>  single or double?
<jtv> Single.
<jtv> `Timeline`
<jtv> And while you're there, please normalize __all__ to multi-line format.
<jtv> Below that, according to the standards I learned at the time, the XXX should go "XXX RobertCollins 2010-09-01 bug=623199: Undesirable but pragmatic."
<lifeless> switched around
<jtv> In diff ll. 389â390, why do you keep the dead code around?
<jtv> Is it likely that we'll want to get back to it very soon, and that it won't have changed by then?
<lifeless> yes
<lifeless> fixing the mess up will be hard
<lifeless> but it makes a lot of stuff terrible.
<lifeless> feature flags
<lifeless> this
<lifeless> request timing in general
<lifeless> security contexts
<lifeless> code clarity all through lib/canonical/launchpad/webapp
<lifeless> I am generally an advocate of 'if we are deleting it, let the vcs remember'
<lifeless> but in this case I want folk looking at this code to see how it *should* look.
<jtv> The two approaches look quite similar to me, just structured slightly differently.
<lifeless> one is thread local
<jtv> Ahhh
<lifeless> which has all sorts of caveats and issues; the other is zope interface based, which is more flexible
<jtv> That clarifies things.
<jtv> So two aspects get intermixed here: the way you get to the request, and the way you create the timeline on demand.
<lifeless> mmm
<lifeless> I'm not sure I see what you mean
<jtv> I think I said it wrong.  Would this be right?  "You changed two different aspectsâwhere the timeline is supposed to be, and how you create it on demand."
<lifeless> yes
<lifeless> kindof
<lifeless> there used to be an api to get the timeline in its less general form
<lifeless> so this adds an api to get the timeline which lets other places than adapter.py write to it
<lifeless> what existed before was a readonly api, as it were
<lifeless> to make the new api easy to use, it creates on demand, but in practice, 'set_request_started' will still be getting used everywhere.
<lifeless> so until we nuke that function, the new niceness won't really be visible
<jtv> So what you have is getter/setter functionality which you changed to use a different path to the variable, and on-demand creation which could work unchanged for both the old and the new model.
<lifeless> fingers crossed :)
<lifeless> I'd like to get rid of the setter, but its needed to migrate
<lifeless> chicken and egg, or really big patches.
<jtv> What confused me is that the dead code differs from the live code on both scores, yet only one of the two matters.
<lifeless> oh, ah
<jtv> How about instead of keeping the dead code, you just have an XXX to say "in the future we will keep this in request.annotations['timeline'] instead of webapp.adapter._local.request_timeline"?
<jtv> Then just keep the structure of the live code, and when the time comes, only change the place where you get/set the timeline.
<lifeless> so, now I know what you were thinking
<jtv> It had a lot to do with that 8-day head start.  Doesn't necessarily make sense semantically.  :)
<lifeless> these two implementations of these two  functions are externally identically equivalent
<lifeless> that is, get_request_timeline autocreates in both cases
<jtv> Right, but you're interleaving dead and live code that differ in multiple aspects, for no good reason.  Highly confusing!
<lifeless> and set_request_timeline sets it
<lifeless> jtv: they don't different in multiple aspects, except one thing: the return from set_, which is just for avoiding duplication because _local isn't a dict.
<jtv> Well there's that too.
<jtv> I'd *definitely* avoid that particular difference between the live and the dead code.
<lifeless> mmm
<lifeless> I would rather you look at the hairy stuff
<lifeless> this seems really minor to me.
<lifeless> I can make set_ return as well in the old code path, but there is no need.
<jtv> This _is_ really minor, but there shouldn't be such big obstacles to understanding the code.
<lifeless> I don't understand the obstacles
<lifeless> I guess
<lifeless> its two lines
<jtv> It's more than two lines if you count the code that you'll want to remove later on; you just optimized for minimizing the dead code instead of for clarity.
<lifeless> I'm optimising for vision
<jtv> Well my vision is blurred right now!
<jtv> SCNR
<lifeless> if the desired code is not visible
<lifeless> the reasons for the external import are less clear
<lifeless> the understanding of someone coming and saying 'why is this the way it is' will be less
<lifeless> if adapter wasn't a terrible terrible global variable pita
<lifeless> I wouldn't need to do this and noone would need to care.
<lifeless> brb, getting wood for the fire
<lifeless> back
<jtv> OK.  Here's something simple you could do to make it clear what the intention of the code is: the "Disabled code path" comments say that the dead code is undesired.  If it's the desired code, make that clear.
<lifeless> here is a patch to increase the info in that file
<lifeless> http://pastebin.com/5BHK8qhA
<lifeless> I'll add in the change you just suggested
<jtv> Thanks.
<lifeless>     # Disabled code path: bug 623199, ideally we would use this code path.
<_mup_> Bug #623199: scripts do not establish valid zope partiticipations <Launchpad Foundations:New> <https://launchpad.net/bugs/623199>
<lifeless> hows that
<jtv> Much better, but "ideally" suggests that this dead code can stick around indefinitely, until perhaps it no longer makes any sense at all.  Either you have an XXX saying that the code must change to this new form and soon, or the dead code is dead weight.
<lifeless> there is an xxx, right up the top of the file
<lifeless> can make them all look like that if you like, but they aren't going to vary independently
<lifeless> only the first one is needed for the XXX report
<jtv> Who's going to make the change, and when?
<lifeless> don't know, as soon as I can arrange it
<jtv> The reason I ask is that the relative value of having the future code spelled out decreases with the time it takes to get to it; the effort involved; and how far removed the person doing it was removed from what you've been working on for the past 8 days.  The change you pasted me makes the intent clear, which is a much bigger help.
<lifeless> When the future code shapes architecture, I disagree about the value diminishing; I think its very helpful to be clear about how things *should* be
<lifeless> if it was just a different query on a maybe-table, it would be a bit of yagni
<jtv> lifeless: that's all very nice, but the part you're documenting in the form of dead code is easily reconstructed from the intent.  Going the other way is harder: trying to figure out what the dead code is trying to say, and restoring it.
<lifeless> I don't understand
<jtv> It's a shame to throw away better code that almost worked, but I think in a case like this it's for the best.
<lifeless> I don't follow your argument for deleting it
<jtv> What you have is dead code.  As a reader, I have to figure out why the dead code is there, and how it fits into the picture.  Then, if I want to fix the issue, I have to figure out how it relates to the live code.  The two sit slightly awkwardly together because the same code paths would work for both yet they follow different code paths.  But the fundamental change is one that's concisely expressed in one line, and everything else follows from thereây
<jtv> So now you're making the documentation better, which is good, but it makes the dead code entirely redundant.
<lifeless> well it doesn't
<lifeless> a picture paints a thousand words
<lifeless> the code is exact. Its a direct statement.
<lifeless> anything else is a pale shadow.
<jtv> It specifies irrelevant detail and leaves the essence implicit.
<jtv> The essential difference between the live and the dead code is that the timeline is kept in a different place.
<lifeless> no
<lifeless> thats not the essential difference.
<jtv> Then what is?  And can the code make it clearer to me?
<lifeless> or rather, phrasing the different that way trivialises a huge change
<lifeless> webapp.adapter uses thread locals
<lifeless> its not safe with:
<lifeless>  - twisted
<lifeless>  - threadpools
<lifeless>  - async of any sort
<lifeless> we do a tonne of crazy stuff
<lifeless> all the work we deferToThread in the twisted apps we have could easily break in subtle ways, at any time
<lifeless> its very important to me that we should the right way of doing it, even if we can't.
<jtv> lifeless: No, those are the reasons behind the effort that produced the dead code.  The effort itself AFAICT is to keep the timeline in a different place, where it's thread-safe.  Moreover, the dead code in the getter and setter keeps all of this entirely implicit, and hides it among secondary differences because the two code paths avoidably use different idioms for on-demand setting.
<lifeless> do you mean unavoidably?
<jtv> No, I mean avoidably.
<lifeless> ones a dict
<lifeless> one isn't
<lifeless> I don't see how you can use the same idiom
<lifeless> any still be writing code idiomatic to the type
<lifeless> anyhow, this is driving me up the wall. I feel like you want to make it perfect in one particular slant, which destroys its value in other ways. I care about the other ways much more.
<jtv> I'm glad we're agreed.  What I'm saying is: don't struggle so hard to keep 2 lines of dead code around if instead you can make the intent clear enough that the code follows automatically.
<jtv> If it was _just_ two lines of difference, it'd be defensible.  If all these non-essential changes follow, it becomes struggling.
<jtv> (I'm counting the lines you'd delete in order to make the dead code live)
<jtv> If you really want to keep them, then I can live with that.  Just be clearer about the intent.
<lifeless> Well, I've added the prose we discussed and rewritten docstrings etc as i pastebinned.
<lifeless> I'm cooperating on achieving our mutal goals of good code and clean design
<lifeless> the 'non essential changes' you seem to be referring too, would become longer if those two lines of code are not there.
<jtv> And even if it doesn't sound like it, I appreciate that and I do care about getting your improvements in.
<jtv> For the record, I don't object to nonessential changes per se.  Only the difficulty of figuring out how the "before" and "after" pictures of the code relate, if they are in the code side by side.
<jtv> (In the tests I do very much like what you did for the same purpose: add a testable specification of how it _should_ work, even if that can't be implemented yet)
<jtv> A TimedAction doesn't need a description?
<lifeless> context?
<lifeless> there is a class docstring
<jtv> Sorry, I meant "category."
<lifeless> its mandatory in the constructor
<lifeless> its not evaluated by the object
<lifeless> same for detail
<jtv> Just curious because I saw one of the tests pass None as a category.
<lifeless> can change, I was just only-filling-in-what-really-mattered
 * lifeless does the happy dance, the performance work from yesterday is merged.
<jtv> I don't see a particular need to change it myself, just wondering if you might (from a more complete perspective)
<lifeless> I think its fine
<jtv> ah, that reminds me of some queries I wanted to zap on one of our slower pages
<jtv> â¦but that's for another time
<jtv> I think my world would be just a tad nicer if right-click menus never immediately placed a potentially harmful action right under the mouse pointer.
<lifeless> heh yes
<lifeless> that would be nice
<jtv> I'm wondering if Timeline.baseline could ever misleadâ¦  I could be wrong, but ISTM the timeline could be created explicitly, in which case it's meaningful; or when a first action came along, in which case it's basically the same as self.actions[0].start; or implicitly as a side effect of something indeterminate, in which case it's shaky at best.
<lifeless> thats a good point
<lifeless> I think we should a) move to a request.annotations model, and then b) make an IRequestStart hook trigger a timeline existence
<lifeless> Oh, and this is a reason to put an else block in adapter.py
<lifeless> where you asked about originally.
<jtv> Would it be easier perhaps to have a "start timeline" event?
<lifeless> I don't think so, because the point is that you want requesttimeline to be /subscribed/ not /emitting/
<lifeless> +    request = get_current_browser_request()
<lifeless>      if request_statements is not None:
<lifeless>          # Requires poking at the API; default is to Just Work.
<lifeless> -        request = get_current_browser_request()
<lifeless>          set_request_timeline(request, Timeline(request_statements))
<lifeless> +    else:
<lifeless> +        # Ensure a timeline is created, so that time offset for actions is
<lifeless> +        # reasonable.
<lifeless> +        set_request_timeline(request, Timeline())
<lifeless> bah, haven't changed the first comment ther,e will do so - but you see the change
<jtv> I used the wrong term again.  Forgive me.  A start _action_, so you never have to wonder whether the actions list is empty and don't have to keep a baseline time.
<lifeless> interesting idea.
<lifeless> let me think for a minute
<jtv> Hmm... I think I can find an easier spelling for the if/else you just pasted.  :-)
<lifeless> oh, sure I can pull the set_request call out of it
<jtv> set_request_timeline(request, Timeline(request_statements))
<lifeless> but its a line longer with another local variable
<jtv> I think ^^ is equivalent to the entire if/else.
<lifeless> I think that would mislead the eye
<lifeless> its true that when it is None it is equivalent to not passing it because the implementation uses None as the sentinel
<lifeless> ok, start actions.
<lifeless> I think that would need more special cases.
<lifeless> because:
<lifeless> log_tuples are derived from the baseline, not the prior action.
<lifeless> they have to be because people may use a LimitedList (until we sort out checkwatches etc...)
<jtv> Well, the baseline would just be self.actions[0].start
<lifeless> and if you derived it recursively, the first action can disappear
<lifeless> so, if you had a start action, you have to make sure it cannot magically go away when someone uses a LimitedList
<jtv> oic
<lifeless> which would require gymnastics
<lifeless> also
<jtv> So then by all means don't do that.
<lifeless> the start is an instantaneous thing, not a blocking event, so its not interesting like other TimedEvents
<jtv> Maybe though just pull the datetime.datetime.now(UTC) out into a method so it doesn't open you up to maintenance skew?
<lifeless> what do you mean?
<jtv> Well actually I have ulterior motives also.  I'll explain.
<jtv> I sometimes get those annoying "hey you're mixing timezone-aware timestamps with timezone-naÃ¯ve ones" errors.
<lifeless> yes, they suck. This api is entirely tz aware because of that.
<jtv> To minimize that distraction while developing, I like having the code that "reads the clock" for one particular item concentrated in one place.
<jtv> The ulterior one is, of course, that it's a very easy way to mess with timestamps in tests.  There's supposed to be better ways to do that, but I haven't gotten around to them yet.
<jtv> (And by "mess with," I mean "produce predictable")
<lifeless> sure, a test double time source
<lifeless> do we have one system wide already?
<jtv> I don't _think_ so
<lifeless> Its a little complex to get right, and perhaps a simple mock is better.
<lifeless> I defintely don't want to go down that hole speculatively here.
<lifeless> I like the idea, I just think is very conceptually different, and as you can see, only one test would really benefit today.
<jtv> Noâ¦ the main thing I'm driving at is "clock-reading code is often best extracted and concentrated."
<lifeless> I'm agreeing.
<lifeless> we have it all over the place
<lifeless> I'm saying 'Rather than start a new trend in this patch, we should do it separately'
<jtv> Well starting a new trend is a different matter perhaps.
<lifeless> When I have a strong opinion on the how of it I'll do that in small pieces (like this entire patch is)
<lifeless> I don't for clock reading code; I can see at least three different approaches
<jtv> Something else: in the  summarize_requests output, maybe just generalize "queries" to "actions," and possibly replace "issued" with "performed" or somesuch?
<jtv> (No strong feelings on the clock issue here, so do it the way you want it)
<jtv> lifeless: any ideas on the baseline issue?
<lifeless> jtv: whats the bsaeline issue ?
<jtv> Maybe pass it explicitly based on request_starttime?
<lifeless> that it might be out of sync?
<lifeless> hmm
 * lifeless strongly suspects thats a misfeature
<lifeless> so right now:
<jtv> Oh, that's not much good if you create on-demand, is it
<lifeless>  - no tests failed
<lifeless>  - production doesn't play tricks with time
<lifeless> I think its a yagni, with the patch I pasted in here applied.
<jtv> If you're always creating the timeline early on, that solves it.
<lifeless> yeah
<jtv> Another small thing: in diff lines 229â232, the preferred way to access items in a tuple is to unpack it.  I think that'd be a lot clearer in this case.
<jtv> That's errorlog.py, starting around line 454 or so.
<lifeless> doing
<jtv> thx
<lifeless> -            log_tuple = action.log_tuple()
<lifeless> +            start, end, category, detail = action.log_tuple()
<lifeless>              statements.append(
<lifeless> -                log_tuple[:2] +
<lifeless> -                (_safestr(log_tuple[2]), _safestr(log_tuple[3])))
<lifeless> +                (start, end, _safestr(category), _safestr(detail)))
<lifeless> I have some habits from writing extremely tight python code
<jtv> Wonderful.
<jtv> Believe me, SteveA had to kick some of those habits out of me when I was new...
<jtv> lifeless: I notice you have a few cases where you look for categories starting with "SQL-".  Is that something that's going away?  Or perhaps something to move into the timeline class?
<lifeless> well, I'm not sure these will get kicked out
<lifeless> but I will certainly be reminding myself when code is inner loop or not
<lifeless> jtv: its going away, its the old API : I didn't want to have to update tests in this patch
<jtv> OK
<lifeless> tests that consume the old API I mean.
<jtv> Yes, I understand.
<lifeless> the SQL- prefix in adapter.py will stay, but nothing will filter on it in the main lp codebase
<lifeless> oopstools may, but the format is a disk format and tricky to evolve
<lifeless> its a separate concern to the timeline anyway
<jtv> Then it's not worth messing with.  I did have to squint at this one though:
<jtv> 262	+    >>> for action in get_request_timeline(
<jtv> 263	+    ...    get_current_browser_request()).actions:
<jtv> 264	+    ...    if not action.category.startswith("SQL-"):
<jtv> 265	+    ...        continue
<jtv> 266	+    ...    print action.detail
<jtv> Wouldn't that be much clearer as:
<jtv> >>> timeline = get_request_timeline()
<jtv> [insert argument I forgot because of the line break in the original]
<jtv> >>> for action in timeline.actions:
<jtv> ...     if action.category.startswith("SQL-"):
<jtv> ...         print action.detail
<lifeless> 6 of 1 I think
<lifeless> changed
<jtv> Thanks.
<jtv> lifeless: In webapp/servers.py I see you're replacing sql_statements with nonpython_statements.  It could be helpful to standardize on a single term, for grepability etc.
<lifeless> like actions ?
<jtv> Yes, that's a good one.  :)
<lifeless> changed
<jtv> lifeless: thanks.  Then, in TestRequestTimeline, there's a pair of comments with --start-- and --end--.  Is that idiomatic?  I haven't come across it before, and tbh it's the sort of thing I might easily break if there were a few more tests between them.
<lifeless> its grouping the tests to delete when we fix the bug
<jtv> Ah!
<jtv> Obvious once you know it. :)
<lifeless> I'll add a one-liner reference to the bug on the top one
<lifeless>      # Tests while adapter._local contains the timeline --start---
<lifeless> +    # When bug=623199 is closed these should be deleted.
<lifeless>      def test_new_request_get_request_timeline_uses_webapp(self):
<jtv> Great.  Maybe just "# Delete when bug=623199 is fixed" for impact?
<jtv> On our team at least we've been following somewhat different XXX practices: we'll happily add multiple XXX'es for a single bug, to mark all the places that need changing.  (They're meant to irk us, after all.)  This is one of the places where we'd do that.
<lifeless> +    # Delete when bug=623199 is fixed and the timeline store moved to
<lifeless> +    # annotations.
<jtv> Even better.
<lifeless> so, I don't think irking people helps stuff get done
<lifeless> I think its important to find all the call sites
<lifeless> and if you see a number that is actually for a fixed bug thats important too
<lifeless> so - to me - having it in each context that its relevant is crucial
<lifeless> swamping folk with it is just cruel
<jtv> Well, it should reflect your intent to fix it.
<jtv>  We were supposed, once upon a time, to pursue XXX'es per se.
<jtv> Then, the link to a bug became mandatory and we're supposed to pursue bugs.
<jtv> But yes, now we find that there are always more bugs than you can solve.  :)
<jtv> So maybe you're right and we shouldn't overdo that.
<jtv> lifeless: on a different matter, in TestTimedAction.test_log_tuple, it _would_ be attractive to get rid of the indeterminacy in the timer.
<lifeless> jtv: well, it is determinate :)
<lifeless> the test won't fail because of clock issues
<lifeless> back shortly, organising foods
<jtv> lifeless: in TimedAction, log_tuple doesn't meet our standard for method names.  But it does meet our standard for property names, and it looks relatively cheap to compute, so maybe just make it a property?
<lifeless> ugh
<lifeless> I will have to bring this up, PEP8 is good.
<lifeless> I will rename it
<lifeless> (and no, it does computation, so its not suitable for being a property)
<lifeless> jtv: this is btw, a pathology of the lp review process, I think.
<jtv> lifeless: which one?
<lifeless> specification over outcome
<lifeless> I will expound on it another time
<jtv> OK, let's get this review done first.
<jtv> lifeless: typo: s/sancrosant/sacrosanct/g
<jtv> In OverlappingActionError.
<lifeless> thanks
<jtv> Speaking of which, ISTM there are two separate basic cases where you'd run into the non-overlapping rule: failing to mark completion, and nesting.  For the former, I'd say the rule is more than just a way to simplify analysisâit detects potential bugs.
<lifeless> it does, but not a worthwhile class to detect
<lifeless> because
<lifeless> those events won't be logged *anyway* in such a case.
<lifeless> so the developer putting the notifier together will see it immediately.
<lifeless> I mean, it nice as a bonus, but I'd take nesting, if desired, without argument
<jtv> Why won't those events be logged anyway?
<lifeless> something that isn't finished has no OOPS representation today
<lifeless> you could fudge it
<jtv> For a query that probably won't matter much, since a failure normally aborts the transaction.  But for generalized actions, it may be worth catching if the exception path isn't handled properly.
<lifeless> I'd like to call YAGNI here
<lifeless> it can be added
<lifeless> its not needed now, because every request ends with DB queries which will trigger if the thing before isn't closed off properly.
<jtv> I don't see the argument for yagni.  The functionality is already there, is it not?
<lifeless> no
<lifeless> we'd need to add a close-off call, to say 'check that the last thing isn't finished', and call that at the end of the request
<lifeless> note that 'end of the request' its terribly badly defined today for scripts - see the bug I have been ranting about.
<lifeless> its not that its terribly hard, its that it has no interesting use case today; you'd need to be using this in a situation that is outside of launchpad core for it matter in any practical sense.
* jelmer changed the topic of #launchpad-reviews to: On call: StevenK, jtv, jelmer || reviewing: -, -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> and it is a bit hard to do properly, because of 'request' and 'scripts' mixing so badly.
<jtv> Well in the case where it happens at the end of a request, it'll usually be fairly obvious that the action is there.  I'm thinking more of catch-retry cases.
<lifeless> try: action; except: actionagain
<lifeless> ?
<jtv> Well, probably more like except: alternativeaction.
<lifeless> that will raise already, so no changes are needed, if action() doesn't close off.
<jtv> That's exactly what I'm saying.
<lifeless> then I'm horribly confused
<lifeless> I thought you were asking for a change
<jtv> No.  I'm saying, the check will detect potential bugs.
<lifeless> ok, so I acked that. I said it wasn't a deliberate feature.
<jtv> Yes.  I'm saying you shouldn't be so apologetic for the limitation.
<jtv> But anyway, I'm done and approving the branch.
<lifeless> thank you very much for spending this time on the review.
<jtv> It's what OCRs are for.
<lifeless> jtv: so you're clicky-clicking in the webui now ?
<jtv> lifeless: yes, if you are using that term to describe the sound of my keyboard.
<lifeless> jtv: oh, are there other changes I need to make?
<jtv> Fear not.  There were a few interruptions slowing me down.
 * lifeless wants to throw it at ec2 to find out what hidden braindamage I missed
<lifeless> I ran the previous iteration through ec2 test
<jtv> The only thing is "make sure the comments on the dead-code lines aren't easily misread as saying the future code is obsolete."
<lifeless> which is how i found out about that heinous bug
<lifeless> jtv: did that
<jtv> That must've been a painful discovery.
<lifeless>     # Disabled code path: bug 623199, ideally we would use this code path.
<_mup_> Bug #623199: scripts do not establish valid zope partiticipations <Launchpad Foundations:New> <https://launchpad.net/bugs/623199>
<jtv> Ah yes.  Good.  Now let me finish my review so we can all get on.  :)
<lifeless> jtv: it was unpleasant. Plus I was 1 day into caffeine withdrawal. Not a good combo.
<jtv> àº­àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²àº²
<lifeless> sory, I don't have that glyph
<jtv> It's "AAAAAAAAAAAAAA!" in Lao.
<lifeless> :)
<jtv> Do you have it in Thai?  à¸­à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²à¸²
<lifeless> yes
<lifeless> looks like hills or something
<jtv> lifeless: it is done
<jtv> time for a break
<lifeless> thanks
* StevenK changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv-afk changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi jelmer! I've got one for your queue when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-getByDistroSeries/+merge/34289
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> noodles775: hey!
<jelmer> noodles775: Of course
<noodles775> Great, thanks.
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> gar, forgot to push my lint fixes.
<noodles775> r9735 is pushed now.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> EdwinGrubbs, customers!  Got time for this one?  https://code.launchpad.net/~jtv/launchpad/recife-findtranslationmessage/+merge/34297
<EdwinGrubbs> jtv: I can do it
<jtv> thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> EdwinGrubbs: diff still hasn't showed up.  :(
<EdwinGrubbs> jtv: I ran diff myself against the recife branch.
<jtv> sorry for the trouble!
<gmb> jelmer, EdwinGrubbs: Can either of you gents review https://code.edge.launchpad.net/~gmb/launchpad/prompt-user-to-subscribe-to-master-bug-485627/+merge/34302 for me please?
<EdwinGrubbs> gmb: If jelmer can't do it now, I can do it in an hour or two.
<gmb> EdwinGrubbs, Thanks. I'll add it to the queue.
<jelmer> I'll take it then, just wrapping up a review for noodles775.
<gmb> jelmer, Thanks
* gmb changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> jtv: when makeCurrentTranslationMessage enumerates a list, is it expected that the list contains the plural forms in order of 0, 1, 2, etc.?
<jtv> EdwinGrubbs: yes
<jtv> So a translation of a singular and a plural would be [singular, plural]
<jtv> And a translation to a single-form language, or a translation of a string without a number in it, would be [translation]
<jtv> But in the API we're standardizing on, those are respectively {0: singular, 1: plural} and {0: translation}
<EdwinGrubbs> jtv: findTranslationMessage() still has a potranslations parameter that appears to be unused since you set the potranslations variable in the method.
<gmb> jelmer, I need to step out for a while, so please feel free to ask any questions on the MP and I'll reply when I return.
<jelmer> gmb: *nod*
 * gmb exits stage right
<jtv> EdwinGrubbs: whoopsâwell spotted.  That was an intermediate attempt at solving the multi-format question.
<EdwinGrubbs> jtv: ok, that was my only concern. r=me
<jtv> \o/ thanks
<EdwinGrubbs> jelmer: are you reviewing my branch already? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-597738-bug-service-status/+merge/34250
<noodles775> Thanks for the review jelmer
<jelmer> EdwinGrubbs: Yes, I started on it before you were here but then switched to reviewing noodles'
<EdwinGrubbs> jelmer: ok, I was going to ask if you could do that now. I'll take gmb's branch.
<jelmer> EdwinGrubbs: Oh, ok. That works for me as well.
<jelmer> EdwinGrubbs: btw, it's not really an issue for the review (seems like a formatting issue) but did you see your branch has a conflict?
<EdwinGrubbs> jelmer: I can imagine that since I'm dependent on a branch that may have landed recently with some minor changes.
<EdwinGrubbs> gmb: r=me
<gmb> EdwinGrubbs, Thanks.
<bryceh>  On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bryceh changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bryceh> dur
<leonardr> salgado, can i ask you to take one more look at my branch (http://pastebin.ubuntu.com/486932/). it makes the changes you requested and fixes some test failures, partly by moving more code into the *Public interfaces
<salgado> leonardr, sure
<salgado>        <require
<salgado>            permission="zope.Public"
<salgado> -          interface="canonical.launchpad.interfaces.IOAuthAccessTokenVerification"/>
<salgado> you can use <allow interface="...">  instead of <require permission="..." interface="...">
<leonardr> it's the same when the permission is zope.public?
<salgado> yes
<salgado> what I meant is that
<salgado> you can use <allow interface="...">  instead of <require permission="zope.Public" interface="...">
<salgado> if you want any other permission, then you should use <require>
<leonardr> ok
* jelmer changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: jtv  || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> I don't think EdwinGrubbs is really still reviewing my branch.  :)
* jtv changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: -  || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> ah, ok :-)
<lifeless> jtv: hi
<jtv> hi
<lifeless> jtv: a) go to bed :) b) I had two tiny tests that needed a s/queries/activies/ that i missed
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: -  || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> lifeless: aye-aye.  :-)
<lifeless> :)
<salgado> leonardr, I've asked a few other questions on the m-p, in case you didn't see
<leonardr> salgado, checking
<leonardr> salgado: the permission isn't changing in this diff. the permission changed last week, and i only noticed the test failure today
<leonardr> the permission changed last week because we couldn't come up with a way to make the access level be 'write' for oauthtoken objects and 'read for everything else
<salgado> ok
<leonardr> i needed to remove the security proxy as soon as I changed the permission for the oauth token, but again, i only saw the test failure today
<leonardr> salgado, i've responded on the mp
<EdwinGrubbs> jelmer: I have responded to your review.
<EdwinGrubbs> bryceh: did you want a review of your branch besides the db reviews by stub and lifeless?
<EdwinGrubbs> bryceh: I saw that you put your name in the review queue, but I didn't know if there was a different branch that I'm not seeing that you want reviewed.
<jelmer> EdwinGrubbs, still there?
<EdwinGrubbs> jelmer: yep
<jelmer> EdwinGrubbs: Great, I'll have a look at the updated MP.
<bryceh> EdwinGrubbs, thanks.  The branch is stripped down to just db schema changes, so perhaps is just review of it by stub and lifeless sufficient, or should it receive regular review as well?
<EdwinGrubbs> bryceh: it's fine if they just do the db review, especially since you already had a pre-imp call with somebody from the bugs team. I'll take your name out of the queue, since stub and lifeless aren't oncall reviewers.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bryceh> EdwinGrubbs, ok thanks
<lifeless> actually I am
<lifeless> I think I'm on this morning
 * lifeless checks timetable
* lifeless changed the topic of #launchpad-reviews to: On call: Edwin, Lifeless || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> stub is ocr on monday asiapac fwiw
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Lifeless || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-09-02
<lifeless> bryceh: what was up ?
<lifeless> rockstar/thumper: can I get you to look at a small diff for me: 1 liner .
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/oops/revision/11435
<rockstar> lifeless, ugh, loggerhead.
<lifeless> I loaded it for you already , should be fast
<rockstar> lifeless, why are we making the timeout bigger?
<lifeless> because that number is in milliseconds
<lifeless> in a test
<rockstar> lifeless, why are you only changing the test?
<rockstar> Is this a testfix?
<lifeless> for a branch I'm trying to merge
<lifeless> what happening is that this test, which is independent of all my changes, is breaking
<bryceh> lifeless, mind reviewing https://code.edge.launchpad.net/~bryceharrington/launchpad/lp-617679-db/+merge/34321 when you get a chance?
<lifeless> hmm, I might dig deeper
<lifeless> rockstar: thanks for asking those questions
<lifeless> rockstar: it passes locally
<lifeless> rockstar: which is extremely frustrating
<rockstar> lifeless, yeah, I suspect there's a timing issue somewhere.
<lifeless> what it does
<lifeless> is sleeps for 0.1 second while the time taken is < that the soft reques ttimeout
<lifeless> the failure is in
<lifeless> the report() below - it shows a soft timeout
<lifeless> but its expecting a hard timeout not a soft one, presumably because the soft timeout is higher than the hard one
<lifeless> rockstar: if its ok with you, I'll try this change
<lifeless> rockstar: and if it works chalk it up to terrible-testing-approaches
<lifeless> rockstar: if it doesn't I'll dig deep
<rockstar> lifeless, I defer to your judgment then.
<lifeless> rockstar: thanks!
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/bug-618372/+merge/34383
* lifeless 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
<lifeless> ^ that branch is a 2 liner; if anyone wants to eyeball it
<mwhudson> lifeless: done
<lifeless> thanks
<lifeless> I have another
<lifeless> but I'll do that after dinner
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi noodles775, i have a MP for review: https://code.edge.launchpad.net/~bac/launchpad/bug-154587/+merge/34401 .  i need to go away for a bit to eat breakfast, though.
<jtv> noodles775, would you mind this one on your queue while I'm already EOD?  https://code.launchpad.net/~jtv/launchpad/recife-diverge/+merge/34407
<noodles775> bac, jtv: sure
<jtv> noodles775: fantastic, thanks
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: bac  || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> noodles775: https://code.edge.launchpad.net/~wgrant/launchpad/ppa-key-name/+merge/34408, if you have a few minutes.
<noodles775> wgrant: oooh, nice easy one :)
<wgrant> Indeed.
<noodles775> wgrant: approved and ec2 landing.
<wgrant> noodles775: Thanks.
<StevenK> I still maintain that wgrant should use ec2 test. :-P
<noodles775> With a paid-for account?
<wgrant> StevenK: I can use it... it just can't land.
<wgrant> noodles775: And it wasn't really formally reviewed before.
<StevenK> wgrant: 'ec2 test'
<noodles775> wgrant: I know, I still treated it as if unreviewed :)
<wgrant> StevenK: Hmmm?
<StevenK> wgrant: 'ec2 test' runs the tests and mails you the results, 'ec2 land' runs the tests, and lands it if the tests passed
<wgrant> StevenK: Yes, but there's not much point me running ec2 test.
<StevenK> wgrant: IE, you can quite happily run 'ec2 test' and then ask someone to submit it to PQM, but meh
<wgrant> People generally won't.
<StevenK> wgrant: It isn't fair when you respond to my teasing with rationale.
<wgrant> :(
* noodles775 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> salgado, take a look at http://pastebin.ubuntu.com/487261/ and hopefully we can wrap up this branch
<salgado> leonardr, looks fine to me
* rockstar changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> deryck: can you do a ui review of my branch modifying bugs.lp.net/$project/+index? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-597738-bug-service-status/+merge/34250
<deryck> EdwinGrubbs, yes, was just about to look at in fact.  However, I'm not a UI reviewer or UI reviewer mentee.
<EdwinGrubbs> deryck: oh, I misread the reviewer wiki page.  If you're interested, I'm open to feedback, but don't feel like I'm waiting on you for the review.
<EdwinGrubbs> noodles775, rockstar: can one of you do a UI review for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-597738-bug-service-status/+merge/34250
<rockstar> EdwinGrubbs, I'll take a look.
<EdwinGrubbs> thanks
<deryck> EdwinGrubbs, I'll just glance at the page since it's bugs related
<deryck> and leave ui to rockstar :-)
<noodles775> Thanks rockstar. Might be worth seeing if henninge wants a first look too (although he's probably EODing soon like me).
<rockstar> Oh, I thought he would have EOD'd.
<henninge> rockstar: I can have a look.
<rockstar> henninge, oh yay! Yeah, take a look.
<rockstar> EdwinGrubbs, what? No screenshot?!  :)
<henninge> Yes, I am a bit disappointed myself ... ;)
 * henninge just branched
<henninge> rocketfuel-get ...
<allenap> rockstar: Could you review a snippet for me please? It's a workaround for bug 628762 so I can get lp:~allenap/launchpad/cache-experiment-roll-out landed. http://paste.ubuntu.com/487315/
<_mup_> Bug #628762: propertycache adaption failures in test suite <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/628762>
<allenap> rockstar: The merge proposal for the whole branch is at https://code.edge.launchpad.net/~allenap/launchpad/cache-experiment-roll-out/+merge/33542
<rockstar> allenap, I suspect I need to look at the whole branch.
<allenap> rockstar: The whole branch is pretty huge; there are a lot of mechanical changes. I think you'll be able to understand it if you look at propertycache.py, propertycache.txt and bug 628762.
<_mup_> Bug #628762: propertycache adaption failures in test suite <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/628762>
<rockstar> allenap, I'd be happier of you had lifeless review that.
<allenap> rockstar: Okay.
<allenap> rockstar: Can I put myself in the queue for a couple of tarmac branches then? One, lp:~allenap/tarmac/votes-needed-plugin, is for the benefit of Landscape (though it's generally useful), and the other is just a clean up.
<rockstar> allenap, I'm happy to take some Tarmac branches, yes.
<allenap> rockstar: Cool, thanks.
* allenap changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: -  || queue: [allenap, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: -  || queue: [allenap, allenap, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> rockstar: when you're able to get to it, here's the MP. https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-remaining-official-bools/+merge/34436
<henninge> rockstar: done
<henninge> *now* I am EOD
<rockstar> henninge, have a good night!
<henninge> thanks ;)
<rockstar> jcsackett, it looks like your branch has conflicts.
<jcsackett> rockstar: blast, you're right, i forgot to merge in devel.
<jcsackett> one sec, sorry for the bother.
<jcsackett> rockstar: should be pushed up momentarily.
<rockstar> allenap, didn't you already have a thing class?
* rockstar changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, could you please review https://code.launchpad.net/~abentley/launchpad/recipe-interfaces/+merge/34453 ?
* abentley changed the topic of #launchpad-reviews to:  On Call: rockstar || reviewing: jcsackett || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, could you please review https://code.launchpad.net/~abentley/launchpad/permit-commands/+merge/34456 ?
* abentley changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: jcsackett || queue: [abentley, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> abentley, on it.
<abentley> rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/safe-v3/+merge/34465 ?
<rockstar> abentley, add it to the backlog and I'll get to it.
* abentley changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: jcsackett || queue: [abentley, abentley, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, take a wild guess what's in this diff: http://pastebin.ubuntu.com/487423/
<rockstar> abentley, these branches were all pipes, correct?
<salgado> leonardr, heh.  seems fine to me
<abentley> rockstar, yes, but not all the same pipeline.  The last one is in a pipeline with the build-outside-chroot one.
<rockstar> abentley, yeah, I think there might have been a hiccup in setting a dependent branch somewhere.
<abentley> rockstar, something wrong?
<rockstar> Was permit commands related to the recipe-interfaces pipe?
<abentley> rockstar, yes.
<rockstar> abentley, I think the dependent branch didn't get set right on permit-commands
<abentley> rockstar, I think you're right.
<rockstar> abentley, it would be great if you could fix it.
* rockstar changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, I can't fix it without creating a new merge proposal.
<rockstar> abentley, would you mind?  I assume this diff is much smaller that way.
<abentley> rockstar, ah.  I thought you'd already reviewed that one.  sure.
<abentley> rockstar, corrected version: https://code.edge.launchpad.net/~abentley/launchpad/permit-commands/+merge/34476
<rockstar> abentley, thanks
<abentley>  rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/allow-0.3/+merge/34478 ?
<rockstar> abentley, you're an ANIMAL!
* abentley changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: abentley || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, they're all small.
<jcsackett> rockstar: branch was updated per your remarks.
<rockstar> jcsackett, great, I'll take a look.
<jcsackett> thanks.
* rockstar 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
<thumper> mwhudson: feel like a review?
<thumper> mwhudson: it is a codehosting type xmlrpc one
<thumper> just your cuppa tea
#launchpad-reviews 2010-09-03
<thumper> mwhudson: please?
<lifeless> hmm, friday has no asiapac ocr
<lifeless> thumper: why aren't you in the ocr rotation ?
<thumper> lifeless: because it was decided a while back that team leads are exempt
<thumper> :)
<lifeless> mmm
<thumper> 'cause we're busy
<lifeless> self inflicted
<lifeless> :P
<thumper> :P
<thumper> ââ¹
<lifeless> o/~
<mwhudson> thumper: hi, sure
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/xmlrpc-lp-name-resolution/+merge/34500
<thumper> mwhudson: thanks
<mwhudson> why are code review diffs in proportional fonts at the moment?
<lifeless> !
<lifeless> probably the ubuntu beta font changes
<mwhudson> i don't have the font installed
<mwhudson> i guess that might be it
<lifeless> file a bug
<mwhudson> ok
<mwhudson> launchpad-code?
<lifeless> mwhudson: can I borrow your brain for a minute
<lifeless> mwhudson: launchpad-web perhaps
<lifeless> mwhudson: xmlrpc timeout handling is either broken, or terribly confusing, or both
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/short-lp-name-for-private-branches/+merge/34509 is the last part
<mwhudson> lifeless: xmlrpc timeouts _ought_ to be the same as everything else
<mwhudson> i guess it's probably a separate config
<lifeless> mwhudson: do you have, oh 60 seconds ?
<mwhudson> lifeless: yeah
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/oops/revision/11435
<lifeless> incremental thing
<lifeless> should be pretty self-explanatory
<lifeless> this actually fixes the bug 'statements that hit the timeout policy are not in the statement log'
<mwhudson> lifeless: looks good to me
<lifeless> thanks!
<mwhudson> thumper: sorry for the delay
<mwhudson> thumper: the assertResolves docstring has become a bit mangled
<thumper> mwhudson: nm
<thumper> I'm running the tests through ec2
<thumper> and about to run and get more coffee beans
<thumper> we have the wine for tonight :)
<thumper> mwhudson: does the docstring need some attention
<thumper> ?
<thumper> I should take a look I guess
<thumper> oversight that I've missed that
<mwhudson> thumper: yeah, the summary line mentions arguments its no longer parsed and the next line doesn't make sense at the grammar level
<mwhudson> sorry, not the next line
<mwhudson>         :param lp_path: The path the short lp alias path for the branch.
<lifeless> maybe a matcher would help?
 * thumper is back with coffee
<thumper> mwhudson: are you able to finish the reviews off?
<thumper> mwhudson: these last two branches finish off something for our stakeholders :)
<mwhudson> thumper: back now
<mwhudson> thumper: one down
<mwhudson> thumper: two down
<thumper> mwhudson: you approved the proposal without approving the review (must fix that) https://code.edge.launchpad.net/~thumper/launchpad/short-lp-name-for-private-branches/+merge/34509
<lifeless> thumper: fix it? just make it automatic :)
<thumper> lifeless: yes... I know
<thumper> lifeless: it isn't fixed yet
<lifeless> thumper: kk, had a scary moment where I thought you meant 'reject approval if there is no 'approve' review.
<thumper> what?
<mwhudson> oh ok
<thumper> we have an implicit review approval through email
<lifeless> thumper: I bound 'fix it' to the wrong action
<thumper> it just doesn't do it through the web (yet)
<mwhudson> thumper: done
<thumper> mwhudson: thanks
<lifeless> thumper: nice, sounds great
* henninge changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [henninge,henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Good morning jtv!
<jtv> hi henninge!  Forgot to turn on pidgin today
<jtv> Nobody on call?  :(
<jtv> On Call: - || reviewing: - || queue: [henninge,henninge,jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: no Abel yet, no ... ;(
<henninge> jtv: let's trade reviews, then.
<jtv> henninge: OK!
<jtv> Which one do you want reviewed first?
<henninge> jtv: sampledata depends on the other one.
* jtv changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> OK
<henninge> jtv: thank you
<jtv> Mine: https://code.edge.launchpad.net/~jtv/launchpad/bug-618393/+merge/34515
<henninge> jtv: mine did not stay under the limit like yours does ... Sorry
<henninge> Claimed it already ;)
<jtv> henninge: your cover letter looks like a good readâI'm almost sorry that it goes to me who already knows the background!
<henninge> I know, such a waste ....
<henninge> (just kidding)
<henninge> jtv: that's ok, I like to do that anyway to wrap up a branch for myself.
<jtv> I agree.  There's also the historical record, and just the mental step of taking a step back and explaining.  So I'm _almost_ sorry.  :)
<jtv> henninge: about the statistics checks you removed from translationmessage-views.txt: I think that wasn't so much a complicated way of testing that the submission succeeded, as a misplaced test for the statistics.  An example of two ancient ills of our doctests: testing low-level things in too much detail (basically unit-testing in integration tests) and throwing different tests into the same scenario because it's the scenario, not t
<henninge> (truncated)
<jtv> An example of two ancient ills of our doctests:
<jtv> testing low-level things in too much detail (basically unit-testing in integration tests)
<jtv> and throwing different tests into the same scenario because it's the scenario, not the functionality, that dictates the structure of the test.
<henninge> Well, my other thought was "statistics are tested elsewhere, no need to that here".
<henninge> jtv: we'll be loosing codehosting in an hour, btw
<jtv> Oh!  Thanks for pointing that out.
<jtv> Saw the email and didn't pay enough attention to it.
<jtv> henninge: I think you did a good thing in removing this, and hopefully we can gradually cut down on doctests until we can actually figure out what they do and remove them completely.  :)  Please don't make the narrative say things like "Let's submit a translation" though.
<henninge> sorry
<henninge> I don't like that either, actually, but I was keeping in tone with the test as it was ... ;-)
<henninge> 26	-It's time to check the submission of translations and the IPOFile statistics
<henninge> 27	-update.
<henninge> 28	-
<henninge> 29	-But first, let's see current values.
<jtv> Yes, it's horrible.
<jtv> hi noodles775 btwâthanks for your review!  Raises some difficult questions.
<noodles775> jtv: no problem... And yes, I found it hard :/
<jtv> There's a lot of complex background knowledge and intertwined data modeling there, and we've been reviewing a lot of this stuff among ourselves.
<jtv> So perhaps a reminder to me that that is not always the case.  :-)
<jtv> I was a bit surprised that you got an impression of strong emotions from some of the comments in the code.  Wasn't meant like that at allâsaying "this record is in the way" just seemed like a good description of the situation.  Is there anything there that you'd like me to change?
<StevenK> thumper: I think you're EOW, but changes pushed for db-add-ifp-job.
<jtv> If thumper is having his weekend now, maybe that's a good time to look at my wikkid MP.  :)
<noodles775> jtv: no, not at all, I was just trying to be funny with that comment.
<jtv> ah, phew!  :)
<jtv> I misread your facial expression then
<noodles775> Well yes, funny and sympathetic at the same time.
<jtv> (emoticons are awesomeâ¦ who needs a webcam for facial expressions)
<jtv> noodles775: I'm engaged in mutual reviews with Henning right now, but will get back to it soon
<noodles775> Sure.
<jtv> henninge: were we figuring out "this is a local suggestion" based on the pofile attribute rather than on which method call came up with the suggestion?
<henninge> jtv: yes
<jtv> may be a leftover from the caching we had before sharingâ¦ but glad to be rid of it!
<jtv> I see you disabled unmasking in gettext-check-messages, but did not delete the code.  Why is that?
 * jtv remembers to check back with the excellent cover letter
<henninge> I thought I explained that ... ;)
<henninge> jtv: We have a card to revisit that script and maybe we can come up with a way to re-enable it.
<jtv> henninge: I appreciate the sentiment, but keeping commented-out code around is not usually a good idea.  We have bzr to remember the old code for us!
<jtv> henninge: I see now that the change you made is really quite minimal.  That's nice in some ways, but if it leaves code dead, better to clean it up now.
<jtv> Future work on the script will be easier if all the code that's in there actually works, and the "how did we use to do this" code is available separately in bzr history.
<jtv> henninge_: you're blinking
<jtv> henninge: daily router nap?
<henninge> daily reconnect
<henninge> yup
<jtv> (14:37:01) jtv: henninge: I see now that the change you made is really quite minimal.  That's nice in some ways, but if it leaves code dead, better to clean it up now.
<jtv> (14:37:32) jtv: Future work on the script will be easier if all the code that's in there actually works, and the "how did we use to do this" code is available separately in bzr history.
<henninge> I have to move that out of my working hours again.
<jtv> I'm sure you can whip something up with "at" and "wget."  :-)
<henninge> jtv: I turn my computer off when I leave work ...
<jtv> then leave it on for once, and set a shutdown time right after the "at" time!
<jtv> hey danilos
<danilos> jtv, hey hey
<henninge> Hi danilos! ;)
<henninge> jtv:
<henninge> 133	+        self._licenses = tuple([
<henninge> 134	+            License.items[id] for id in sorted(license_ids)])
<henninge> henninge:
* adeuring changed the topic of #launchpad-reviews to: On Call: - || reviewing: adeuring || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> hi adeuring!
<adeuring> hi jtv
<jtv> How's the weather in your neck of the woods?
<adeuring> relatively cold and rainy
<jtv> here too, but according to the Gnome weather applet the humidity still makes it _feel_ like 39.1Â°
 * adeuring should try that too
<adeuring> is there also an option to feel snow during winter?
<jtv> adeuring: not where I am
<adeuring> henninge: which branch do you want reviewed?
<jtv> adeuring: I'm working on one; the other depends on it
<adeuring> jtv: ah, thanks!
<henninge> adeuring: the sampledata branch is a simple sampledata change with many rows.
<henninge> adeuring: I am not even sure it needs a review, though
<adeuring> henninge: well, rs=me on the sampledata change
<henninge> adeuring: thanks!
* henninge changed the topic of #launchpad-reviews to: On Call: - || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: The docstring for ensureBrowserPOFile is a bit ambivalent: it "ensures" and "makes sure" that the browser_pofile is setâ¦ "if possible."  Which is it?
<jtv> The only scenario I can imagine it coming up empty is where all templates that contained a potmsgset have been deleted.
<jtv> Manually.
<jtv> So maybe we simply shouldn't delete POTemplates without deleting orphaned potmsgsets.
<henninge> jtv: See, I was not sure about that ... ;)
<jtv> OTOH over a third of our potmsgsets seem to be orphansâ¦
<jtv> Maybe it's time to clean up some more there!
<henninge> jtv: if they are orphans they won't come up in browser code, will they?
<jtv> external suggestions.
<jtv> henninge: they can still show up as external suggestions.
<henninge> jtv: yes, I understand. I think that is the case where this used.
<henninge> jtv: I think they should not be used for external suggestions, though.
<henninge> So it will take an extra check to see if a browser_pofile could be found before displaying it.
<jtv> henninge: yes, for the immediate term I think that'd solve it.
<jtv> However it also means that 17% of our TMs become completely and utterly useless, even as external suggestionsâso we should probably clean them up.
<jtv> Minus the "probably."  :-)
<henninge> s/probably/definitly/
<henninge> ;-)
<henninge> jtv: store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
<henninge> Don't we have IStore now to replace that?
<jtv> henninge: err yes, that'd be ISlaveStore.
<henninge> jtv: I search and search but cannot really find any problems with your branch ...
<jtv> henninge: try harder!  :-)
 * henninge gives it one more go 
<jtv> Punish me babny!
<jtv> (Sorry, it's hard to type correctly in the heat of passion)
<henninge> jtv: although you did not introduce it, can you please fix this:
<henninge> 59	         # Prefill pillar.product.licenses.
<henninge> 60	-        for pillar_name, other, product, project, distro, license_ids in (
<henninge> 61	+        for pillar_name, other, product, project, distro, licenses in (
<henninge> 62	             result[:limit]):
<henninge> line 62
<jtv> I looked at it briefly but couldn't think of anything that'd make it fit.  :)
<jtv> I'll look again.
<jtv> Grr circular import somewhere
<henninge> jtv: you could pull the de-tupling into the loop...
<henninge> jtv: Where?
<jtv> Ah yes good
<jtv> Not sure yet looks ugly
<jtv> it says shipit
<jtv> henninge: would it be convenient if ensureBrowserPOFile also returned self.browser_pofile?
<henninge> no, I don't think so.
<jtv> Well you've got a few cases that could benefit: ll.104 & 123 in the diff.
<jtv> In other words, all call sites.  :)
<henninge> jtv: I am fine with that but it should still set browser_pofile because that may be used later in the code.
<jtv> henninge: yes, hence the "also"  :-)
<henninge> jtv: review sent, r=me
<jtv> henninge: cool, thanks!
<jtv> Still trying to figure out what's wrong with the import error that seems to have nothing to do with me.
<henninge> jtv: which branch? Mine or yours?
<jtv> mine
<jtv> But another one for you: in getTranslationMessages, the Coalesce line-wraps a bit awkwardly.  How about putting it into a variable called e.g. applicable_template?
<jtv> Then the query could just say "applicable_template == self.potemplate.id"
<henninge> good idea
<jtv> The really nice side effect is that the variable name introduces documentation without needing comment.
<henninge> jtv: Will I see all you suggestions in the review or should I do them right now?
<jtv> henninge: either way's fine for me, as long as I know that I'm not waiting for you to implement them before I go on to the next one.  :)
<jtv> So I'll move on.
<henninge> jtv: I prefer waiting for the review and then do them all in one go.
<jtv> In updateTranslation you pass pofile=None to the TranslationMessage constructor.  It's a bit moot with the Recife changes coming up, but why not remove that argument altogether?
<jtv> Oh, OK
<henninge> jtv: I did not want to mess with the interface. But is pofile not used elsewhere in updateTranslation?
<jtv> The argument, not the parameter.
<jtv> I mean: there should be no need for the call site to pass pofile=None to the TranslationMessage constructor.
<jtv> Should be the same as not passing pofile there at all.
<henninge> that's fine
* bac changed the topic of #launchpad-reviews to: On Call: - || reviewing: adeuring, bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> good morning adeuring
<adeuring> hi bac!
<bac> adeuring: busy day?
<adeuring> bac: not at all :)
* adeuring changed the topic of #launchpad-reviews to:  On Call: adeuring, bac || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: review sent.
<jtv> henninge: I've not received any review
 * henninge checks
<jtv> Also, for all those who read German: http://www.nichtlustig.de/toondb/100901.html
<henninge> jtv: hm, I have it in "Sent" ...
<henninge> jtv: and I have received yours
<jtv> henninge: I do not have yours in "Received," nor has the page updated.
<henninge> I can resent it
<henninge> resend
<jtv> Maybe just use the web UI?
<henninge> Well, it worked the other day.
<henninge> jtv: done
<henninge> I will go to lunch now.
<jtv> henninge: and page updated.  Thanks!
<jtv> noodles775: getting back to your review nowâ¦ I've implemented the smaller suggestions already, but now the same shipit problem is hitting me.  :(
<bac> henninge: thanks for the sampledata update
<noodles775> jtv: I thought you mentioned re-merging devel brought the fix in?
<jtv> noodles775: devel, yes.  But this is a long-lived feature branch based off db-stable.  :(
<noodles775> Ah, right. But that shouldn't effect whether the review continues should it?
<noodles775> (and won't you guys need to bring the same fix in anyway? either when it hits db-stable or manually?)
<noodles775> jtv: actually, it might be easier to wait until the fix hits db-stable and just merge. That'll give me a chance to try to get my own fix in before pqm closes too :)
<jtv> noodles775: yes, I think so too
<jtv> After all I can't easily test whether a particular change I make during the review is good.  I'm basically blocked on this.
<jtv> We'll have to merge a fresh db-stable eventually.
<jtv> We do that periodically
<matsubara> adeuring, Hi Abel, could you review https://code.edge.launchpad.net/~matsubara/launchpad/update-ec2-merge-workflow/+merge/34486 for me please?
<adeuring> matsubara: sure
<matsubara> thanks adeuring
<jtv> bac: very small one for review: https://code.launchpad.net/~jtv/launchpad/bug-629442/+merge/34539
<bac> jtv: ok
<jtv> bac: thx
* bac changed the topic of #launchpad-reviews to:  On Call: adeuring, bac || reviewing: -, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> bac: grrr slow diffâI'll poll for it to appear and then let you know
<bac> jtv: no bother
<jtv> bac: we have diff
<jtv> repeat: we have diff
<bac> thank you, houston
<adeuring> matsubara: in lines 48..55 of the diff: if both (--no-qa or --incr) and --rollback=... are defined, the former options are ignored. Should we issue are warning in this case?
<matsubara> adeuring, you mean like raising an exception if --no-qa=True and -- incr=True and --rollback=123?
<matsubara> adeuring, sounds like a good idea, yes
<adeuring> matsubara: I don't know if this is worth an exception, just printing a user warning might be enough. But I leave decision to you ;)
<bac> jtv: lovely, r=bac
<jtv> thanks bac!
* bac changed the topic of #launchpad-reviews to:  On Call: adeuring, bac || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<matsubara> adeuring, ok, I'll go with the exception as this is the same pattern used by the other options.
<adeuring> matsubara: OK
<adeuring> matsubara: in line 109, you catch a TypeError, probably raised in line 55, or somewhere in the option parser. But TypeErrors are quite generic -- could you move the "try: ... except TypeError:" directly to the place(s) where it occurs and raise the BzrComandError there? TypeError are so generic, and catching them not very close to the code where they are expected can hide completely unrelated errors.
<matsubara> adeuring, just a sec, I'm on the stand up call
<matsubara> adeuring, moving BzrCommandError to autoland.py wouldn't make much sense. I just tried something else instead and I think I can remove the exception handling
<adeuring> matsubara: ok
<matsubara> adeuring, since 67..71 I'm defining the --rollback option as int, if I pass something else like --rollback=foo, the option parser will return me a nice error message
<matsubara> so, I'll remove the exception handling from there as it's unecessary in that case.
<adeuring> matsubara: right, I wandered too if it was even necessary. (BTW, What happens for a missing parameter value)
<matsubara> adeuring, this https://pastebin.canonical.com/36722/
<adeuring> matsubara: so, that's an optparse issue -- let's not worry about it. I was just curious if this might cause the type error in 'rolback=%d'' % rollbak
<matsubara> adeuring, yep, I was being extra careful catching that TypeError as I thought 'rollback=%d'' % rollback' would raise that, but then optparse will take care of that for us.
<adeuring> exaxtly
<adeuring> matsubara: so, with "except TypeError:" removed and the other change about the options --no-qa and --rollback, r=me
<matsubara> adeuring, I will push the branch with your review comments in a moment. thanks for the review!
<danilos> adeuring, bac: got some time for a quick review? https://code.edge.launchpad.net/~danilo/launchpad/bug-548375/+merge/34544
<bac> danilos: yep!
<adeuring> danilos: sure
* bac changed the topic of #launchpad-reviews to:  On Call: adeuring, bac || reviewing: -, danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> adeuring: i beat you to it
<danilos> heh, I'll let you guys fight it over, though bac was first ;)
<adeuring> bac: ok ;)
<danilos> bac, oh, I just noticed there's one typo in the test (line 23): is_published=True should be is_published=is_published (use the passed in value: forgotten change after the unification of two tests)
<bac> danilos: ok
* noodles775 changed the topic of #launchpad-reviews to:  On Call: adeuring, bac || reviewing: -, danilos || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> bac, most of the changes are drive-by or lint fixes, basically, just the tests and 3 lines I mention are specific to this bug fix; entire test_translation_importer.py is drive-by cleanups
<bac> danilos: ok
<noodles775> Hi adeuring, bac. When you've time, could one of you please look at: https://code.edge.launchpad.net/~michael.nelson/launchpad/628711-generate-ppa-htacess-too-slow-try-2/+merge/34546
<adeuring> noodles775: sure
<noodles775> Thanks adeuring
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring, bac || reviewing: noodles, danilos || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On Call: adeuring, bac || reviewing: noodles, danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> :)
<bac> danilos: done.  thanks.
* bac changed the topic of #launchpad-reviews to: On Call: adeuring, bac || reviewing: noodles, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> bac, cheers
<jelmer> hi adeuring, bac
<bac> hi jelmer
<jelmer> Can I add another Soyuz branch to the queue?
<jelmer> It's at https://code.edge.launchpad.net/~jelmer/launchpad/506256-remove-popen/+merge/34549
<adeuring> bac: could you take the review? I'm busy with noodles's review and some other issues
<bac> adeuring: sure
<bac> jelmer: sure
<noodles775> adeuring: is it worth me sending my branch off to ec2 test (not to land) or can you already see changes needed?
<noodles775> (so I can get in before pqm closes)
<bac> jelmer: i just reviewed your tiny branch.  will start on the other one now.
<jelmer> bac: thanks
<bac> jelmer: i get this:
<bac>     from testtools.content_type import UTF8_TEXT
<bac> ImportError: cannot import name UTF8_TEXT
<bac> jelmer:  is this a new dependency?
<jelmer> bac, I've seen that before as well, but it's not from my branch
<jelmer> bac: have you updated sourcedeps recently?
<bac> this morning
<bac> make tea, shave, update rocketfuel.
<bac> ok, so it's not that routine but i did update this a.m.
<jelmer> hmm
<bac> trying your tests on devel
<bac> jelmer: thinking about your LPCONFIG change....perhaps you should run it by the LOSAs, just so they will know about the change
<bac> and see if they have comments.  it does affect the way they start things
<jcsackett> bac: i got that error once (UTF8_TEXT) post-update and make clean && make build fixed it.
<bac> jcsackett: that's what i'm doing now.  good to know.
<jelmer> bac: That's a good idea - thanks
<bac> jelmer: it's not good to surprises the losas...
<adeuring> noodles775: sorry, I was somewhat distracted by other issues... In exportSubscriptions(), can't you do the equivalent of all_activre_tokens.difference(...) in SQL?
<noodles775> adeuring: I'm not sure what you're asking? That is being executed as one SQL statement?
<adeuring> noodles775: argh, yes!
<adeuring> sorry for the noise
<noodles775> np! Always good to ask :)
<bac> jelmer: you mention a failing test. is it ./bin/test lp.buildmaster ?  I'm getting ForbiddenAttribute, among other failures
<jelmer> bac: no, it should be in lp.archiveuploader
<jelmer> "./bin/test lp.buildmaster" is passing for me here - what's the exact error you're getting?
<adeuring> noodles775: seems that I am constantlxy distracted by some other (urgent) issues -- would you mind if I put your review back into the queue?
<noodles775> adeuring: no, I understand. Hopefully bac will have some head-space :)
<adeuring> noodles775: thanks!
* adeuring changed the topic of #launchpad-reviews to: On Call:  bac || reviewing: -, - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> bac: when you're free, it's this one (which I'm hoping to get in before pqm closes ;)): https://code.edge.launchpad.net/~michael.nelson/launchpad/628711-generate-ppa-htacess-too-slow-try-2/+merge/34546
<bac> noodles775: i'll have a look when i get done with jelmer's
<noodles775> Thanks bac
<noodles775> bac: actually, I've just found an issue with my branch while doing some QA, so please don't start it until I ping you again :)
<bac> noodles775: ok
<bac> brb
<henninge> gmb: still around?
<gmb> henninge, Yes
<henninge> gmb: what do I need to do for a pre-emptive r-c? Add you for an r-c review?
<gmb> henninge, Yes please.
* gmb changed the topic of #launchpad-reviews to: On Call:  bac || reviewing: -, - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM for RC reviews please add him as a reviewer
* gmb changed the topic of #launchpad-reviews to: On Call:  bac || reviewing: -, - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<gmb> </punctuationpendantry>
<henninge> gmb: https://code.edge.launchpad.net/~henninge/launchpad/devel-bug-597359-sampledata/+merge/34444
 * gmb looks
<henninge> gmb: I am waiting for the prerequisite branch to make it through ec2 but I am not sure that I can watch it closely enough tonight to get the timing right. ;-)
<bac> jelmer: here are the test failures i'm seeing: http://pastebin.ubuntu.com/487888/
<jelmer> bac: the testNoFiles one is what I'm aware of, I'll try to reproduce the other two.
<gmb> henninge, rc=me
<henninge> gmb: thanks!
<noodles775> bac: Mine is ready to go now (I've push the small change and already QA'd it on dogfood)
<bac> noodles775: is 11492 the latest?
<bac> that's what the MP shows
<noodles775> bac: 11493 is what I've push 10mins ago.
<bac> noodles775: ok, i'll wait
<noodles775> bac: its displaying for me?
<bac> jelmer & noodles775: i'm working to get your reviews done.  unfortunately i have to leave in 1 hour
<noodles775> gmb: I've added you for a pre-emptive r-c to https://code.edge.launchpad.net/~michael.nelson/launchpad/628711-generate-ppa-htacess-too-slow-try-2/+merge/34546
<bac> noodles775: see it now
<noodles775> (if you've time)
<noodles775> Thanks bac
<gmb> noodles775, Looking now.
<bac> noodles775: my gem today is defaultdict.  thanks!
<noodles775> :)
<bac> noodles775: looks good to me.  r=bac
<noodles775> Thanks bac
* bac changed the topic of #launchpad-reviews to: On Call:  bac || reviewing: jelmer || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<noodles775> gmb: I need to run, but I'll be checking back later to land this branch. I've just updated the related bug with the QA on dogfood. Thanks!
<gmb> noodles775, np
<bac> jelmer how's it coming?
 * bac finds quick lunch
<jelmer> bac: So, I can reproduce the errors locally but I don't see how I didn't get them on df
<jelmer> bac: I'm still investigating
<bac> jelmer: ok
<bac> jelmer: i've got about 30 minutes left.  EdwinGrubbs has agreed to cover last minute submissions after that.
<jelmer> bac: ok, thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: On Call:  bac, Edwin || reviewing: jelmer || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
* sinzui changed the topic of #launchpad-reviews to: On Call:  bac, Edwin || reviewing: jelmer || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<sinzui> EdwinGrubbs, , bac: I have a oops fix for answers: https://code.edge.launchpad.net/~sinzui/launchpad/question-title-0/+merge/34555 <- I made that in the UI because my email does not seem to have arrived.
<EdwinGrubbs> sinzui: I'll take it.
* bac changed the topic of #launchpad-reviews to: On Call:  Edwin || reviewing: sinzui || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<bac> jelmer: i need to dash.
<jelmer> bac: Ok
<bac> EdwinGrubbs: i owe you a Lone Star in dallas.
<bac> maybe a pearl
<jelmer> bac: Thanks for having a look.
<EdwinGrubbs> sinzui: r=me
<sinzui> EdwinGrubbs, thanks
#launchpad-reviews 2010-09-04
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/34600] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<mwhudson> lifeless: did you think about
<mwhudson> 278	+ request = get_current_browser_request()
<mwhudson> 279	+ timeline = get_request_timeline(request)
<mwhudson> a helper for that ^ ?
<lifeless> yes
<lifeless> I felt it coupled things a little to much
<lifeless> really we want the IAnnotations context thing
<mwhudson> ah yeah
<lifeless> this is at least clear that the context is 'a request' (for now)
<lifeless> and if you have one, just passing it in and using it is ok
<mwhudson> yeah, fair enough
<mwhudson> another general comment: make actions context managers?
<lifeless> definitely not
<lifeless> you could make timeline a context manager
<lifeless> and timedaction a context
<lifeless> (__enter__ on timeline, returns a new TimedAction, __exit__ on TimedAction)
<lifeless> the definitely not is because
<lifeless> with action:
<lifeless>  ...
<lifeless> with action:
<lifeless>  ...
<lifeless> would not have any hope of DTRT cleanly
<lifeless> with timeline as action:
<lifeless>  ...
<lifeless> would
<lifeless> or
<mwhudson> yeah, i guess that would be better
<lifeless> with timeline.start('foo', 'bar'):
<lifeless>  ...
<lifeless> is probably what would work well.
<lifeless> we can revisit and do this, I think its a reasonable train of thought
<lifeless> but - for SQL, due to the way tracers are done, certainly not complete.
<lifeless> and the first cut of this was to replace request_statements
<mwhudson> lifeless: branch reviewed
<lifeless> thank you
<mwhudson> lifeless: it's clear you've thought about this much more than me, so i'm happy :-)
<mwhudson> heh, stupid braino in the review comment
<mwhudson> oh well
<mwhudson> s/database/librarian/
<lifeless> database almost matter
<lifeless> I've answer there for posterity
<lifeless> login() should really be 'start_request()'
<lifeless> given what it does
<lifeless> or something
* lifeless 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 || gmb is RM; for RC reviews please add him as a reviewer
<mwhudson> set_participation() would be more confusing but would encourage understanding of what it does
<lifeless> true
<mwhudson> personally, i prefer calling it a participation in general and a request in the appserver, but this idea may not have caught on yet
