=== 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
[09:24] <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:25] <jtv> Ahem... MP is https://code.launchpad.net/~jtv/launchpad/extract-approval-js/+merge/28852
[09:29] <noodles775> Sorry jtv - I'm working to get an urgent feature done right now...
[09:29]  * jtv continues his search for prey
[09:33] <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:42] <rinze> jtv: I can code review, but I'm not a UI reviewer
[09:43] <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:44] <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
[12:46] <rinze> jtv: I haven't forgotten your branch but it's taking a wee bit longer since my JavaScript is a bit rusty.
[12:47] <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:51] <rinze> jtv: Yeah, that'd be useful, though there doesn't appear to be one around at the moment :-(
[12:52] <jtv> ...and there's the catch.
[12:52] <rinze> (I've just been looking at https://dev.launchpad.net/JavaScriptReviewNotes)
[12:54] <jtv> rinze: looking at those myself now... I see that my documentation needs some tweaks.
[12:56] <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:57] <jtv> bigjools: so speed matters... right ho
[12:57] <bigjools> thanks
[12:58] <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:59] <rinze> rinze, ok
[13:00] <bigjools> rinze: how many other nicks have you got? :)
[13:01] <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:04] <rinze> bigjools: just these three :-)
[13:05] <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
=== matsubara-afk is now known as matsubara
[13:15] <jtv> rinze: did you run "make lint" btw?
[13:17] <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:19] <rinze> ok
[13:22] <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:25]  * rinze nods
[13:27] <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:28] <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
=== 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
[13:28] <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
=== 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
[13:29] <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:31] <jtv> rinze: btw I was wondering... what does CannotRestrictArchitectures._fmt do?
[13:35] <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:36] <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:37] <rinze> it's what some of the other exceptions used as well
[13:37] <rinze> jtv: That makes sense
[13:38] <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:49] <jtv> rinze: btw lint doesn't complain about it, but try to avoid trailing whitespace as well.
[13:56] <flacoste> bigjools: what do you need me for?
[14:17] <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:19] <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:20] <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:23] <rinze> heh, ok
[14:31] <jtv> bigjools, line 66 of your diff: s/Lets/Let's/
[14:32] <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:33] <jtv> Do it all in French!
[14:33] <jtv> no wait
[14:33] <bigjools> Enc...
[14:33] <bigjools> no waut
[14:33] <jtv> ?
[14:34] <bigjools> typing is clearly not my strong point today
[14:35] <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:36] <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:37] <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:38] <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:40] <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:41] <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:43] <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
=== 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
[14:55]  * jtv is taking a break
=== 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
[15:17] <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
=== 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
[16:04] <EdwinGrubbs> jtv-afk: what does "test_getVariable_smoke" mean? I don't see anything called "smoke".
=== jtv-afk is now known as jtv
[16:32] <jtv> EdwinGrubbs: smoke test :)
[16:34] <EdwinGrubbs> I kinda guessed, but it seemed a little vague.
[16:35] <EdwinGrubbs> jtv: review sent
[16:35] <jtv> EdwinGrubbs: thanks!
=== 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
[17:20] <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:22] <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:23] <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:24] <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:25] <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:26] <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:27] <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:28] <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.
=== deryck is now known as deryck[lunch]
[17:30] <jtv> It's one-to-many... but you can still "group by"
[17:31] <jtv> select sourcepackagename.id, sum(messagecount) from sourcepackagename join potemplate on potemplate.sourcepackagename = sourcepackagename.id group by sourcepackagename.id;
[17:35] <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:38] <rinze> jtv: I've pushed an updated version of my branch
[17:39] <jtv> EdwinGrubbs: in POTFileImporter
[17:39] <jtv> rinze: cool, thanks... I'll get on it in a minute
[17:41] <jtv> rinze: in ll.66—67, could you capitalize and punctuate the error string?
[17:42] <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:43] <rinze> doh
[17:43] <jtv> rinze: anyway, with that one remark, I'm approving.  Please give me a moment to document.
[17:47] <jtv> rinze: done
[17:47] <rinze> jtv: thanks!
[17:47] <jtv> np
[17:47]  * jtv is about done
=== 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
[17:56] <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:57] <EdwinGrubbs> rinze: sure
[17:58] <rinze> EdwinGrubbs, https://code.edge.launchpad.net/~jelmer/launchpad/stable-fix/+merge/28912
=== deryck[lunch] is now known as deryck
[18:11] <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:12] <rinze> EdwinGrubbs: It merges stable into db-devel so it's got all the other changes from stable as well
=== matsubara-lunch is now known as matsubara
[18:15] <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:19] <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:23] <rinze> EdwinGrubbs, the two files are lib/lp/soyuz/{configure.zcml,model/archive.py}
[18:30] <EdwinGrubbs> rinze: what editor do you use?
[18:30] <rinze> EdwinGrubbs, vi
=== jtv is now known as jtv-afk
[18:32] <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:33] <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:34] <EdwinGrubbs> setlocal list listchars=tab:>~,trail:*
[18:35] <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:39] <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:52] <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
[19:16] <abentley> EdwinGrubbs, could you please review https://code.edge.launchpad.net/~abentley/launchpad/fix-stuck/+merge/28924 ?
[19:19] <EdwinGrubbs> abentley: sure
=== 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
[19:35] <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:36] <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:38] <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:42] <abentley> EdwinGrubbs, how would that be a benefit?
[19:45] <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:52] <EdwinGrubbs> abentley: r=me
[19:53] <abentley> EdwinGrubbs, thanks!
=== matsubara is now known as matsubara-afk
[20:49] <abentley> rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/process-upload-no-cwd/+merge/28682 ?
[20:53] <rockstar> abentley, ack
=== 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
[22:37] <bdmurray> leonardr: my mp is ready for re-review
[22:44] <leonardr> bdmurray: ok, i'll get to it tomorrow morning. remind me if i don't get to it
[22:46] <bdmurray> leonardr: okay thanks
=== 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
[23:24] <EdwinGrubbs> sinzui: I didn't see your branch in the queue. I can do it later today.
[23:29] <sinzui> EdwinGrubbs, okay
=== flacoste is now known as flacoste_afk