/srv/irclogs.ubuntu.com/2010/04/23/#launchpad-reviews.txt

=== Edwin-lunch is now known as EdwinGrubbs
=== adiroiban changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [adiroiban(UI bug-146178), noodles, bigjools, sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
thumperrockstar: https://code.edge.launchpad.net/~thumper/launchpad/new-code-import-owner/+merge/2397402:28
=== thumper changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
rockstarthumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-index-redux/+merge/2397603:12
abentleythumper, could you please review https://code.edge.launchpad.net/~abentley/launchpad/build-from-recipe-api/+merge/23981 ?04:39
thumperwere we too noisy?04:39
abentleythumper, no, my bowels were too full.04:39
=== jtv1 is now known as jtv
noodles775adiroiban: do you want me to land your bug-487137 now?07:19
adiroibannoodles775: only if you have time :)07:22
noodles775adiroiban: heh, it takes all of 10 seconds ;), and the same for 61081 I assume?07:22
noodles775Thanks for the updates!07:22
adiroibannoodles775: yes. they need to be tested07:23
adiroibannoodles775: can you please also send this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ?07:24
noodles775Sure.07:24
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775adeuring: can I get you to take a look at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-build-farm-job-model/+merge/2391310:20
adeuringnoodles775: sure10:20
noodles775Thanks!10:20
* noodles775 has another one which will follow too.10:20
=== noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
bigjoolsadeuring: hi, you seem to have removed my branch from the queue, unless you reviewed it?11:09
adeuringbigjools:sorry... I think the queue was empty when I joined the channel... I even wasn't aware that you asked for a reveiw...11:10
adeuringbigjools: will look at your branch when I've finished noodles'11:10
bigjoolsadeuring: ah ok, dunno what reset the topic then, it was there earlier!11:11
bigjoolsthanks11:11
noodles775bigjools: when I logged in this morning at 7:00UTC, the queue was empty :/11:15
bigjoolsoh :/11:15
bigjoolsit was there last night, prob what I am thinking of11:15
noodles775yeah, I was surprised too.11:16
adeuringnoodles775: r=me, a few nitpicks11:22
wgrant5 items were mysteriously dropped from the queue 9 hours ago.11:23
adeuringbigjools: i assume this is your branch: https://code.edge.launchpad.net/~julian-edwards/launchpad/delete-ppa-part2/+merge/23967 ?11:24
noodles775Thanks adeuring11:24
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bigjools || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775wgrant: Thanks! Could you pop them back in so other people don't get annoyed? :)11:25
=== wgrant changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bigjools || queue: [noodles, adiroiban(UI bug-146178), sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
bigjoolsadeuring: yes, thanks11:38
adeuringbigjools: a naive question: can't you use "if os.path.exists(root_dir)" instead of catching two exceptions?11:48
noodles775Hi adeuring, I think you forgot to include your comment when you reviewed my branch (instead using the MP description)?11:48
bigjoolsadeuring: I prefer the exceptions, something else may go wrong, like permissions being wrong etc.11:49
bigjoolsadeuring: it seems more robust11:49
bigjoolsbut happy to be persuaded otherwise?11:50
adeuringnoodles775: sorry... stupid copy&paste mistake. I've added a comment11:50
adeuringbigjools: OK. But there may be other errors, like odd permissiono settings preventing the deletion. And now we don't see any specific reason why the deletion failed.11:51
bigjoolsadeuring: it prints the reason in the log11:52
adeuringbigjools: argh, yes, you are right. I did not see the little "e)" at the end of the log.warning() call. Sorry11:52
* adeuring really needs better glasses11:52
bigjoolshehe, np, easily missed11:52
adeuringbigjools: so, r=me11:53
bigjoolsadeuring: great, thanks very much11:53
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles || queue: [adiroiban(UI bug-146178), sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
adeuringnoodles775: r=me12:31
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: [lunch] || queue: [adiroiban(UI bug-146178), sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775Thanks adeuring12:54
=== mrevell is now known as mrevellunch
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: adiroiban(UI bug-146178) || queue: [, sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara-afk is now known as matsubara
* adeuring does not do ui reviews... 13:21
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: sinzui, || queue: [adiroiban(UI bug-146178), adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== mrevellunch is now known as mrevell
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: adiroiban(bug-402235) || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
sinzuiadeuring, thanks for seeing the bug I added to the code. I will reply with a fix soon14:49
sinzuinoodles775, I have an awesome (and I mean that in the historical context) branch that updates CSS. I hope you can find time to review it15:29
noodles775sinzui: great!15:29
sinzuiadeuring, I replied to your review15:34
* adeuring is looking15:34
adeuringsinzui: approved15:39
sinzuiadeuring, thanks15:39
noodles775sinzui: In your MP text, you mention removing the duplicate css for ul/li.language, but afaics MP diff just shows those as moved?15:47
noodles775Wow... the re-org. looks excellent.15:48
sinzuinoodles775, it was defined twice!15:48
noodles775sinzui: but on your MP, I can see it added (from a move) on lines 340ff, removed on lines 1578ff, ah, right, then removed again at 3228. OK.15:50
sinzuinoodles775, I really hope that future additions/changes to the css will be easier with this reorganisation. I lost my evening to an obsessive need to understand all the rules we are using15:52
noodles775sinzui: I think it definitely will... and yes, looking at the organisation I can see why it took an evening!15:52
sinzuinoodles775, have you used the W3C validator? I was looking for a tool in apt and could not find anything as good as the website15:54
henningenoodles775, jelmer: Can you please re-affirm your review for my branch? I want to merge it into devel.15:54
henningehttps://code.edge.launchpad.net/~henninge/launchpad/bug-565294-devel/+merge/2402015:54
noodles775sinzui: not for a few years, I'll give it a whirl next time I do some css work.15:57
noodles775sinzui: another (build) tool I've used in the past for nicer stylesheet organisation is http://sass-lang.com/15:58
sinzuithat looks practical and is compatible with mars' ideas15:59
noodles775I did mention it a while back, but I think the fact that it's ruby killed the discussion (yet more developer dependencies).16:00
sinzuiruby needs another 5 years to evolve into python16:00
noodles775lol16:01
sinzuiIt wanted to be perl done right, but soon learned python done right is even better16:01
sinzuiI like ruby's symbol feature. I hope python steals that idea16:02
noodles775henninge: done.16:04
henningecheers16:04
noodles775mrevell: just a heads-up that I assume your branch will conflict with sinzui's. You may want to wait until his lands and then remerge devel.16:12
mrevellThanks noodles775, I'll leave it until after the weekend in that case.16:13
sinzuimrevell, is your branch styles for help?16:14
mrevellsinzui, Yes16:14
sinzuimrevell, you get your own section now16:14
mrevellsinzui, or, rather, it updates the help style help16:14
mrevellsinzui, https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-heat-help-bug-544799/+merge/2401616:15
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
sinzuimrevell, you may cry. my branch is an extensive reorg. You will may want to update/add the styles by hand.16:16
noodles775It should just be adding the one line for a.help.icon.16:17
mrevellsinzui, Heh, no tears here :) As noodles775 says, it was just a one line change.16:17
sinzuimrevell, you certainly wont have a problem finding your classes in the css.16:17
noodles775:)16:17
mrevellCool :)16:17
=== matsubara is now known as matsubara-lunch
=== adeuring changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara-lunch is now known as matsubara
=== gary_poster is now known as gary-lunch
bachi sinzui, can you take a look at this small chunk i separated out:  https://code.edge.launchpad.net/~bac/launchpad/bug-569101/+merge/2403419:34
* sinzui looks19:35
bacsinzui: oops: s/The Launchpad/The Launchpad object/19:36
sinzuiToo late. I just approved it19:36
sinzuinow you have to land it as it is :)19:36
bacbah19:37
bacthanks, though19:37
=== gary-lunch is now known as gary_poster
bacsinzui: have time for another gem?  https://code.edge.launchpad.net/~bac/launchpad/lplib-testing/+merge/2403820:45
baca melange, if you will20:45
baca cornucopia of code snippets20:45
=== EdwinGrubbs is now known as Edwin-lunch
sinzuioh, I have reviewed the build_doctest_suite before. I glad the work will see the light of day21:02
sinzuibac: I cannot review your proposal. It is read-only21:05
bacwell that's dumb21:05
sinzuibac: you are listed as the only person who can review it21:06
bacweird21:06
bacsinzui: and now?21:06
bacsinzui: i tried to make it depend on my previous branch but it wants to merge into it21:07
baclet me blow away that MP and try again21:08
sinzuioh21:09
sinzuiI was just reviewing it21:09
sinzuibac: sorry you had fixed it21:09
sinzuibac: my only remark about the code was that I think this line is missing punctuation: # Add doctests using default setup/teardown21:10
bacsinzui: sorry, can you try again with https://code.edge.launchpad.net/~bac/launchpad/lplib-testing/+merge/2404021:10
* sinzui reviews the next proposal before a vengeful bac kills it21:11
baci think the first one was doomed21:11
sinzuibac: I want to rename your kanban card  to be about refactoring lplib tests.21:20
bacok21:20
sinzuibac: is your branch done with my review? Can I move the card to Review/Done?21:21
bacyes21:21
Edwin-lunchsinzui, bac: do either one of you want to review my branch. After I took out an assertion, the size of the branch collapsed. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-248518-setPreferredEmail/+merge/2404121:25
sinzuiI will21:25
=== Edwin-lunch is now known as EdwinGrubbs
* sinzui claims it before bac deletes it21:25
EdwinGrubbssinzui: btw, I blame pipeline for the completely unnecessary prereq branch.21:26
sinzuiEdwinGrubbs, I see you found an elegant fix in the factory21:28
sinzuiEdwinGrubbs, I am surprised there is not comparable test for account.activate() in doc/account.21:33
sinzuiI think your test is the only documentation we have21:33
EdwinGrubbssinzui: do you want me to add a test to doc/account?21:34
sinzuino. I think there is a unit test for it. From launchpad's perspective, we care about the person.21:35
sinzuiEdwinGrubbs, My initial though was to move your test, but I think your decision was right. I think your tests tells other launchpad developer how to get a person's account activated21:36
sinzuiEdwinGrubbs, I approved your branch.21:37
EdwinGrubbssinzui: thanks21:38
=== matsubara is now known as matsubara-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!