[05:12] <thumper> mwhudson: a trivial review? https://code.edge.launchpad.net/~thumper/launchpad/move-branch-errors/+merge/31602
[05:21] <mwhudson> thumper: done
[05:25] <thumper> mwhudson: ta
[12:34] <mrevell> Anyone care to review a very small change? it's the removal of text from the LP footer. https://code.launchpad.net/~matthew.revell/launchpad/remove-survey-summer-2010/+merge/31628
[12:40] <jelmer> mrevell-lunch: I'll take it.
[12:57] <mrevell> thanks jelmer
[14:45] <mars> jelmer, ping, any movement on your buildd-trivial branch?  It looks like you asked Lamont to review it?
[14:45] <jelmer> mars: Hi
[14:45] <jelmer> mars: Yep, I've asked for review from him but he's on holidays at the moment. There's no urgency though, just some drive-by cleanups.
[14:46] <mars> ok, any chance someone else could review it to get the work out of the queue?
[14:47] <mars> I looked at it, but it is over my head
[14:48] <jelmer> mars: I'd rather wait for lamont, perhaps I can make it not show up in the general queue somehow though.
[14:48] <mars> only by switching it to "Work in Progress", which would be a hack
[14:48] <mars> jelmer, ok, don't worry about it
[14:49] <jelmer> I've just voted "Abstain" on it, it's no longer waiting for review from launchpad-reviewers now.
[14:49] <mars> oh, neat idea
[14:49] <mars> thanks
[14:57] <adeuring> mars: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-612779/+merge/31637 ?
[14:58] <mars> adeuring, sure, you're next
[14:58] <adeuring> mars: thanks!
[15:07] <mars> adeuring, ping
[15:07] <adeuring> mars: yes?
[15:07] <mars> adeuring, uhm, that class name, StreamOrRedirectLibraryFileAliasView, is very suspicious
[15:08] <adeuring> mars: perhaps... What do you mean by "suspicious"?
[15:08] <mars> if you have AorBorCView, then I would say "use polymorphism and create AView, BView, and CView implementations coming from a factory"
[15:09] <mars> StreamOrRedirectLibraryFileAliasView and SafeStreamOrRedirectLibraryFileAliasView sounds like an object design problem
[15:09] <mars> or a poorly named class
[15:10] <adeuring> mars: sounds reasionable. But lifeless is anyway going to reorganize the whole implementation of acces to restricted LFAs, so I think we can leave this somewhat confusion stuiff as it is for now
[15:10] <mars> adeuring, perhaps you could shed some light on this: is StreamOrRedirectLibraryFileAliasView two classes, or is it once class and the class name is telling you what the implementation does? (Stream, and it might redirect the stream)
[15:11] <adeuring> mars: well, I think it is intended to mean "either stream some content or redirect to another URL"
[15:12] <mars> ok, so an object design problem then
[15:12] <adeuring> probably
[15:12] <mars> if it was just a bad class name, that would be easy to fix
[15:12] <mars> oh well
[15:13] <mars> adeuring, ok, thanks for the explanation.  I'll make a note about it in my review for when lifeless reads it
[15:13] <adeuring> ok
[15:15] <adeuring> mars: this class has other issues as well: It first downloads all data from the Librarian, and once that is done, it begins to deliver the data to the client. Not very cleer for large files...
[15:15] <mars> eeww, yuck
[15:15] <mars> hope the librarian isn't slow that day
[15:16] <mars> oh well
[15:16] <adeuring> mars: but again, lifeless idea is to serve restricted data directly from the Librarian (secured by some token), so that won't be an issue soon
[15:17] <mars> I wouldn't know how to fix it myself anyway.  I don't know if the app servers can flush data in their response buffer or not - you would need that if you were to fix it
[15:17] <mars> interesting.  ok.
[15:19] <adeuring> mars: right
[16:01] <bigjools> mars: hi, are you reviewing?
[16:04] <mars> bigjools, yep!
[16:04] <mars> bigjools, feel free to jump in line
[16:04] <bigjools> mars: excellent, I've got a small branch
[16:04] <bigjools> thanks
[17:11] <mars> bigjools, done
[17:18] <bigjools> mars: thank you
[17:20] <bigjools> mars: so, no, the doctest is absolutely not the right place for that test but I don't have the time to convert that all to unit tests and I am loathe to write a new unit test and not include the existing ones.
[17:20] <bigjools> but honestly, that's quite a nice doctest file by soyuz standards :)
[17:31] <mars> bigjools, I would really prefer if you added just this one test.  For me, finding a home for new tests is a barrier to entry - if you blaze that trail, then everyone can point others in that direction later
[17:32] <bigjools> mars: I really, really hate splitting tests up between files, it's extremely confusing
[17:32] <mars> bigjools, what would be split, exactly?
[17:32] <bigjools> the unit tests
[17:33] <bigjools> it's the crappy situation that we've been trying to eradicate in Soyuz tests for the last three years
[17:33] <mars> they would be split from the doctests, and that's bad?  Or they would be split across N unit test files, and that would be bad?
[17:33] <bigjools> it's so much harder to develop when you start making changes and think you've fixed all the tests only to get a bunch more fail in a file you didn't remember
[17:33] <bigjools> so some would be in the doctests and some in the new unit test we're proposing
[17:33] <bigjools> I would rather move them all at once
[17:34] <mars> bigjools, and when will you or your team have a single block of time to spend rewriting the entire doctest file into a proper doctest/unit test pairing?
[17:35] <bigjools> does it matter?
[17:35] <mars> well, if the answer is "never"... :)
[17:35] <bigjools> we have worse problems to fix
[17:35] <bigjools> but tests split all over the place has pissed me off so much in the past
[17:36] <bigjools> consider me jaded
[17:36] <bigjools> this was supposed to be a quick fix!
[17:37] <mars> I understad, but it is quick at the expense of increasing the team's work a few years down the line, and at the expense of our documentation.
[17:40] <bigjools> I simply don't have the time for this right now, I'm sorry.  I budgeted some time to make this essential fix and now I have 6 other things that all need doing yesterday.
[17:40] <bigjools> I'll pass it on to someone else if you insist on the change.
[17:41] <mars> I do insist.  Land what you have, but please have someone refactor it in a follow-up branch, or build on this one.
[17:42] <mars> bigjools, I'll mark the proposal accordingly so you can land it.
[17:42] <mars> before your EOD
[17:44] <bigjools> mars: thanks
[21:56] <rockstar> mars, can I get a quick review from you? https://code.edge.launchpad.net/~rockstar/launchpad/person-recipe-listings/+merge/31688
[21:57] <mars> rockstar, yep
[21:58] <mars> rockstar, "earma" ?
[21:58] <rockstar> mars, no idea what that's about.  Reverting.
[21:58] <mars> do you use Vim or something?
[21:58] <mars> :)
[21:59] <mars> rockstar, looks good, r=mars.  I am surprised no portlet view tests died on this change.
[21:59] <rockstar> mars, yes, but I also using some fun painkillers, so I might as well be using emacs...
[22:00] <mars> hehe
[22:01] <mars> rockstar, approved