| === 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 | ||
| 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 | 05:59 |
| === 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 | ||
| jtv | lifeless: every time someone opens a new communication with the word "So," I'm tempted to replace it with "Therefore." | 06:01 |
| 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:03 |
| 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:06 |
| lifeless | clarity in a long function which is simple-but-extended | 06:07 |
| mwhudson | i think that validpersoncache was actually a cache at some point | 06:08 |
| mwhudson | but that changed, maybe during the account/person split | 06:09 |
| * mwhudson may very well be wrong, of course | 06:09 | |
| 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:10 |
| 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:11 |
| 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:12 |
| jtv | That's just the thing. I think this is more readable, not less: | 06:14 |
| 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:15 |
| 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:18 |
| 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:19 |
| 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:20 |
| jtv | I also looked at the source file; it could do with some cleanup, yes. | 06:21 |
| 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:22 |
| jtv | Thanks. I think the problem with the length of the method is just exposing that the method is too long. | 06:23 |
| jtv | ISTM prepopulate_person really just wants to be driven by a type → attribute name dict. | 06:24 |
| 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:25 |
| 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:26 |
| jtv | lifeless: r=me. May you find a nice, round tuit soon. | 06:28 |
| lifeless | yes | 06:28 |
| lifeless | thank you | 06:29 |
| lifeless | I'll finish chatting with stub about this before landing | 06:29 |
| === 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 | ||
| jtv | lunchtime! | 06:31 |
| === 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 | ||
| jelmer | 'morning jtv | 09:36 |
| jtv | hi jelmer | 09:36 |
| 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:37 |
| jelmer | jtv: I have a branch up for review if you have time. | 09:39 |
| jtv | jelmer: that's what ocr is for. | 09:39 |
| === 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 | ||
| jelmer | jtv: Just trying to be polite :-) | 09:40 |
| jelmer | jtv: The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa/+merge/32908. | 09:41 |
| === 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 | ||
| 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:45 |
| jelmer | jtv: Yep, that's indeed the intention. I'll change it to use iterkeys(). | 09:46 |
| jtv | thx | 09:46 |
| jtv | jelmer: are you using a list of tuples for the user-defined fields? Wouldn't a dict make more sense there? | 09:55 |
| 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:57 |
| 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:58 |
| 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. | 09:59 |
| jtv | I'd guess you'd want a test to verify that problems with this are caught on input. | 10: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:04 |
| 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:09 |
| === jelmer_ is now known as jelmer | ||
| 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:11 |
| * 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? | 10:12 |
| jelmer | Much as I'd like to avoid it I'm storing my private calendar in Google calendar these days. | 11:25 |
| jml | jelmer, linux needs a good desktop calendar app | 11:44 |
| 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:46 |
| 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:47 |
| 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:48 |
| jelmer | jtv: Sure, will do. | 11:49 |
| jtv | thx | 11:49 |
| 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:50 |
| 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:51 |
| === danilo_ is now known as danilos | ||
| jelmer | hehe | 11:52 |
| jtv | jelmer: I'm so glad I can talk to someone who gets that one. :-) | 11:56 |
| 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? | 11:57 |
| 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:03 |
| jtv | Same for 266—291 | 12:04 |
| jelmer | "make lint" is unfortunately quite chatty about some of the files that I've touched | 12:06 |
| 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:07 |
| === 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 | ||
| jelmer | Will do, thanks for the review. | 12:08 |
| jtv | no worries, have a good one! | 12:08 |
| * jtv is approaching eod | 12:09 | |
| jelmer | jml: wow, this is one big branch :-) | 12:19 |
| jml | jelmer, yeah. sorry. | 12:20 |
| 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:32 |
| 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:36 |
| jelmer | jml: --not-really doesn't really explain to me what it's not doing | 12:38 |
| 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:39 |
| jml | jelmer, I'm going to delete the whole thing. it's buggy anyway | 12:40 |
| jelmer | jtv: Sure, no problemo | 12:40 |
| 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:41 |
| 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:42 |
| jtv | jelmer: argh, the diff is polluted because I branched from devel and proposed for production-devel. Hang on. | 12:43 |
| jtv | jelmer: this one should fare better — https://code.edge.launchpad.net/~jtv/launchpad/pd-bug-617431/+merge/32991 | 12:49 |
| jelmer | jtv: thanks | 12:50 |
| jtv | gargh. the push got interrupted so the diff is still incomplete | 12:51 |
| jtv | re-pushing... | 12:51 |
| 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 | 12:53 |
| === matsubara-afk is now known as matsubara | ||
| gmb | jelmer, Sure, I'll take a look in the next half hour | 13:08 |
| === leonardr_ is now known as leonardr | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| jelmer | gmb: Thanks for the re-review. | 14:14 |
| gmb | np | 14:15 |
| === 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 | ||
| mars | jelmer, thanks for reviewing jml's branch | 15:18 |
| jml | mars, did you write the daemonize function in ec2test remote? | 15:19 |
| mars | jml, no, I thought gary did | 15:19 |
| mars | jml, argh, the file history is broken! Maybe bzr explorer can jump the gap | 15:22 |
| jml | mars, no matter, I think I'm figuring out the reason why something is so without having to consult the original author | 15:23 |
| === salgado is now known as salgado-lunch | ||
| sinzui | EdwinGrubbs, jelmer: can either of you review my https://code.launchpad.net/~sinzui/launchpad/delete-team-2/+merge/32948 branch? | 16:11 |
| EdwinGrubbs | sinzui: sure | 16:12 |
| === 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 | ||
| EdwinGrubbs | sinzui: does the test runner no longer require the test_suite to find tests? | 16:15 |
| sinzui | EdwinGrubbs, it does not | 16:15 |
| bigjools | salgado-lunch: -1 on your last change, I use those policies on the command line for testing | 16:19 |
| EdwinGrubbs | sinzui: r=me, login_person and login_celebrity are not in alphabetical order | 16:26 |
| * sinzui will fix | 16:27 | |
| sinzui | and make lint happy | 16:27 |
| leonardr | jelmer, edwin: please add https://code.edge.launchpad.net/~leonardr/lazr.restful/integrate-optimized-len/+merge/33023 to the queue | 16:48 |
| === 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 | ||
| 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:02 |
| bigjools | salgado: ok that's fine if we can still use them from the command line | 17:03 |
| === matsubara is now known as matsubara-lunch | ||
| === deryck is now known as deryck[lunch] | ||
| === Ursinha is now known as Ursinha-lunch | ||
| jml | jelmer, thanks for that review, btw | 17:41 |
| jml | sinzui, actually, you might prefer login_as to login_person | 17:42 |
| 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:43 |
| 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:47 |
| EdwinGrubbs | jelmer: sure, I can do it. | 17:48 |
| 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:50 |
| jelmer | jml: *nod* | 17:54 |
| === matsubara-lunch is now known as matsubara | ||
| === deryck[lunch] is now known as deryck | ||
| === Ursinha-lunch is now known as Ursinha | ||
| jelmer | 'morning thumper | 19:34 |
| 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:35 |
| jelmer | I'm not looking for a review necessarily, more of a post-pre-imp-call :-) | 19:36 |
| === benji is now known as benji-lunch | ||
| EdwinGrubbs | leonardr: buildout couldn't find lazr.batchnavigator=1.2.2 | 19:57 |
| leonardr | EdwinGrubbs: it's not released yet. check out lp:lazr.batchnavigator | 19:58 |
| jcsackett | mars: ping | 20:00 |
| sinzui | jelmer, EdwinGrubbs: I have a very small branch for review: https://code.launchpad.net/~sinzui/launchpad/remove-mailman-beta-key/+merge/33042 | 20:02 |
| 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:04 |
| 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:05 |
| EdwinGrubbs | sinzui: I'll look at it | 20:08 |
| === 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 | ||
| EdwinGrubbs | sinzui: oh, Francis already approved it with a comment. | 20:09 |
| sinzui | oh | 20:09 |
| mars | hi jcsackett | 20:16 |
| 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:17 |
| 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:19 |
| thumper | jelmer: ack, I'll get to it after breakfast | 20:21 |
| 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:23 |
| === 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 | ||
| jcsackett | Edwin-lunch, jelmer: https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820 | 20:45 |
| 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:46 |
| === matsubara is now known as matsubara-afk | ||
| 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 | 20:49 |
| === salgado is now known as salgado-afk | ||
| === Edwin-lunch is now known as EdwinGrubbs | ||
| EdwinGrubbs | jcsackett: I can look at it. | 21:57 |
| === 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 | ||
| jcsackett | EdwinGrubbs: thanks! | 21:58 |
| === _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 | ||
| 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:33 |
| 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:34 |
| 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:35 |
| jelmer | thumper: Should I try to resubmit ? | 22:47 |
| === Ursinha is now known as Ursinha-bbl | ||
| === jcsackett is now known as jcsackett|afk | ||
| thumper | jelmer: perhaps recreating targetting devel? | 23:07 |
| jelmer | thumper, I'll have a look | 23:10 |
| EdwinGrubbs | jcsackett|afk: I have a question about the team_memberships attribute. | 23:29 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!