/srv/irclogs.ubuntu.com/2010/08/03/#launchpad-reviews.txt

=== Ursinha-afk is now known as Ursinha
=== Ursinha is now known as Ursinha-afk
thumpermwhudson: a trivial review? https://code.edge.launchpad.net/~thumper/launchpad/move-branch-errors/+merge/3160205:12
mwhudsonthumper: done05:21
thumpermwhudson: ta05:25
mrevellAnyone 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/3162812:34
=== mrevell is now known as mrevell-lunch
jelmermrevell-lunch: I'll take it.12:40
=== mrevell-lunch is now known as mrevell
mrevellthanks jelmer12: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
marsjelmer, ping, any movement on your buildd-trivial branch?  It looks like you asked Lamont to review it?14:45
jelmermars: Hi14:45
jelmermars: 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
marsok, any chance someone else could review it to get the work out of the queue?14:46
marsI looked at it, but it is over my head14:47
jelmermars: I'd rather wait for lamont, perhaps I can make it not show up in the general queue somehow though.14:48
marsonly by switching it to "Work in Progress", which would be a hack14:48
marsjelmer, ok, don't worry about it14:48
jelmerI've just voted "Abstain" on it, it's no longer waiting for review from launchpad-reviewers now.14:49
marsoh, neat idea14:49
marsthanks14:49
adeuringmars: 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
marsadeuring, sure, you're next14:58
adeuringmars: 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
marsadeuring, ping15:07
adeuringmars: yes?15:07
marsadeuring, uhm, that class name, StreamOrRedirectLibraryFileAliasView, is very suspicious15:07
adeuringmars: perhaps... What do you mean by "suspicious"?15:08
marsif you have AorBorCView, then I would say "use polymorphism and create AView, BView, and CView implementations coming from a factory"15:08
marsStreamOrRedirectLibraryFileAliasView and SafeStreamOrRedirectLibraryFileAliasView sounds like an object design problem15:09
marsor a poorly named class15:09
adeuringmars: 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 now15:10
marsadeuring, 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
adeuringmars: well, I think it is intended to mean "either stream some content or redirect to another URL"15:11
marsok, so an object design problem then15:12
adeuringprobably15:12
marsif it was just a bad class name, that would be easy to fix15:12
marsoh well15:12
marsadeuring, ok, thanks for the explanation.  I'll make a note about it in my review for when lifeless reads it15:13
adeuringok15:13
adeuringmars: 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
marseeww, yuck15:15
marshope the librarian isn't slow that day15:15
marsoh well15:16
adeuringmars: but again, lifeless idea is to serve restricted data directly from the Librarian (secured by some token), so that won't be an issue soon15:16
marsI 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 it15:17
marsinteresting.  ok.15:17
adeuringmars: right15: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]
bigjoolsmars: hi, are you reviewing?16:01
marsbigjools, yep!16:04
marsbigjools, feel free to jump in line16:04
bigjoolsmars: excellent, I've got a small branch16:04
bigjoolsthanks16: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
marsbigjools, done17:11
bigjoolsmars: thank you17:18
bigjoolsmars: 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
bigjoolsbut 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
marsbigjools, 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 later17:31
bigjoolsmars: I really, really hate splitting tests up between files, it's extremely confusing17:32
marsbigjools, what would be split, exactly?17:32
bigjoolsthe unit tests17:32
bigjoolsit's the crappy situation that we've been trying to eradicate in Soyuz tests for the last three years17:33
marsthey 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
bigjoolsit'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 remember17:33
bigjoolsso some would be in the doctests and some in the new unit test we're proposing17:33
bigjoolsI would rather move them all at once17:33
marsbigjools, 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
bigjoolsdoes it matter?17:35
marswell, if the answer is "never"... :)17:35
bigjoolswe have worse problems to fix17:35
bigjoolsbut tests split all over the place has pissed me off so much in the past17:35
bigjoolsconsider me jaded17:36
bigjoolsthis was supposed to be a quick fix!17:36
marsI 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
bigjoolsI 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
bigjoolsI'll pass it on to someone else if you insist on the change.17:40
marsI do insist.  Land what you have, but please have someone refactor it in a follow-up branch, or build on this one.17:41
marsbigjools, I'll mark the proposal accordingly so you can land it.17:42
marsbefore your EOD17:42
bigjoolsmars: thanks17: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
rockstarmars, can I get a quick review from you? https://code.edge.launchpad.net/~rockstar/launchpad/person-recipe-listings/+merge/3168821:56
marsrockstar, yep21: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
marsrockstar, "earma" ?21:58
rockstarmars, no idea what that's about.  Reverting.21:58
marsdo you use Vim or something?21:58
mars:)21:58
marsrockstar, looks good, r=mars.  I am surprised no portlet view tests died on this change.21:59
rockstarmars, yes, but I also using some fun painkillers, so I might as well be using emacs...21:59
marshehe22:00
marsrockstar, approved22: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!