| === 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 On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === 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 | Anyone willing to review a branch for the OCR? I've been leaning on henninge far too much for my reviews. | 09:24 |
|---|---|---|
| jtv | noodles775, you could do this! | 09:24 |
| jtv | Willing to review https://code.edge.launchpad.net/~jtv/launchpad/extract-approval-js ? | 09:24 |
| jtv | Ahem... MP is https://code.launchpad.net/~jtv/launchpad/extract-approval-js/+merge/28852 | 09:25 |
| noodles775 | Sorry jtv - I'm working to get an urgent feature done right now... | 09:29 |
| * jtv continues his search for prey | 09:29 | |
| jtv | rockstar, on the off-chance you're working right now: I'm hoping to get a javascript review... https://code.launchpad.net/~jtv/launchpad/extract-approval-js/+merge/28852 | 09:33 |
| rinze | jtv: I can code review, but I'm not a UI reviewer | 09:42 |
| jtv | rinze: that's fine—there's no UI change apart from chromium now showing things properly like other browsers. | 09:43 |
| jtv | rinze: is javascript a problem? | 09:43 |
| rinze | no, javascript should be fine | 09:44 |
| jtv | And... change of nick? | 09:44 |
| * rinze claims the MP | 09:44 | |
| jtv | thanks | 09:44 |
| rinze | jtv: I switched to two nicks that I can connect/disconnect independently, one for work and one for other FOSS stuff | 09:44 |
| jtv | begrijpelijk | 09:44 |
| rinze | jtv: I haven't forgotten your branch but it's taking a wee bit longer since my JavaScript is a bit rusty. | 12:46 |
| jtv | rinze: in that case, it'd probably be better to go over it with a javascript-specialized reviewer—gets you unrusted and also lets you pick up JS coding guidelines. | 12:47 |
| rinze | jtv: Yeah, that'd be useful, though there doesn't appear to be one around at the moment :-( | 12:51 |
| jtv | ...and there's the catch. | 12:52 |
| rinze | (I've just been looking at https://dev.launchpad.net/JavaScriptReviewNotes) | 12:52 |
| jtv | rinze: looking at those myself now... I see that my documentation needs some tweaks. | 12:54 |
| bigjools | jtv: hi | 12:56 |
| jtv | hi bigjools | 12:56 |
| bigjools | jtv: can you review this for me please: https://code.edge.launchpad.net/~julian-edwards/launchpad/copy-archive-fix-component/+merge/28871 | 12:56 |
| bigjools | it's going to be a CP | 12:56 |
| jtv | bigjools: so speed matters... right ho | 12:57 |
| bigjools | thanks | 12:57 |
| jtv | rinze: I was in the process of updating my documentation; will finish that after julian's review. BTW I think I forgot to mention that there was no lint. | 12:58 |
| rinze | rinze, ok | 12:59 |
| bigjools | rinze: how many other nicks have you got? :) | 13:00 |
| jtv | bigjools: approved | 13:01 |
| bigjools | jtv: cheers | 13:01 |
| bigjools | now, I need flacoste to do the same :) | 13:01 |
| jtv | quite | 13:01 |
| jtv | I suppose you can land on devel in the meantime. | 13:01 |
| rinze | bigjools: just these three :-) | 13:04 |
| rinze | jtv: I also have another branch up for review | 13:05 |
| jtv | rinze: let's have it then | 13:05 |
| rinze | https://code.edge.launchpad.net/~jelmer/launchpad/600153-main-archive-builds-on-restricted/+merge/28867 | 13:05 |
| === matsubara-afk is now known as matsubara | ||
| jtv | rinze: did you run "make lint" btw? | 13:15 |
| rinze | jtv: No, I haven't run make lint | 13:17 |
| jtv | Make it a habit. We're supposed to do that before submitting the MP. | 13:17 |
| rinze | ok | 13:19 |
| jtv | Also, I know it's not your fault (or at least not in this branch), but could you give _get_enabled_restricted_families and _set_enabled_restricted_families docstrings explaining what their purpose is? If they're methods, give them dromedaryCase names rather than underscore_separated names. | 13:22 |
| * rinze nods | 13:25 | |
| jtv | Also, in _set_enabled_restricted_families please consider avoiding that nasty line break in the "if" condition. Not very important, but would-be-nice. | 13:27 |
| bigjools | jtv: I have another branch up, not urgent though, at your leisure! | 13:28 |
| * bigjools needs to eat | 13:28 | |
| jtv | bigjools: on the queue please | 13:28 |
| rinze | jtv: I considered that but I couldn't think of an easy way of doing that without adding a nested if statement | 13:28 |
| === bigjools changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | done, ta | 13:28 |
| jtv | Thanks. My fault for not updating the topic. | 13:28 |
| jtv | On call: jtv || reviewing: rinze || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | 13:28 |
| === rinze changed the topic of #launchpad-reviews to: On call: jtv || reviewing: rinze || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | rinze: in this case a nested would not be _that_ bad if you combine it with a temporary variable. | 13:29 |
| jtv | rinze: thanks :) | 13:29 |
| jtv | rinze: btw I was wondering... what does CannotRestrictArchitectures._fmt do? | 13:31 |
| rinze | jtv: I believe it's used for the stringified version of the exception | 13:35 |
| jtv | rinze: stringified version? You mean for __repr__'ing it? | 13:35 |
| jtv | rinze: come to think of it I'd prefer it if you just passed the error message to the constructor. | 13:36 |
| rinze | I meant __str__, but now that I'm checking it doesn't actually appear to be the case. | 13:36 |
| rinze | it's what some of the other exceptions used as well | 13:37 |
| rinze | jtv: That makes sense | 13:37 |
| jtv | Also avoids changes between python versions... the underscore sort of says "don't mess with this, it looks funny and there's a faint ticking noise coming out of it." | 13:38 |
| jtv | rinze: btw lint doesn't complain about it, but try to avoid trailing whitespace as well. | 13:49 |
| flacoste | bigjools: what do you need me for? | 13:56 |
| jtv | rinze: you have a line overrun in your docstring for test_main_archive_can_use_restricted. You can fix that by turning it into a comment—we don't usually write docstrings for test methods. | 14:17 |
| jtv | rinze: and have a look at TestCase.assertContentEqual; will save you a bit of listification there. | 14:19 |
| rinze | rinze: The style guide is ambiguous in that regard: "Write in the docstring of each test case what is being tested. As a special case for test methods, a comment block at the beginning of the method is considered an acceptable substitute to a docstring." | 14:19 |
| rinze | s/rinze/jtv/ | 14:20 |
| rinze | jtv: Ah, I wasn't aware of that one - thanks | 14:20 |
| jtv | rinze: we don't usually write docstrings there... saves us having to follow the docstring format. | 14:20 |
| jtv | So don't feel too bad about being merely "considered acceptable." :-) | 14:20 |
| rinze | heh, ok | 14:23 |
| jtv | bigjools, line 66 of your diff: s/Lets/Let's/ | 14:31 |
| bigjools | grar | 14:32 |
| jtv | But frankly starting paragraphs with that is usually an indicator that you're trying to pass off setup code as documentation. | 14:32 |
| jtv | You may just want to say something like "we have a" rather than "let's make a" | 14:32 |
| bigjools | I welcome any recommendation to make it better | 14:32 |
| jtv | Do it all in French! | 14:33 |
| jtv | no wait | 14:33 |
| bigjools | Enc... | 14:33 |
| bigjools | no waut | 14:33 |
| jtv | ? | 14:33 |
| bigjools | typing is clearly not my strong point today | 14:34 |
| jtv | bigjools: instead of... | 14:35 |
| jtv | https://code.launchpad.net/~julian-edwards/launchpad/api-commercial-ppas-bug-597211/+merge/28877 | 14:35 |
| jtv | argh | 14:35 |
| jtv | Instead of... | 14:35 |
| jtv | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) | 14:35 |
| bigjools | are we putting the missing U back? | 14:36 |
| jtv | ...you can just say... | 14:36 |
| jtv | store = IStore(Archive) | 14:36 |
| jtv | The missing U back? I wish. But now we can say it's a metric U and some people just haven't gotten around to adopting it yet. | 14:36 |
| bigjools | does that select a slave or master depending on the operation? | 14:36 |
| jtv | Yes, that's the default flavour. | 14:37 |
| bigjools | cool | 14:37 |
| jtv | There's also IMasterStore and ISlaveStore. Thank stub for that. | 14:37 |
| bigjools | we should do a branch to fix all of those at once | 14:37 |
| bigjools | yeah I know, I have a jihad branch that is forcibly choosing ISlaveStore but it breaks a million tests | 14:37 |
| jtv | In some places you'll need to commit just to make the slave catch up. | 14:38 |
| bigjools | I know - but it's not as simple as that | 14:38 |
| jtv | bigjools: I was hoping for more here :) | 14:40 |
| jtv | what complications do you have in mind? | 14:40 |
| bigjools | I'm gonna take a new strategy of using the Slave policy in top-level scripts and cast objects to IMasterObject where I need to write them | 14:40 |
| jtv | BTW with those changes the branch looks good to me. Nice work. | 14:40 |
| stub | If IStore(Archive) doesn't work because it is not derived from SQLBase (not sure), they can review my branch with the one line fix. | 14:41 |
| jtv | bigjools: I just approved your branch | 14:43 |
| bigjools | jtv: grassyass | 14:43 |
| jtv | rinze, how's yours coming along? | 14:43 |
| jtv | bigjools: de nutter | 14:43 |
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| * jtv is taking a break | 14:55 | |
| === jtv is now known as jtv-afk | ||
| === jtv-afk is now known as jtv | ||
| === jtv is now known as jtv-afk | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv-afk | hi EdwinGrubbs! | 15:17 |
| jtv-afk | Maybe you can review one of mine? | 15:17 |
| EdwinGrubbs | jtv-afk: sure | 15:17 |
| jtv-afk | EdwinGrubbs: e.g. https://code.launchpad.net/~jtv/launchpad/bug-580337/+merge/28880 | 15:17 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: rinze, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | jtv-afk: what does "test_getVariable_smoke" mean? I don't see anything called "smoke". | 16:04 |
| === jtv-afk is now known as jtv | ||
| jtv | EdwinGrubbs: smoke test :) | 16:32 |
| EdwinGrubbs | I kinda guessed, but it seemed a little vague. | 16:34 |
| EdwinGrubbs | jtv: review sent | 16:35 |
| jtv | EdwinGrubbs: thanks! | 16:35 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-lunch | ||
| EdwinGrubbs | jtv: btw, do you have any thoughts on the email I sent about adding code to POTemplate.importFromQueue() to cache the message count? | 17:20 |
| jtv | EdwinGrubbs: oh, I swapped that out... hang on | 17:20 |
| jtv | EdwinGrubbs: istm this count doesn't have to be time-accurate so an offline job may be more suitable. Apart from that it made sense. | 17:22 |
| jtv | But is the caching necessary? Summing the template message counts for a package shouldn't be too costly. | 17:23 |
| jtv | (As long as you don't go doing it in python) | 17:23 |
| EdwinGrubbs | jtv: the problem occurs because the +needs-packaging page sums the message counts for each sourcepackage and then sorts the sourcepackages based on that. | 17:24 |
| jtv | EdwinGrubbs: SQL-side or python-side? | 17:24 |
| EdwinGrubbs | jtv: it's summed in sql | 17:24 |
| jtv | Oh, it's the old "sort before you batch" problem? | 17:25 |
| jtv | Where you don't have any performance problems thanks to batching, _except_ determining the order to batch in scales really badly with overall table size? | 17:25 |
| EdwinGrubbs | jtv: I'm assuming that there are multiple POTemplates per sourcepackage, since we are using a subquery. If it was one-to-one, we could join instead. | 17:26 |
| jtv | Well you can still left-join—it's just a question of how costly that is. | 17:27 |
| EdwinGrubbs | does the message count typically change more than once a day for each sourcepackage? If not, a daily cronjob would actually be more db intensive. | 17:27 |
| jtv | Unless we pick out only the templates that _have_ changed. | 17:28 |
| jtv | A message queue would be ideal, but timestamps would do it as well. | 17:28 |
| EdwinGrubbs | jtv: are you saying it's either 1-to-1 or 1-to-0? I thought it was 1-to-many, which a left-join wouldn't help. | 17:28 |
| * EdwinGrubbs feels like I'm talking about world cup scores. | 17:28 | |
| === deryck is now known as deryck[lunch] | ||
| jtv | It's one-to-many... but you can still "group by" | 17:30 |
| jtv | select sourcepackagename.id, sum(messagecount) from sourcepackagename join potemplate on potemplate.sourcepackagename = sourcepackagename.id group by sourcepackagename.id; | 17:31 |
| EdwinGrubbs | jtv: how is POTemplate.date_last_updated updated? I don't see a trigger, and the POTemplate class doesn't seem to do it automatically or in importFromQueue(). | 17:35 |
| rinze | jtv: I've pushed an updated version of my branch | 17:38 |
| jtv | EdwinGrubbs: in POTFileImporter | 17:39 |
| jtv | rinze: cool, thanks... I'll get on it in a minute | 17:39 |
| jtv | rinze: in ll.66—67, could you capitalize and punctuate the error string? | 17:41 |
| jtv | EdwinGrubbs: so it is up-to-date, though it may be python-side time rather than database-side time. But I guess that could be fixed if needed. | 17:42 |
| rinze | rinze, yep | 17:42 |
| jtv | rinze: you're talking to yourself again :) | 17:42 |
| rinze | doh | 17:43 |
| jtv | rinze: anyway, with that one remark, I'm approving. Please give me a moment to document. | 17:43 |
| jtv | rinze: done | 17:47 |
| rinze | jtv: thanks! | 17:47 |
| jtv | np | 17:47 |
| * jtv is about done | 17:47 | |
| === jtv 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 | ||
| rinze | EdwinGrubbs: Can I add a branch to your queue? It's a simple one to fix the merge of stable into db-devel | 17:56 |
| EdwinGrubbs | rinze: sure | 17:57 |
| rinze | EdwinGrubbs, https://code.edge.launchpad.net/~jelmer/launchpad/stable-fix/+merge/28912 | 17:58 |
| === deryck[lunch] is now known as deryck | ||
| EdwinGrubbs | rinze: this branch appears to have a lot more than resolved conflicts. Is there another branch that was already reviewed but never landed? | 18:11 |
| rinze | EdwinGrubbs: It merges stable into db-devel so it's got all the other changes from stable as well | 18:12 |
| === matsubara-lunch is now known as matsubara | ||
| rinze | EdwinGrubbs: I don't have much experience fixing these sorts of merges, I wonder what the best way is to present these changes for review. | 18:15 |
| EdwinGrubbs | rinze: it's a good idea to add a comment to the mp which shows just the two files that you had to modify. | 18:19 |
| rinze | EdwinGrubbs, the two files are lib/lp/soyuz/{configure.zcml,model/archive.py} | 18:23 |
| EdwinGrubbs | rinze: what editor do you use? | 18:30 |
| rinze | EdwinGrubbs, vi | 18:30 |
| === jtv is now known as jtv-afk | ||
| EdwinGrubbs | rinze: your branch contains an unusual amount of dangling whitespace and tab-characters as opposed to spaces. Most of us have vi or emacs configured to highlight these. See https://dev.launchpad.net/UltimateVimPythonSetup | 18:32 |
| EdwinGrubbs | rinze: the most import configs are | 18:33 |
| EdwinGrubbs | highlight WhitespaceEOL ctermbg=red guibg=red | 18:33 |
| EdwinGrubbs | match WhitespaceEOL /\s\+$/ | 18:33 |
| EdwinGrubbs | and | 18:33 |
| EdwinGrubbs | setlocal list listchars=tab:>~,trail:* | 18:34 |
| rinze | EdwinGrubbs, thanks | 18:35 |
| EdwinGrubbs | rinze: actually, setlocal should be just 'set' unless it is called from a command that checks the filetype of each newly opened file. | 18:35 |
| EdwinGrubbs | rinze: btw, at the very bottom of that wiki page there is a branch that I made that incorporates almost all the configs, so you don't have to copy and paste or download dependencies. | 18:39 |
| rinze | EdwinGrubbs, thanks for the review | 18:52 |
| rinze | EdwinGrubbs, and thanks for the hint about the vim config branch, I'll have a look at it sometime | 18:52 |
| abentley | EdwinGrubbs, could you please review https://code.edge.launchpad.net/~abentley/launchpad/fix-stuck/+merge/28924 ? | 19:16 |
| EdwinGrubbs | abentley: sure | 19:19 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | abentley: looking at the .bzrignore, it seems odd that the source package files would be placed directly in the lib/canonical/ directory and not in a subdirectory. | 19:35 |
| abentley | EdwinGrubbs, IME, building a package places the files in the parent directory. | 19:36 |
| EdwinGrubbs | oh, I see, you didn't want to have two levels of temp directories. | 19:36 |
| EdwinGrubbs | abentley: it also seems like SourcePackageRecipeBuildManager.initiate() would benefit from adding assert isinstance(self.author_name, unicode) so you get an early warning if None or a str is passed in. | 19:38 |
| abentley | EdwinGrubbs, how would that be a benefit? | 19:42 |
| EdwinGrubbs | abentley: well, it would only catch tests that initialize the object without running it. I'm not sure if that is even plausible. | 19:45 |
| EdwinGrubbs | abentley: r=me | 19:52 |
| abentley | EdwinGrubbs, thanks! | 19:53 |
| === matsubara is now known as matsubara-afk | ||
| abentley | rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/process-upload-no-cwd/+merge/28682 ? | 20:49 |
| rockstar | abentley, ack | 20:53 |
| === salgado is now known as salgado-afk | ||
| === sinzui changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: abentley || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bdmurray | leonardr: my mp is ready for re-review | 22:37 |
| leonardr | bdmurray: ok, i'll get to it tomorrow morning. remind me if i don't get to it | 22:44 |
| bdmurray | leonardr: okay thanks | 22:46 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | sinzui: I didn't see your branch in the queue. I can do it later today. | 23:24 |
| sinzui | EdwinGrubbs, okay | 23:29 |
| === flacoste is now known as flacoste_afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!