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

=== Ursinha-afk is now known as Ursinha
jtvthumper: on what server are ITipChanged events fired and handled?03:49
thumperjtv: only within the branch scanner process03:50
jtvSo that's where the buildfarmjobs all get created then, I guess.03:50
jtvWhat server does that run on?03:50
jtvIs that codehosting, or the script server?03:50
jtvthumper: (I need to know for a CP request)03:51
jtvthumper: I can guess it's codehosting, but this isn't a moment for guessing.  :)03:55
thumperloganberry03:55
jtvSo the script server!  Thanks.03:56
mwhudsonthumper: https://code.edge.launchpad.net/~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary/+merge/3372805:05
mwhudsonthumper, jtv: can you take a look at https://code.edge.launchpad.net/~mwhudson/launchpad/initialize-bzrlib-in-ec2-bug-624434/+merge/33730 ?05:39
* thumper looks05:40
* jtv goes off to look05:40
thumperwhy does it still say updating diff?05:41
jtvthumper: it's been doing that lately… for perhaps two hours at a time.05:41
thumperseems screwy05:41
* thumper wonders if there is a bug filed05:42
jtvmwhudson: is with still future?05:42
thumperjtv: it is for 2.505:42
mwhudsonjtv: don't we still deploy on 2.5 ?05:42
mwhudsonfor this specific file i guess it doesn't matter05:42
mwhudsonthumper: it doesn't say updating diff for me05:43
jtvWe still have stuff on dapper afaik, so that makes sense05:43
thumperreplication lag?05:43
jtvmwhudson: it's gone now05:43
mwhudsonthumper: ta05:44
jtvthumper already approved it?05:44
jtvah yes05:44
jtvmwhudson: under what circumstances does one need to initialize bzrlib?  I'm wondering if we need to do the same in our buildfarm jobs.05:45
mwhudsonjtv: we probably should, yeah05:45
mwhudsoni don't think it's a specific requirement yet05:45
jtvyet?05:46
mwhudsonwell05:46
mwhudsoni suspect as bzr changes, it will become more necessary05:46
mwhudsonor less necessary, if the people with pitchforks win out i guess05:46
jtvheh05:49
* jtv imagines a tough Hudson going "what kind of idiot brings a pitchfork to a flamewar"05:50
lifelesswell right now if you don't it won't log properly06:13
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvnoodles775, got one for ya: https://code.edge.launchpad.net/~jtv/launchpad/recife-new-resetcurrenttranslation/+merge/3374710:18
jtvI warn you, not for the faint of heart.10:18
jtvNot for the code per se; more for the murder mystery.10:18
jtv…and having thus ensured rapt attention from my potential reviewer, I trundle off to other work.10:19
noodles775:)10:22
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvoh, hi benji!11:18
jtvdid I mention you really did free us of our one daily-recurring oops?11:18
benjijtv: awesome!11:19
jtvvery11:19
benjijelmer: for some reason ec2 land doesn't think you approved https://code.edge.launchpad.net/~benji/launchpad/check-in-wadl/+merge/33661: "ec2: ERROR: Merge proposal is not approved. Get it approved, or use --force to land it without approval."11:23
benjiI'm about to go AFK, I suppose I'll use --force when I get back if you don't have a better idea.  Thanks.11:23
jelmerbenji: Sorry, I had voted approved but not marked the whole MP as approved. Done now.11:26
noodles775jtv: on line 288 of the MP diff, why do you remove the assertion?12:03
jtvnoodles775: not removing it—that's just an awkward slicing of the diff.  Just replacing it with a clearer one: is_diverged.12:04
jtvThat's a property that returns "potemplate is not None"12:04
noodles775Right, thanks.12:05
salgadojtv, around?  did you see my reply to those two reviews you did for me yesterday?12:27
jtvsalgado: ah, I knew there was something I wanted to check up on…12:27
jtvsalgado: ah, and of course I never saw the most important part in my email.12:28
salgado:)12:29
jtvThat's why your later follow-up didn't make any sense to me.  I never saw the part it was a follow-up to.12:30
jtvsalgado: I _would_ be looking at your additional revisions at this point if codebrowse were willing to show them to me.12:30
salgadojtv, I can generate the incr. diffs here if you'd like12:31
jtvsalgado: nah, at some point it'll start working again, I know it12:32
jtvSee?  I've already gone from "Please try again" to "This website is not available."12:32
jtvEr… hang on12:32
jtvsalgado: got 'em.  Some pretty big changes there; I won't be able to review them tonight.  Will tomorrow do?12:51
jelmernoodles775: Can I add a trivial soyuz branch to the queue ?12:51
salgadojtv, big changes?  which ones?  I thought they were all small-ish12:52
jelmerStevenK: did you see my followup to the merge proposals you reviewed the other day?12:52
jtvsalgado: isn't this one of your post-review changes?  http://bazaar.launchpad.net/~salgado/launchpad/remove-security-upload-policy/revision/1142712:52
salgadojtv, no, that's the initial diff you've reviewed12:53
jtvah good12:53
jtv(before you say it: no, I did _not_ bother to read it :)12:54
salgadohttp://bazaar.launchpad.net/~salgado/launchpad/remove-security-upload-policy/revision/1142812:54
salgadothat's the one you haven't seen yet12:54
salgadojtv, 11427 is the mostly red diff, where I removed tons of stuff12:54
jtvsalgado: 11428 is still giving me about 7 pages.  Dinner just arrived; I'll look at it tomorrow.12:56
jtvThe code, not the dinner.12:56
salgadojtv, do you mind if I get someone to review these other bits, then?12:57
jtvsalgado: not at all.12:57
salgadocool, I'll do that, then12:57
jtvok12:57
=== matsubara-afk is now known as matsubara
=== henninge_ is now known as henninge
salgadohi noodles775.  care to do a couple follow up reviews for me?  they're both quite simple13:14
noodles775salgado: sure!13:18
salgadocool, thanks13:19
=== salgado changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jtv || queue: [salgado*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jelmer || queue: [salgado*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jelmernoodles775: the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/remove-archiveuploader-utils/+merge/3376413:22
noodles775jelmer: yep, just merged it (for some reason the diff not yet generated on the MP)13:23
noodles775jelmer: r=me :) Nice and easy.13:24
jelmernoodles775: thanks :-)13:24
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: salgado || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
salgadonoodles775, so, I think it's a good idea to take https://code.edge.launchpad.net/~salgado/launchpad/remove-security-upload-policy/+merge/33581 first13:26
noodles775k, thanks.13:26
leonardrnoodles775, i need a one-line review, can i sneak in before salgado's next review?13:26
salgadojtv's reviewed it, but there were test failures on the revision he reviewed13:26
noodles775leonardr: sure, if it really is a one-liner ;)13:26
leonardrit is13:27
leonardrnoodles, take a look at http://bazaar.launchpad.net/~leonardr/launchpad/optimized-length-2/revision/11392 which is my last reviewed code for this branch13:28
leonardrthere's some code that sets resultset._select.*13:29
leonardrthe tests failed because not all ResultSet objects have ._select, only the Storm ones13:29
leonardrso i added this line:13:30
leonardrif hasattr(resultset, '_select'):13:30
noodles775salgado: So, looking at r11428 (the unreviewed revision), it adds TestStagedBinaryUploadBase back in, but not TestStagedSecurityUploads?13:32
salgadonoodles775, right, I've only added it back in because test_buildduploads.py needs TestStagedBinaryUploadBase13:33
gmballenap, Can I get an r[s]= from you to merge that branch from yesterday into devel. I merged it into db-devel by mistake.13:33
salgadonoodles775, I've removed TestStagedSecurityUploads because that's a test for the 'security' upload policy, which the branch removed13:33
noodles775OK.13:33
* noodles775 reads the MP13:34
gmballenap, Actually, I suspect I don't need to ask you for *another* r=, since the code's the same :)13:34
salgadonoodles775, and I've added a new unit test (in the other MP) to exercise the same code that was exercised by the doctest chunk I've removed13:40
noodles775salgado: The doctest snippet that you removed in r11428 (reject instead of fail for non-existent pocket), you mention that you'll cover it with a unit test in the following branch...13:40
noodles775lol13:40
noodles775but13:40
salgado:)13:40
salgadohttp://bazaar.launchpad.net/~salgado/launchpad/vostok-upload-policy/revision/1143213:41
noodles775as far as I can see, the following branch tests that an assertion is raised, but not that a rejection is sent13:41
* noodles775 double checks, guessing that he missed it.13:41
salgadothat's correct13:41
noodles775Is there a test elsewhere that ensures when the assertion is raised an email is sent?13:42
salgadoI thought just checking for the exception was good enough, as we have plenty of tests showing a rejection is sent when there's an exception13:42
salgadoI could be wrong13:42
noodles775OK.13:42
jmlnoodles775, would you please review https://code.edge.launchpad.net/~jml/launchpad/one-email-on-crash/+merge/33768 ?13:44
jmlthere appears to be some glitch on production stopping diffs from being generated.13:44
jmlI guess I should escalate to a losa13:44
noodles775jml: sure.13:44
jmlnoodles775, thanks.13:45
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: leonardr || queue: [salgado, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775leonardr: I don't see a one-liner? It's r11442 that needs to be reviewed right? This: http://bazaar.launchpad.net/~leonardr/launchpad/optimized-length-2/revision/1144213:51
jmlyay, emails.13:55
leonardrnoodles775, 11442 corrects a typo made to the original one-liner. i was hoping to just give you the code so it wouldn't be so complicated13:59
leonardri'll just do another merge proposal and get in the queue13:59
noodles775leonardr: I'm happy to review just the one-line that's changed, but currently I can only see it in 11442 along with a few other changes. If the rest is already reviewed, just paste a diff and I can review that, otherwise yes, a new MP is probably easier.14:01
leonardrnoodles775: i see the problem. i had to create a new branch with the old changes, so all the old changes show up as one revision14:04
leonardrthis code is the only code that hasn't been reviewed yet:14:04
leonardr+        if hasattr(resultset, '_select'):14:04
leonardr+            resultset._select.limit = Undef14:04
leonardr+            resultset._select.offset = Undef14:04
noodles775leonardr: r=me (I assume the second two lines were there previously, just not indented). Thanks!14:05
leonardrnoodles, yeah14:05
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775jml: why do you set test_directory when it's passed as None?14:51
noodles775(that is, why do you set it to a non-existent directory)14:52
jmlnoodles775, umm, not sure. maybe because stuff explodes.14:52
* jml tries.14:52
noodles775According to the docs, I'd assume it wouldn't explode when its None, but will in its current state?14:53
noodles775But yes, science is the best answer :)14:53
salgadothanks for the reviews, noodles77514:54
noodles775jml: ah, you just moved that code... I'd not realised. But still worth checking.14:55
noodles775salgado: np.14:55
jmlnoodles775, yeah, deleting that bit works.14:56
noodles775jml: OK, the other option would be to make it a required arg (which I guess was the original intent?)14:57
jmlnoodles775, well, it's a required arg of the constructor, but most tests just don't care about it.15:02
noodles775k15:02
jmlnoodles775, so I'd rather leave it optional in the make_foo() method15:02
noodles775Great.15:02
=== noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== salgado is now known as salgado-lunch
=== Ursinha is now known as Ursinha-lunch
=== deryck is now known as deryck[lunch]
=== benji is now known as benji-lunch
=== salgado-lunch is now known as salgado
=== matsubara is now known as matsubara-lunch
=== Ursinha-lunch is now known as Ursinha
=== benji-lunch is now known as benji
=== deryck[lunch] is now known as deryck
=== matsubara-lunch is now known as matsubara
leonardrsalgado, i have a branch at https://code.launchpad.net/~leonardr/lazr.restful/its-really-read-only/+merge/33820 that you might enjoy reviewing19:54
leonardr(or anyone, really)19:55
salgadoleonardr, sure, I'll take it19:57
leonardrsalgado, thanks19:58
derycksinzui, could I get your review (code/ui) of my CSS changes?19:59
sinzuiyep19:59
derycksinzui, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/good-god-comment-fonts-yall/+merge/3382220:00
sinzuideryck, do you know what uses p.search-results?20:03
derycksinzui, looked to be things like suggestions of projects when registering a new project.  I saw no difference when testing locally, so saw no harm in making all these 116% changes together.20:04
sinzuiI'll look into that. that implies something else redefined the style. maybe we do not need it20:05
salgadotagged_as_readonly = ('readonly' in tags and tags.get('readonly'))20:10
salgadoif (readonly and not tagged_as_readonly):20:10
salgado    raise TypeError(...)20:10
salgadoif not readonly and tagged_as_readonly:20:10
salgado    readonly = True20:10
salgadoleonardr, how about that?20:11
salgadothat won't work, I think20:12
salgadoyeah, it will raise TypeError if the field is readonly and there's no readonly tag20:13
sinzuideryck, I think I see YUI font css on the body tag. I see our font rules are work, but YUI is also defining font-size and line-height20:16
sinzuiI hate YUI20:16
deryckyeah, it made this work much harder than it should have been, IMHO20:16
deryckand I generally love YUI20:16
sinzuiyep, the lines spacing on bug descriptions is easy to read when I disable the YUI rule20:17
deryckI noticed in this work the great mis-match in descriptions and the rest of the page20:18
leonardrsalgado, fine20:18
sinzuibut why is it's rule *after* ours?20:18
salgadoleonardr, that won't work, but I have another idea I'm suggesting in my review20:18
leonardrsalgado: i can just change the names around so i don't redefine 'readonly'20:18
derycksinzui, you mean the description's rule?  I assumed it was set via JS on the fly.  But I didn't look into it.20:20
sinzuino, webkit provides an inspector that shows the order of rules, and it lets you toggle them20:20
sinzuiI thought it was the widget too, but it only applies margin rules20:21
deryckwell, there are the rules in the stylesheets in lazr-js.  That's what I meant.20:23
deryckwell, part of what I meant/thought.  Not saying it correctly. :-)20:23
deryckI'm hesitant to change the CSS on the description widget for this branch, just because it's quite fiddling to get all the spacing and alignment correct.20:24
jcsackettEdwinGrubbs: did you/will you have a chance to look at the update to my MP?20:54
EdwinGrubbsjcsackett: I actually just started to look at it.20:56
jcsackettEdwinGrubbs: fantastic. :-)20:56
EdwinGrubbsjcsackett: I commented on one more thing that I want changed. I marked it Approved, so you don't have to reply on the MP after you make the changes.21:18
jcsackettEdwinGrubbs: sounds good. thanks, man.21:20
=== matsubara is now known as matsubara-afk

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