=== jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mwhudson_ is now known as mwhudson [05:59] https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/32953 [05:59] jtv: ^ [05:59] lifeless: I take it this is a review request? [05:59] yes please === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [06:01] lifeless: every time someone opens a new communication with the word "So," I'm tempted to replace it with "Therefore." [06:03] 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] jtv: yeah [06:06] I wasn't aware of it either [06:06] I think it came with the openid stuff, or something [06:06] lifeless: what are the "# --" comments? [06:06] ah, I see now [06:07] clarity in a long function which is simple-but-extended [06:08] i think that validpersoncache was actually a cache at some point [06:09] but that changed, maybe during the account/person split [06:09] * mwhudson may very well be wrong, of course [06:10] 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] conditions = AND( [06:11] conditions, [06:11] Or(... [06:11] (I gather the mixing of SQLObject AND and Storm Or is harmless then) [06:12] the AND is a mistake [06:12] I'll change it [06:12] the indenting shows grouping, I think its much less clear without it [06:12] aim for readability [06:14] That's just the thing. I think this is more readable, not less: [06:15] conditions = And( [06:15] conditions, [06:15] Or( [06:15] Account.status == None, [06:15] Account.status == AccountStatus.ACTIVE)) [06:15] I can do that [06:15] that wasn't what you seemed to be asking for [06:18] 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] (I prefer the former, but would also accept the latter) [06:18] jtv: I don't see the point of moving conditions to a separate line [06:19] lifeless: it makes it much easier to see that the line needs continuation. [06:19] really? [06:19] 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] so [06:19] I will do this [06:19] but if we do it consistently in the function [06:20] it will add 30% or so to a long function [06:20] thats unpleasant [06:20] I presume you're looking at the diff only atm ? [06:21] I also looked at the source file; it could do with some cleanup, yes. [06:22] I think its definitely cleaner on the valid = ( [06:22] line [06:22] I remain ambivalent at best on the conditions section [06:22] but done, for the stuff in the diff [06:23] Thanks. I think the problem with the length of the method is just exposing that the method is too long. [06:24] ISTM prepopulate_person really just wants to be driven by a type → attribute name dict. [06:25] (Although aliases may get in the way) [06:25] rev 11371 pushed for your pleasure [06:25] * jtv shudders with delight [06:25] in future work I would like to push the adapters down into the base property definitions [06:25] and that way the code is shared [06:26] there was just not-quite-enough tuits in doing this orginally [06:26] The adapters being the bits that figure out where each result column goes? [06:28] lifeless: r=me. May you find a nice, round tuit soon. [06:28] yes [06:29] thank you [06:29] I'll finish chatting with stub about this before landing === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [06:31] lunchtime! === jtv is now known as jtv-eat === jtv-eat is now known as jtv === jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [09:36] 'morning jtv [09:36] hi jelmer [09:37] Thank god you're here. It's been a madhouse. There was a branch to review. [09:37] A very small one, but still. [09:37] :-) [09:39] jtv: I have a branch up for review if you have time. [09:39] jelmer: that's what ocr is for. === jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: -, jelmer || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: -, jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [09:40] jtv: Just trying to be polite :-) [09:41] jtv: The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa/+merge/32908. === jtv changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: jelmer, jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [09:45] 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] jtv: Yep, that's indeed the intention. I'll change it to use iterkeys(). [09:46] thx [09:55] jelmer: are you using a list of tuples for the user-defined fields? Wouldn't a dict make more sense there? [09:57] 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] 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] OK [09:58] jelmer: then another question: don't you need any kind of escaping? For multi-line text attributes for instance? [09:59] jtv: That's already done for us, as the input file would have the same escaping. [09:59] Perhaps I should add some tests to verify that though. [10:00] I'd guess you'd want a test to verify that problems with this are caught on input. [10:04] 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] jelmer_: you blinked [10:09] don't know if you got this earlier: [10:09] (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] (time is UTC+0700) === jelmer_ is now known as jelmer [10:11] jtv: Evolution's memory usage is bringing my laptop down on a regular basis, it's getting a bit ridiculous. [10:11] 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] When did catastrophic data loss become acceptable outside the windows world? [11:25] Much as I'd like to avoid it I'm storing my private calendar in Google calendar these days. [11:44] jelmer, linux needs a good desktop calendar app [11:46] jelmer: sorry, got distracted and completely forgot to complete your review. All I had left was some simple layout issues. [11:46] jtv: No worries [11:46] jml: It also needs a good email client, still. [11:47] jelmer: in lib/lp/archiveuploader/nascentuploadfile.py line 334 or thereabouts, could you normalise the list comprehension? Something like: [11:47] jelmer, I guess mutt is still the best. [11:47] jelmer: [11:47] return [ [11:47] (field, contents) [11:47] for (field, contents) in control [11:47] if field not in self.known_fields] [11:48] 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] jtv: Sure, will do. [11:49] thx [11:50] 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] 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] 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. === danilo_ is now known as danilos [11:52] hehe [11:56] jelmer: I'm so glad I can talk to someone who gets that one. :-) [11:57] 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] jtv: Nope, that's a formatting glitch. FIxed. [12:03] thx [12:03] jelmer: Another mis-formatted list in lines 229—245 of the diff. Maybe better put it in a variable of its own. [12:03] (Did you run "make lint" btw?) [12:04] Same for 266—291 [12:06] "make lint" is unfortunately quite chatty about some of the files that I've touched [12:07] 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] Meanwhile, branch approved. === jtv changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:08] Will do, thanks for the review. [12:08] no worries, have a good one! [12:09] * jtv is approaching eod [12:19] jml: wow, this is one big branch :-) [12:20] jelmer, yeah. sorry. [12:32] gmb: hi [12:32] gmb: I've updated the branch you reviewed yesterday and was wondering if you could have another look. [12:36] jml: is there any reason you use Branch.open_containing vs Branch.open ? [12:36] jelmer, I don't, it's preserved from the original. I reckon we should use Branch.open & would happily change the code. [12:38] jml: --not-really doesn't really explain to me what it's not doing [12:39] What about --no-tests or --test-setup-only ? [12:39] 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] jelmer, I'm going to delete the whole thing. it's buggy anyway [12:40] jtv: Sure, no problemo [12:41] thanks! [12:41] Still updating diff. :( [12:41] 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] 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] jtv: Ok [12:42] jelmer, cool, thanks. [12:43] jelmer: argh, the diff is polluted because I branched from devel and proposed for production-devel. Hang on. [12:49] jelmer: this one should fare better — https://code.edge.launchpad.net/~jtv/launchpad/pd-bug-617431/+merge/32991 [12:50] jtv: thanks [12:51] gargh. the push got interrupted so the diff is still incomplete [12:51] re-pushing... [12:53] Ah no, it was entirely my fault. A different push got interrupted. [12:53] I'm going for lunch now, will have a look afterwards. [12:53] jelmer: ok === matsubara-afk is now known as matsubara [13:08] jelmer, Sure, I'll take a look in the next half hour === leonardr_ is now known as leonardr === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [14:14] gmb: Thanks for the re-review. [14:15] np === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, - || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:18] jelmer, thanks for reviewing jml's branch [15:19] mars, did you write the daemonize function in ec2test remote? [15:19] jml, no, I thought gary did [15:22] jml, argh, the file history is broken! Maybe bzr explorer can jump the gap [15:23] mars, no matter, I think I'm figuring out the reason why something is so without having to consult the original author === salgado is now known as salgado-lunch [16:11] EdwinGrubbs, jelmer: can either of you review my https://code.launchpad.net/~sinzui/launchpad/delete-team-2/+merge/32948 branch? [16:12] sinzui: sure === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:15] sinzui: does the test runner no longer require the test_suite to find tests? [16:15] EdwinGrubbs, it does not [16:19] salgado-lunch: -1 on your last change, I use those policies on the command line for testing [16:26] sinzui: r=me, login_person and login_celebrity are not in alphabetical order [16:27] * sinzui will fix [16:27] and make lint happy [16:48] jelmer, edwin: please add https://code.edge.launchpad.net/~leonardr/lazr.restful/integrate-optimized-len/+merge/33023 to the queue === jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === salgado-lunch is now known as salgado [17:02] 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] salgado: ok that's fine if we can still use them from the command line === matsubara is now known as matsubara-lunch === deryck is now known as deryck[lunch] === Ursinha is now known as Ursinha-lunch [17:41] jelmer, thanks for that review, btw [17:42] sinzui, actually, you might prefer login_as to login_person [17:43] jml, Yes, i do. I think I want to update all the registry tests to use the new login* hlpers [17:43] 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] (or now, if you don't mind reviewing possibly broken code) [17:47] jml: yeah, I think so [17:47] Edwin: Will you have time to look at leonardrs mp? [17:47] (I'm going to EOD soon, don't want to promise to take on too much work before then) [17:48] jelmer: sure, I can do it. [17:50] jelmer, http://pastebin.ubuntu.com/480006/ [17:50] jelmer, the main difference in the code is passing in & using filenames rather than file-like objects. [17:54] jml: *nod* === matsubara-lunch is now known as matsubara === deryck[lunch] is now known as deryck === Ursinha-lunch is now known as Ursinha [19:34] 'morning thumper [19:35] 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] I'm not looking for a review necessarily, more of a post-pre-imp-call :-) === benji is now known as benji-lunch [19:57] leonardr: buildout couldn't find lazr.batchnavigator=1.2.2 [19:58] EdwinGrubbs: it's not released yet. check out lp:lazr.batchnavigator [20:00] mars: ping [20:02] jelmer, EdwinGrubbs: I have a very small branch for review: https://code.launchpad.net/~sinzui/launchpad/remove-mailman-beta-key/+merge/33042 [20:04] 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] leonardr: wow, I'm glad I hadn't approved it, even though I hadn't found anything wrong. [20:05] edwin: it's a problem with storm, not with this branch [20:08] sinzui: I'll look at it === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [20:09] sinzui: oh, Francis already approved it with a comment. [20:09] oh [20:16] hi jcsackett [20:17] 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] jcsackett, if you uploaded a set of changes, you can hand just the changes off to the OCR [20:19] there should not be that many of them - a few dozen lines I would think? [20:21] jelmer: ack, I'll get to it after breakfast [20:23] 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] jcsackett, that sounds good [20:23] cool. thanks mars. === EdwinGrubbs is now known as Edwin-lunch === benji-lunch is now known as benji === jcsackett changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [20:45] Edwin-lunch, jelmer: https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820 [20:46] 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. === matsubara is now known as matsubara-afk [20:49] 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] jelmer: no problem, sorry to ping you. :-P === salgado is now known as salgado-afk === Edwin-lunch is now known as EdwinGrubbs [21:57] jcsackett: I can look at it. === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: jtv, sinzui || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:58] EdwinGrubbs: thanks! === _mup__ is now known as _mup_ === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: jcsacket || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [22:33] jelmer: I wish we had a regenerate diff button [22:33] jelmer: as that merge proposal you have has a lot of changes that aren't part of it [22:34] thumper: why a button [22:34] thumper: why not DTRT ? [22:34] lifeless: because we don't regenerate the diff for all merge proposals on the target moving [22:34] lifeless: as it is most often pointless [22:34] thumper: we could trigger an update if its stale when someone logged in views the page [22:34] prerequisite moving? [22:35] james_w: trunk changing doesn't make the mp diff change [22:35] lifeless: it may well do [22:35] lifeless: for certain situations [22:35] the prerequisite branch changing certainly can [22:35] james_w: agreed, and we don't do that yet [22:47] thumper: Should I try to resubmit ? === Ursinha is now known as Ursinha-bbl === jcsackett is now known as jcsackett|afk [23:07] jelmer: perhaps recreating targetting devel? [23:10] thumper, I'll have a look [23:29] jcsackett|afk: I have a question about the team_memberships attribute.