[05:59] <lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/32953
[05:59] <lifeless> jtv: ^
[05:59] <jtv> lifeless: I take it this is a review request?
[05:59] <lifeless> yes please
[06:01] <jtv> lifeless: every time someone opens a new communication with the word "So," I'm tempted to replace it with "Therefore."
[06:03] <jtv> lifeless: I wasn't aware of ValidPersonCache.  We used to have some database views for optimization of translation exports, and we got a tremendous speedup by getting rid of them.
[06:06] <lifeless> jtv: yeah
[06:06] <lifeless> I wasn't aware of it either
[06:06] <lifeless> I think it came with the openid stuff, or something
[06:06] <jtv> lifeless: what are the "# --" comments?
[06:06] <jtv> ah, I see now
[06:07] <lifeless> clarity in a long function which is simple-but-extended
[06:08] <mwhudson> i think that validpersoncache was actually a cache at some point
[06:09] <mwhudson> but that changed, maybe during the account/person split
[06:09]  * mwhudson may very well be wrong, of course
[06:10] <jtv> lifeless: I can't say I care much for the mixed style of passing arguments: chucking the first one in right after the opening parentheses but then tab-indenting the following ones.  Could you align the first argument with the following ones in line 37 of the diff?  I'd also appreciate a matching change in line 52.
[06:11] <jtv> conditions = AND(
[06:11] <jtv>     conditions,
[06:11] <jtv>     Or(...
[06:11] <jtv> (I gather the mixing of SQLObject AND and Storm Or is harmless then)
[06:12] <lifeless> the AND is a mistake
[06:12] <lifeless> I'll change it
[06:12] <lifeless> the indenting shows grouping, I think its much less clear without it
[06:12] <lifeless> aim for readability
[06:14] <jtv> That's just the thing.  I think this is more readable, not less:
[06:15] <jtv> conditions = And(
[06:15] <jtv>     conditions,
[06:15] <jtv>     Or(
[06:15] <jtv>         Account.status == None,
[06:15] <jtv>         Account.status == AccountStatus.ACTIVE))
[06:15] <lifeless> I can do that
[06:15] <lifeless> that wasn't what you seemed to be asking for
[06:18] <jtv> Well I figured you'd find out soon enough that you couldn't take the first argument to the left, and so either had to go to a new line or move the following argument to the right.
[06:18] <jtv> (I prefer the former, but would also accept the latter)
[06:18] <lifeless> jtv: I don't see the point of moving conditions to a separate line
[06:19] <jtv> lifeless: it makes it much easier to see that the line needs continuation.
[06:19] <lifeless> really?
[06:19] <jtv> To me, very.  It also makes it easier to see that in terms of grouping, the "conditions" is on par with the "Or."
[06:19] <lifeless> so
[06:19] <lifeless> I will do this
[06:19] <lifeless> but if we do it consistently in the function
[06:20] <lifeless> it will add 30% or so to a long function
[06:20] <lifeless> thats unpleasant
[06:20] <lifeless> I presume you're looking at the diff only atm ?
[06:21] <jtv> I also looked at the source file; it could do with some cleanup, yes.
[06:22] <lifeless> I think its definitely cleaner on the valid = (
[06:22] <lifeless> line
[06:22] <lifeless> I remain ambivalent at best on the conditions section
[06:22] <lifeless> but done, for the stuff in the diff
[06:23] <jtv> Thanks.  I think the problem with the length of the method is just exposing that the method is too long.
[06:24] <jtv> ISTM prepopulate_person really just wants to be driven by a type → attribute name dict.
[06:25] <jtv> (Although aliases may get in the way)
[06:25] <lifeless> rev 11371 pushed for your pleasure
[06:25]  * jtv shudders with delight
[06:25] <lifeless> in future work I would like to push the adapters down into the base property definitions
[06:25] <lifeless> and that way the code is shared
[06:26] <lifeless> there was just not-quite-enough tuits in doing this orginally
[06:26] <jtv> The adapters being the bits that figure out where each result column goes?
[06:28] <jtv> lifeless: r=me.  May you find a nice, round tuit soon.
[06:28] <lifeless> yes
[06:29] <lifeless> thank you
[06:29] <lifeless> I'll finish chatting with stub about this before landing
[06:31] <jtv> lunchtime!
[09:36] <jelmer> 'morning jtv
[09:36] <jtv> hi jelmer
[09:37] <jtv> Thank god you're here.  It's been a madhouse.  There was a branch to review.
[09:37] <jtv> A very small one, but still.
[09:37] <jelmer> :-)
[09:39] <jelmer> jtv: I have a branch up for review if you have time.
[09:39] <jtv> jelmer: that's what ocr is for.
[09:40] <jelmer> jtv: Just trying to be polite :-)
[09:41] <jelmer> jtv: The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa/+merge/32908.
[09:45] <jtv> jelmer: in line 26 of the diff, I take it self._dict is a dict… do you really want to iterate over the keys there?  If so, better make it explicit.
[09:46] <jelmer> jtv: Yep, that's indeed the intention. I'll change it to use iterkeys().
[09:46] <jtv> thx
[09:55] <jtv> jelmer: are you using a list of tuples for the user-defined fields?  Wouldn't a dict make more sense there?
[09:57] <jtv> Although I can see that would upset some ordering in the tests, so you'd want to use assertContentEqual instead of assertEqual there.
[09:57] <jelmer> jtv: The order in the control field could matter; we discard that information at the moment because we convert it to a dict, but in general order is preserved in Debian control files.
[09:58] <jtv> OK
[09:58] <jtv> jelmer: then another question: don't you need any kind of escaping?  For multi-line text attributes for instance?
[09:59] <jelmer> jtv: That's already done for us, as the input file would have the same escaping.
[09:59] <jelmer> Perhaps I should add some tests to verify that though.
[10:00] <jtv> I'd guess you'd want a test to verify that problems with this are caught on input.
[10:04] <jtv> jelmer: btw kudos for breaking out test_preserves_order and test_files.  I'd also break out the "ignores None" part though, because it's a bit implicit.
[10:09] <jtv> jelmer_: you blinked
[10:09] <jtv> don't know if you got this earlier:
[10:09] <jtv> (16:04:00) jtv: jelmer: btw kudos for breaking out test_preserves_order and test_files.  I'd also break out the "ignores None" part though, because it's a bit implicit.
[10:09] <jtv> (time is UTC+0700)
[10:11] <jelmer> jtv: Evolution's memory usage is bringing my laptop down on a regular basis, it's getting a bit ridiculous.
[10:11] <jelmer> jtv: Thanks, I hadn't seen that.
[10:12]  * jtv stopped trying to use evolution after a few machines decided spontaneously to wipe his calendars and ensure somehow that they couldn't be set up again
[10:12] <jtv> When did catastrophic data loss become acceptable outside the windows world?
[11:25] <jelmer> Much as I'd like to avoid it I'm storing my private calendar in Google calendar these days.
[11:44] <jml> jelmer, linux needs a good desktop calendar app
[11:46] <jtv> jelmer: sorry, got distracted and completely forgot to complete your review.  All I had left was some simple layout issues.
[11:46] <jelmer> jtv: No worries
[11:46] <jelmer> jml: It also needs a good email client, still.
[11:47] <jtv> jelmer: in lib/lp/archiveuploader/nascentuploadfile.py line 334 or thereabouts, could you normalise the list comprehension?  Something like:
[11:47] <jml> jelmer, I guess mutt is still the best.
[11:47] <jtv> jelmer:
[11:47] <jtv> return [
[11:47] <jtv>     (field, contents)
[11:47] <jtv>     for (field, contents) in control
[11:47] <jtv>     if field not in self.known_fields]
[11:48] <jtv> jelmer: also, if it doesn't cause conflicts in other branches, could you reformat the __all__ in lib/lp/archiveuploader/tests/__init__.py to our standard one-item-per-line list notation?
[11:49] <jelmer> jtv: Sure, will do.
[11:49] <jtv> thx
[11:50] <jtv> jelmer: there's also one in lines 166—167 of the diff.  A bit hard to figure out the nesting the way it's currently grouped.
[11:50] <jelmer> jtv: do we have a style guide for list comprehensions? What you're proposing looks reasonable, but it'd be nice to have something to fall back to.
[11:51] <jtv> jelmer: I know we used to…  In any case I think it's common sense not to break lines in unexpected places.  Looks a bit like the twins in FC Knudde completing each other's text balloons.
[11:52] <jelmer> hehe
[11:56] <jtv> jelmer: I'm so glad I can talk to someone who gets that one.  :-)
[11:57] <jtv> jelmer: in line 178 of the diff, there's a weird empty line in the docstring for extend.  Did you plan to write something more there?
[12:03] <jelmer> jtv: Nope, that's a formatting glitch. FIxed.
[12:03] <jtv> thx
[12:03] <jtv> jelmer: Another mis-formatted list in lines 229—245 of the diff.  Maybe better put it in a variable of its own.
[12:03] <jtv> (Did you run "make lint" btw?)
[12:04] <jtv> Same for 266—291
[12:06] <jelmer> "make lint" is unfortunately quite chatty about some of the files that I've touched
[12:07] <jtv> I'm not surprised.  :)  Try to do some drive-bys where you can, and make sure none of your own work is implicated.
[12:07] <jtv> Meanwhile, branch approved.
[12:08] <jelmer> Will do, thanks for the review.
[12:08] <jtv> no worries, have a good one!
[12:09]  * jtv is approaching eod
[12:19] <jelmer> jml: wow, this is one big branch :-)
[12:20] <jml> jelmer, yeah. sorry.
[12:32] <jelmer> gmb: hi
[12:32] <jelmer> gmb: I've updated the branch you reviewed yesterday and was wondering if you could have another look.
[12:36] <jelmer> jml: is there any reason you use Branch.open_containing vs Branch.open ?
[12:36] <jml> jelmer, I don't, it's preserved from the original. I reckon we should use Branch.open & would happily change the code.
[12:38] <jelmer> jml: --not-really doesn't really explain to me what it's not doing
[12:39] <jelmer> What about --no-tests or --test-setup-only ?
[12:39] <jtv> jelmer: I have to run now, but was hoping for a review anyway: https://code.edge.launchpad.net/~jtv/launchpad/bug-617431/+merge/32986
[12:40] <jml> jelmer, I'm going to delete the whole thing. it's buggy anyway
[12:40] <jelmer> jtv: Sure, no problemo
[12:41] <jtv> thanks!
[12:41] <jtv> Still updating diff.  :(
[12:41] <jtv> jelmer: in case the cover letter doesn't make it clear, we're already using the msgfmt program (in tests as well as in production) so msgunfmt, which my test uses, should be available.
[12:42] <jelmer> jml: ok, that renders that concern moot then. Those are the only two real points I have. I like the class splitup and testing ftw.
[12:42] <jelmer> jtv: Ok
[12:42] <jml> jelmer, cool, thanks.
[12:43] <jtv> jelmer: argh, the diff is polluted because I branched from devel and proposed for production-devel.  Hang on.
[12:49] <jtv> jelmer: this one should fare better — https://code.edge.launchpad.net/~jtv/launchpad/pd-bug-617431/+merge/32991
[12:50] <jelmer> jtv: thanks
[12:51] <jtv> gargh.  the push got interrupted so the diff is still incomplete
[12:51] <jtv> re-pushing...
[12:53] <jtv> Ah no, it was entirely my fault.  A different push got interrupted.
[12:53] <jelmer> I'm going for lunch now, will have a look afterwards.
[12:53] <jtv> jelmer: ok
[13:08] <gmb> jelmer, Sure, I'll take a look in  the next half hour
[14:14] <jelmer> gmb: Thanks for the re-review.
[14:15] <gmb> np
[15:18] <mars> jelmer, thanks for reviewing jml's branch
[15:19] <jml> mars, did you write the daemonize function in ec2test remote?
[15:19] <mars> jml, no, I thought gary did
[15:22] <mars> jml, argh, the file history is broken!  Maybe bzr explorer can jump the gap
[15:23] <jml> mars, no matter, I think I'm figuring out the reason why something is so without having to consult the original author
[16:11] <sinzui> EdwinGrubbs, jelmer: can either of you review my https://code.launchpad.net/~sinzui/launchpad/delete-team-2/+merge/32948 branch?
[16:12] <EdwinGrubbs> sinzui: sure
[16:15] <EdwinGrubbs> sinzui: does the test runner no longer require the test_suite to find tests?
[16:15] <sinzui> EdwinGrubbs, it does not
[16:19] <bigjools> salgado-lunch: -1 on your last change, I use those policies on the command line for testing
[16:26] <EdwinGrubbs> sinzui: r=me, login_person and login_celebrity are not in alphabetical order
[16:27]  * sinzui will fix
[16:27] <sinzui> and make lint happy
[16:48] <leonardr> jelmer, edwin: please add https://code.edge.launchpad.net/~leonardr/lazr.restful/integrate-optimized-len/+merge/33023 to the queue
[17:02] <salgado> bigjools, you'll still be able to use them; they'll just live closer to the tests now.  although I'd have to change that XXX into a comment explaining why we want the policies registered all the time
[17:03] <bigjools> salgado: ok that's fine if we can still use them from the command line
[17:41] <jml> jelmer, thanks for that review, btw
[17:42] <jml> sinzui, actually, you might prefer login_as to login_person
[17:43] <sinzui> jml, Yes, i do. I think I want to update all the registry tests to use the new login* hlpers
[17:43] <jml> jelmer, it turns out there was a serious bug preventing it from successfully daemonizing. I've written a fairly hideous test for this and fixed the issue. Once I've verified that it works, could I ask you for a review of the interdiff?
[17:43] <jml> (or now, if you don't mind reviewing possibly broken code)
[17:47] <jelmer> jml: yeah, I think so
[17:47] <jelmer> Edwin: Will you have time to look at leonardrs mp?
[17:47] <jelmer> (I'm going to EOD soon, don't want to promise to take on too much work before then)
[17:48] <EdwinGrubbs> jelmer: sure, I can do it.
[17:50] <jml> jelmer, http://pastebin.ubuntu.com/480006/
[17:50] <jml> jelmer, the main difference in the code is passing in & using filenames rather than file-like objects.
[17:54] <jelmer> jml: *nod*
[19:34] <jelmer> 'morning thumper
[19:35] <jelmer> thumper: when you have a moment, any chance you can take a look at https://code.edge.launchpad.net/~jelmer/launchpad/always-import-link/+merge/32668 to check whether my approach is the right one for bug 504868?
[19:36] <jelmer> I'm not looking for a review necessarily, more of a post-pre-imp-call :-)
[19:57] <EdwinGrubbs> leonardr: buildout couldn't find lazr.batchnavigator=1.2.2
[19:58] <leonardr> EdwinGrubbs: it's not released yet. check out lp:lazr.batchnavigator
[20:00] <jcsackett> mars: ping
[20:02] <sinzui> jelmer, EdwinGrubbs: I have a very small branch for review: https://code.launchpad.net/~sinzui/launchpad/remove-mailman-beta-key/+merge/33042
[20:04] <leonardr> edwin: actually, can you stop reviewing my branch? i've made a discovery that indicates it will screw up launchpad, so i don't want to land it now
[20:05] <EdwinGrubbs> leonardr: wow, I'm glad I hadn't approved it, even though I hadn't found anything wrong.
[20:05] <leonardr> edwin: it's a problem with storm, not with this branch
[20:08] <EdwinGrubbs> sinzui: I'll look at it
[20:09] <EdwinGrubbs> sinzui: oh, Francis already approved it with a comment.
[20:09] <sinzui> oh
[20:16] <mars> hi jcsackett
[20:17] <jcsackett> hi mars. i was wondering if you wanted to continue reviewing the mp i had from yesterday, or if i should hand it off to on-call.
[20:19] <mars> jcsackett, if you uploaded a set of changes, you can hand just the changes off to the OCR
[20:19] <mars> there should not be that many of them - a few dozen lines I would think?
[20:21] <thumper> jelmer: ack, I'll get to it after breakfast
[20:23] <jcsackett> mars: actually, lifeless introduced some good ideas that made it a larger change set, but i think i need to revise the cover letter and i'll pass it off to on call.
[20:23] <mars> jcsackett, that sounds good
[20:23] <jcsackett> cool. thanks mars.
[20:45] <jcsackett> Edwin-lunch, jelmer: https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820
[20:46] <jcsackett> Edwin-lunch, jelmer: the mp is a little muddled b/c some review notes pushed the branch towards a much better solution than initially implemented. i revised the cover letter to reflect this.
[20:49] <jelmer> jcsackett: Sorry, I EOD'd a couple of hours ago but forgot to remove my name. Hopefully Edwin will have time to do a review.
[20:49] <jcsackett> jelmer: no problem, sorry to ping you. :-P
[21:57] <EdwinGrubbs> jcsackett: I can look at it.
[21:58] <jcsackett> EdwinGrubbs: thanks!
[22:33] <thumper> jelmer: I wish we had a regenerate diff button
[22:33] <thumper> jelmer: as that merge proposal you have has a lot of changes that aren't part of it
[22:34] <lifeless> thumper: why a button
[22:34] <lifeless> thumper: why not DTRT ?
[22:34] <thumper> lifeless: because we don't regenerate the diff for all merge proposals on the target moving
[22:34] <thumper> lifeless: as it is most often pointless
[22:34] <lifeless> thumper: we could trigger an update if its stale when someone logged in views the page
[22:34] <james_w> prerequisite moving?
[22:35] <lifeless> james_w: trunk changing doesn't make the mp diff change
[22:35] <thumper> lifeless: it may well do
[22:35] <thumper> lifeless: for certain situations
[22:35] <james_w> the prerequisite branch changing certainly can
[22:35] <thumper> james_w: agreed, and we don't do that yet
[22:47] <jelmer> thumper: Should I try to resubmit ?
[23:07] <thumper> jelmer: perhaps recreating targetting devel?
[23:10] <jelmer> thumper, I'll have a look
[23:29] <EdwinGrubbs> jcsackett|afk: I have a question about the team_memberships attribute.