=== Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk [05:12] mwhudson: a trivial review? https://code.edge.launchpad.net/~thumper/launchpad/move-branch-errors/+merge/31602 [05:21] thumper: done [05:25] mwhudson: ta [12:34] 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 === mrevell is now known as mrevell-lunch [12:40] mrevell-lunch: I'll take it. === mrevell-lunch is now known as mrevell [12:57] thanks jelmer === matsubara-afk is now known as matsubara === Ursinha-afk is now known as Ursinha === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:45] jelmer, ping, any movement on your buildd-trivial branch? It looks like you asked Lamont to review it? [14:45] mars: Hi [14:45] 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] ok, any chance someone else could review it to get the work out of the queue? [14:47] I looked at it, but it is over my head [14:48] mars: I'd rather wait for lamont, perhaps I can make it not show up in the general queue somehow though. [14:48] only by switching it to "Work in Progress", which would be a hack [14:48] jelmer, ok, don't worry about it [14:49] I've just voted "Abstain" on it, it's no longer waiting for review from launchpad-reviewers now. [14:49] oh, neat idea [14:49] thanks [14:57] mars: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-612779/+merge/31637 ? === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant || queue: [abeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:58] adeuring, sure, you're next [14:58] mars: thanks! === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:07] adeuring, ping [15:07] mars: yes? [15:07] adeuring, uhm, that class name, StreamOrRedirectLibraryFileAliasView, is very suspicious [15:08] mars: perhaps... What do you mean by "suspicious"? [15:08] if you have AorBorCView, then I would say "use polymorphism and create AView, BView, and CView implementations coming from a factory" [15:09] StreamOrRedirectLibraryFileAliasView and SafeStreamOrRedirectLibraryFileAliasView sounds like an object design problem [15:09] or a poorly named class [15:10] 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] 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] mars: well, I think it is intended to mean "either stream some content or redirect to another URL" [15:12] ok, so an object design problem then [15:12] probably [15:12] if it was just a bad class name, that would be easy to fix [15:12] oh well [15:13] adeuring, ok, thanks for the explanation. I'll make a note about it in my review for when lifeless reads it [15:13] ok [15:15] 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] eeww, yuck [15:15] hope the librarian isn't slow that day [15:16] oh well [15:16] 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] 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] interesting. ok. [15:19] mars: right === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === salgado is now known as salgado-lunch === deryck is now known as deryck[lunch] [16:01] mars: hi, are you reviewing? [16:04] bigjools, yep! [16:04] bigjools, feel free to jump in line [16:04] mars: excellent, I've got a small branch [16:04] thanks === bigjools changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:11] bigjools, done [17:18] mars: thank you [17:20] 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] but honestly, that's quite a nice doctest file by soyuz standards :) === matsubara is now known as matsubara-lunch === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:31] 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] mars: I really, really hate splitting tests up between files, it's extremely confusing [17:32] bigjools, what would be split, exactly? [17:32] the unit tests [17:33] it's the crappy situation that we've been trying to eradicate in Soyuz tests for the last three years [17:33] 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] 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] so some would be in the doctests and some in the new unit test we're proposing [17:33] I would rather move them all at once [17:34] 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] does it matter? [17:35] well, if the answer is "never"... :) [17:35] we have worse problems to fix [17:35] but tests split all over the place has pissed me off so much in the past [17:36] consider me jaded [17:36] this was supposed to be a quick fix! [17:37] 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] 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] I'll pass it on to someone else if you insist on the change. [17:41] 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] bigjools, I'll mark the proposal accordingly so you can land it. [17:42] before your EOD [17:44] mars: thanks === salgado-lunch is now known as salgado === matsubara-lunch is now known as matsubara === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:56] mars, can I get a quick review from you? https://code.edge.launchpad.net/~rockstar/launchpad/person-recipe-listings/+merge/31688 [21:57] rockstar, yep === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:58] rockstar, "earma" ? [21:58] mars, no idea what that's about. Reverting. [21:58] do you use Vim or something? [21:58] :) [21:59] rockstar, looks good, r=mars. I am surprised no portlet view tests died on this change. [21:59] mars, yes, but I also using some fun painkillers, so I might as well be using emacs... [22:00] hehe [22:01] rockstar, approved === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk === Ursinha is now known as Ursinha-brb === Ursinha-brb is now known as Ursinha-afk