| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| thumper | mwhudson: a trivial review? https://code.edge.launchpad.net/~thumper/launchpad/move-branch-errors/+merge/31602 | 05:12 |
|---|---|---|
| mwhudson | thumper: done | 05:21 |
| thumper | mwhudson: ta | 05:25 |
| 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:34 |
| === mrevell is now known as mrevell-lunch | ||
| jelmer | mrevell-lunch: I'll take it. | 12:40 |
| === mrevell-lunch is now known as mrevell | ||
| mrevell | thanks jelmer | 12:57 |
| === 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 | ||
| 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:45 |
| mars | ok, any chance someone else could review it to get the work out of the queue? | 14:46 |
| mars | I looked at it, but it is over my head | 14:47 |
| 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:48 |
| 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:49 |
| adeuring | mars: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-612779/+merge/31637 ? | 14:57 |
| === 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 | ||
| mars | adeuring, sure, you're next | 14:58 |
| adeuring | mars: thanks! | 14:58 |
| === 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 | ||
| mars | adeuring, ping | 15:07 |
| adeuring | mars: yes? | 15:07 |
| mars | adeuring, uhm, that class name, StreamOrRedirectLibraryFileAliasView, is very suspicious | 15:07 |
| 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:08 |
| mars | StreamOrRedirectLibraryFileAliasView and SafeStreamOrRedirectLibraryFileAliasView sounds like an object design problem | 15:09 |
| mars | or a poorly named class | 15:09 |
| 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:10 |
| adeuring | mars: well, I think it is intended to mean "either stream some content or redirect to another URL" | 15:11 |
| 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:12 |
| 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:13 |
| 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:15 |
| 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:16 |
| 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:17 |
| adeuring | mars: right | 15:19 |
| === 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] | ||
| bigjools | mars: hi, are you reviewing? | 16:01 |
| 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 | 16:04 |
| === 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 | ||
| mars | bigjools, done | 17:11 |
| bigjools | mars: thank you | 17:18 |
| 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:20 |
| === 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 | ||
| 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:31 |
| 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:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| bigjools | consider me jaded | 17:36 |
| bigjools | this was supposed to be a quick fix! | 17:36 |
| 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:37 |
| 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:40 |
| 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:41 |
| mars | bigjools, I'll mark the proposal accordingly so you can land it. | 17:42 |
| mars | before your EOD | 17:42 |
| bigjools | mars: thanks | 17:44 |
| === 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 | ||
| rockstar | mars, can I get a quick review from you? https://code.edge.launchpad.net/~rockstar/launchpad/person-recipe-listings/+merge/31688 | 21:56 |
| mars | rockstar, yep | 21:57 |
| === 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 | ||
| 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:58 |
| 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... | 21:59 |
| mars | hehe | 22:00 |
| mars | rockstar, approved | 22:01 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!