#launchpad-reviews 2010-02-08
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/get-branch-by-date/+merge/18816
<thumper> it is pretty simple
<mwhudson> thumper: continuing your lounge tour?
<thumper> mwhudson: yeah
<thumper> in CHC now
<thumper> I have about an hour before the next boarding call
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jpds changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [jpds(518232)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jpds: Hi! I only noticed your queue entry by chance. You should always poke me (the OCR) when you want a review ... ;-)
<henninge> Unfortunately, I will be afk for a little while now, but I'll look at it afterwards.
<noodles775> \w 13
<jpds> heninge-afk: Sorry about that.
<jpds> noodles775: Hi, can I have a UI review: https://code.launchpad.net/~jpds/launchpad/fix_518385/+merge/18812
<noodles775> Hi jpds, afaik, sinzui is keen to build up UI reviews (assuming we're still going to do some sort of UI review process ourselves).
<noodles775> jpds: so please request one from him on the MP... I'll take a look at it after that.
<jpds> noodles775: Will do.
<henninge> jpds: No need to be sorry, I am looking at it now.
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: jpds(518232) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> henninge: That's another MP. :)
<henninge> jpds: I know, I was referring to your earlier "sorry about that" ;-)
<henninge> jpds: I am looking at this now: https://code.edge.launchpad.net/~jpds/launchpad/fix_518232/+merge/18778
<jpds> henninge: Cool.
<noodles775> hi henning, when you're free, can you review the following? https://code.edge.launchpad.net/~michael.nelson/launchpad/494391-ugly-upload-error-message/+merge/18831
<henninge> noodles775: sure, just stick it in the queue
* noodles775 changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: jpds(518232) || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jpds: I don't see any pre-imp information in your MP.
<henninge> jpds: Who from the registry team did you talk to about this?
<jpds> henninge: sinzui in millbank.
<henninge> jpds: face-to-face, sweet ;)
<henninge> jpds: I find the test a bit hard to read. Couldn't it be made to focus on what is being restricted?
<henninge> jpds: by adding a few more "...", for example?
<jpds> henninge: Could do that.
<henninge> jpds: at least on the repeats.
<henninge> jpds: cool, that's all I could find to complain about ... ;-) r=me
<jpds> henninge: Something like http://pastebin.ubuntu.com/371650/ ?
<henninge> jpds: yes, that looks good ;-)
<jpds> It doesn't seem to like it: http://pastebin.ubuntu.com/371653/
<henninge> jpds: you have consecutive "..." in there. Sorry, didn't look close enough.
<jpds> henninge: On which line?
<henninge> jpds: there is a new line and white space in between but still ...
<henninge> jpds: oh, also I think leading "..." aren't allowed either.
<henninge> jpds: or maybe that is the only problem, and consecutive ones are allowed.
<henninge> Try it out ... ;-)
<henninge> jpds: maybe like this? http://pastebin.ubuntu.com/371658/
<salgado> jml, around? I'd like to know what you think of https://code.launchpad.net/~salgado/launchpad/kill-rogue-ec2-instances/+merge/18834
<henninge> jpds: oh no, like this I meant ... ;) http://pastebin.ubuntu.com/371659/
<jml> salgado, I'll take a look
<jpds> henninge: Pushed changes to test and good to go. :)
<henninge> jpds: ok, so it was the leading "..." ... ;-) Thanks for trying that out.
<jml> salgado, done
<henninge> noodles775: review sent.
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> jml, thanks, but I just realized prepare_tests() is also called when setting up a demo instance, so I'll have to either more the new code to a different method which is only called when running the test suite or use a conditional
<jml> salgado, ahh ok
<jml> salgado, let me have a look
<salgado> run_tests(), maybe?
<jml> salgado, that'd work. I was thinking maybe giving EC2TestRunner's constructor a timeout parameter, and passing that in from builtins.py
<salgado> yeah, that'd be nicer
<jml> salgado, the EC2TestRunner class needs to be exploded.
<noodles775> Thanks henninge
<salgado> jml, check out the updated diff in the mp.  this one doesn't schedule a shutdown on demo instances
<jml> salgado, looking...
<jml> salgado, Looks good. I would have picked seconds, rather than minutes.
<salgado> jml, do you think we need that much granularity?
<jml> salgado, no, I just am unused to seeing computers talk about minutes ):
<jml> I meant to say ":)"
<jml> salgado, no big deal
<salgado> cool. :)
<jpds> henninge: Could you ec2 / land the branch for me please.
<henninge> jpds: yes sure. Did set a commit message?
<henninge> *you*  ;)
<jpds> henninge: Yep.
<henninge> jpds: cool, will land it now.
<bac> henninge: can i add a review to the queue?
<bac> https://code.launchpad.net/~bac/launchpad/bug-514447-anon-api/+merge/18842
 * bac assumes 'yes'
* bac changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: lunch || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bac: please do
<bac> thanks
<henninge> oh, done ;-)
<henninge> np, my job here ... ;)
<allenap> henninge: Fancy another one?
<allenap> https://code.edge.launchpad.net/~allenap/launchpad/twisted-threading-bug-491870/+merge/18843
* allenap changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: lunch || queue [bac, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> allenap: sure
<allenap> henninge: Thanks :)
* abentley changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: lunch, bac || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> abentley: Hi! ;)
* henninge changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: allenap, bac || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> glad I looked here again before starting on bac's branch ... ;)
<henninge> abentley: Hi! ;)
<henninge> glad I looked here again before starting on bac's branch ... ;)
<abentley> henninge, hi.
<abentley> bac, your branch looks reasonable.  Did you actually make any changes to set_attributes, or is it just formatting?
<bac> abentley: formatting.  that section was all on one super-long line
<abentley> bac, cool.  I think the second line needs one more space (before translation_focus).
<bac> ok
<abentley> bac, also, should that be alphabetically sorted?
<bac> abentley: i don't know that sorting is a requirement.  i didn't pay any attention to the content, just did the reformatting.
<abentley> bac, okay.
<abentley> bac, r=me.
<bac> great, thanks
<abentley> bac, another way of handling the AnonymousAuthorization issue would be to merge jamal's branch into yours and then set it as a prerequisite branch.
<bac> abentley: yeah, i realized i should've done that.
<abentley> bac, just thought I should mention because prerequisite branches are newish.
* abentley changed the topic of #launchpad-reviews to: henninge, abentley || reviewing: allenap, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> you can define prerequiesite branches now? that's pretty cool
<bac> abentley: actually, could you explain how to create pre-req branch relationship?
<abentley> bac, when you are working on branch FOO, but that needs changes from branch BAR, you either branch FOO from BAR or merge BAR into FOO.  When you create the merge proposal, you specify that BAR is the prerequisite branch.
<bac> abentley: can you do that last step using 'bzr send'?
<abentley> bac, no.
<abentley> bac, you can do it with lp-submit (from pipeline), though.
<bac> abentley: can you do that even if not using pipelines?  if so, is lp-submit replacing send in general?
<abentley> bac, lp-submit doesn't require pipeslines, but the prerequisite is currently hardwired to be the previous pipe.
<henninge> allenap: I thought lambda func were nothedl in high regard inLP code?
<abentley> bac, I intend for lp-submit to replace send, and I have a branch that adds it to bzr itself.
<bac> abentley: ok.
<allenap> henninge: In Twisted it's a very common pattern. I don't think I'll get shot for that.
<henninge> allenap: I am the one with the gun here, though ... :-P
<allenap> henninge: Heh :)
<henninge> ;)
<henninge> allenap: I have to admit, I lack Twisted familiarity here ...
<allenap> henninge: There are so many callbacks that to write a def for each one gets old very quickly. Using lambdas instead is, I think, accepted practice. Even with the `lambda ignore: ...` pattern.
<allenap> henninge: Actually, I can probably change it to use functools.partial. Let me see.
<allenap> henninge: Ah, no I can't.
<henninge> allenap: I am not too happy with letting another projects coding styles leak into ours.
<henninge> allenap: I think the real question is if we are/will be using Twisted a lot or if this is an isolated case.
<henninge> allenap: I'd like a second opinion on this.
<henninge> abentley: do you have a minute?
<abentley> henninge, sure.
<allenap> henninge: The code team use Twisted quite a lot and I see several uses of it in lp.code and lp.codehosting.
<henninge> abentley: we are discussing the use of lambda, which our coding styles forbid but seem to be in heavy use with Twisted.
<allenap> henninge: Actually, it's probably not worth arguing about this too much. I'm happy to put the calls into a def and be done with it. That way we're both happy :)
<abentley> henninge, I wasn't aware that our coding styles forbid it.
<henninge> abentley: https://dev.launchpad.net/PythonStyleGuide#Use%20of%20lambda,%20and%20operator.attrgetter
<henninge> allenap: sorry to be PITA about it but the use of lambdas goes against explicit coding styles, so I like make sure to DRT.
<henninge> DTRT
<henninge> :-)
<henninge> allenap: yes, with that I'd be happy :)
<abentley> henninge, I think when coding style makes writing code painful, it should be abandoned.
<henninge> abentley: I agree, that's why I raised the question if this is just an isolated case or if it happens regularly.
<henninge> If it's just a single case, the extra pain is neglectable, otherwise we should adapt our coding styles.
<abentley> henninge, with Twisted, you have to define many more callables than with normal python.
<abentley> henninge, often these callables merely adapt existing functions for Twisted use.
<henninge> abentley, allenap: I will put the request to update the coding styles for Twisted code on the reviewers' meeting agenda.
<allenap> henninge: Cool.
<abentley> henninge, so lambdas are pretty useful when writing Twisted code.
<henninge> allenap: leave it as it is for now.
<allenap> henninge: Okay.
<henninge> abentley: I see.
<abentley> henninge, also, the purpose of the callables is to act as callbacks, not to support code reuse.
<henninge> abentley: that is a good point to support this exception.
<abentley> henninge, so it's really a different scenario.
<henninge> allenap: sorry, got a bit distracted ...
<allenap> henninge: :)
<henninge> allenap: Why did you definen a parameter "install_signal_handlers"?
<allenap> henninge: Otherwise there's an error in, I think, checkwatches-cli-switches.txt for a layering violation; SIGCHLD handler left behind.
<henninge> allenap: I mean, why is it configurable?
<henninge> allenap: so, are you saying setting it to "False" would produce an error?
<allenap> henninge: Just to make it work for testing. I could, for example, pass a reactor it, or make the reactor a class var and subclass. Maybe the latter would be better.
<henninge> allenap: argh, I missed that in the test.
<allenap> henninge: If it were left as True in externalbugtracker.txt (not checkwatches-cli-switches.txt; I got that wrong) then there would be an error. The test would pass, but the test suite would complain.
<henninge> I see. I don't know enough about Twisted to asses the other options you mentioned.
<henninge> allenap: I was just wondering that only one code path was tested but now I understand it is to make it testable.
<henninge> allenap: Please make a note in the doc string for install_signal_handlers.
<henninge> for __init__, of course
<allenap> henninge: Sure. I think I might change it to a subclass, but I'll add some more docs to make it clear.
<henninge> thanks
<henninge> allenap: I wonder if there should be a test for SerialScheduler in some way.
<allenap> henninge: I guess it's implicitly tested, but I'll add an explicit test too.
 * allenap goes to look after ill kids.
<henninge> allenap: review sent
* henninge changed the topic of #launchpad-reviews to: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * henninge bows out.
<gmb> abentley: Hi. Do you have time to review https://code.edge.launchpad.net/~gmb/launchpad/fix-b.l.n-timeouts-bug-517798/+merge/18866 for me?
<gmb> Simple fix for bug 517798
<mup> Bug #517798: Launchpad beta timing out three times when I try to access the bug page. <oops> <timeout> <Launchpad Bugs:In Progress by gmb> <https://launchpad.net/bugs/517798>
<leonardr> rockstar, version branch coming your way
<rockstar> leonardr, cool.
<abentley> gmb, happy to.
* abentley changed the topic of #launchpad-reviews to: abentley || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> gmb: r-me
<gmb> abentley: Excellent, thank you.
<leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-pagetest/+merge/18868
<rockstar> leonardr, great.  I'm assisting in a cherry pick right now, will review when that's done.
* abentley changed the topic of #launchpad-reviews to: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, hi.  Something is fishy about this diff.
<rockstar> leonardr, is it not based on one of the previous branches I reviewed?
<rockstar> leonardr, nevermind.  I plead temporary insanity.
<rockstar> leonardr, so, if there's no mutator specified for 2.0, it uses the mutator specified from 1.0?
<leonardr> rockstar: right
<leonardr> same as other named operations
<rockstar> leonardr, so is "beta" the most recent version, and then 3.0, 2.0, and 1.0?
<leonardr> rockstar: right, according to the list of versions defined in the iwebserviceconfiguration
* abentley 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
#launchpad-reviews 2010-02-09
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> morning gmb.  so the forced build has already failed?
<gmb> bac: Really? I hadn't looked at it since I forced it; been going through the review queue.
<bac> or am i reading the waterfall wrong?
<gmb> bac: The bit at the top refers to the last build, not the current one.
<gmb> It's still building.
<gmb> Though I'm going to check the output now that you've mentioned it :)(
<bac> i ran another branch through ec2 last night and got a single failure of xx-front-page-bug-lists.txt.  didn't you make a change about ordering of front page bugs?
<bac> i notice that test was a failure in the buildbot output too
<bac> but, i cannot reproduce it locally
<bac> gmb: ^
<gmb> bac: Hmm. I didn't spot it in the buildbot output but I didn't look much further than the bzr errors. I wonder if my testfix might turn out to be timebomb. I'll check.
<bac> gmb: it just looked like an issue with the output ordering
<gmb> bac: Yeah, that's what I mean. I changed the order by on the query but it's now ordering by bug.datecreated, which is more volatile than (datecreated, id). I might need to change the test to not echo out bug ids.
<gmb> bac: Annoyingly, of course, it passed ec2. Oh well.
<gmb> bac: http://pastebin.ubuntu.com/372418/ stop the test from blowing up, though it no longer tests the ordering of the results (though we don't really care all that much about the ordering of those five bugs as long as they're the most recently filed).
<gmb> bac: Is that worth landing or should I come up with another solution?
 * bac looks
<bac> gmb, i haven't looked at the test in detail but why do you say datecreated is more volatile?  don't you know the creation order?
<gmb> bac: Ideally, yes, but if that test is really timebombing in production then evidently there's something going on that we don't know. Also, the order should be linear - 1, 2, 3, 4, 5 - but if you look at the test, it's not. That was a change I made after chaning the query ordering; I should really have spotted the potential timebomb then.
<gmb> bac: I need to investigate this more, because that ordering is plain weird.
<gmb> But I'm concerned that leaving things as they are will leave the build broken, or at least very susceptible to breakage
<bac> gmb: right, i think it needs some investigation.  i'd hate to put in a test fix that masks a real problem, of course.  but if it's just a testing issue then your fix is good.
<gmb> bac: Yeah. I think we should land the elided version of the test and then find out what's causing the ordering oddness and deciding whether we care.
<bac> ok
<gmb> bac: So can I do that with your r=?
<gmb> (I'll file a bug to track the ordering problem)
<bac> yes
<bac> oh, and happy birthday!
<gmb> Thanks :)
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> gmb: I have a birthday present for you
<bigjools> a 3524 line Soyuz branch :D
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bigjoolsOMGMYEYES || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bigjools: Laugh? I nearly herniated.
<bigjools> gmb: I wouldn't spend too much time doing details on it - it's a very mechanical change
<gmb> bigjools: I'll have a quick glance, but I think you're right. As long as the tests pass and dogfood doesn't melt or something...
<bigjools> well dogfood melts a lot anyway :)
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bigjools: Land it :)
<bigjools> quicker 3k5 branch review evar
<bigjools> quickest*
<bigjools> don't say I never give you anything on your birthday
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bigjools: Indeed.
<bac> gmb: so what's going on with buildbot and bzrlib?
<gmb> bac: WTH... apparently nothing.
<gmb> It seems to be stuck.
<bac> gmb: but there is now a version imcompatability, right?
<leonardr> rockstar: see my latest comment on https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-pagetest/+merge/18868 when you come in
<gmb> bac: Yes, it looks that way. I'll speak to a LOSA.
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, edwin || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, actually, that would be a problem, wouldn't it?
<rockstar> beta should be newer than 3.0, which is newer than 2.0, etc.
<leonardr> rockstar: actually beta is the _earliest_ version, but 3.0 is newer than 2.0, etc
<rockstar> leonardr, what is meant by "earliest" though.
<rockstar> It shouldn't mean that 1.0 overrides it.
<leonardr> rockstar: well, right now launchpad has a 'beta' web service and nothing else
<leonardr> now that i have multi-version working i'm going to create a '1.0' version that's different
<rockstar> leonardr, yes, but then we'll go 1.0, and beta will be what 2.0 is created from.
<leonardr> that's what i mean by saying beta is the earliest
<leonardr> rockstar: no, we do have a 'floating development version', but in launchpad's case that is called 'trunk'
<leonardr> 'beta' is a specific version
<leonardr> that will become obsolete and stop changing
<rockstar> leonardr, huh, that's odd.
<rockstar> leonardr, okay, I guess it makes sense to have 1.0 supersede beta then.
<leonardr> rockstar: if you are confused it's likely others will be confused and think that 'beta' is the permanent cutting edge
<leonardr> but i don't know what we can do about that
<leonardr> it's more like gmail, which was in 'beta' for 5 years and now it's not
<leonardr> the only differnece is that we have to keep 'beta' around for a while
<rockstar> leonardr, yeah.  Having worked on the other side of the REST API (on Tarmac), when the API changes now, baby Jesus cries.
<leonardr> thus the multi-version project
<rockstar> leonardr, I think "trunk" for bleeding edge is probably misnamed, since it's likely it could be in production as "trunk"
<leonardr> rockstar: it's not too late to change it
<leonardr> if you have a better idea
<rockstar> leonardr, well, just what I was assuming where beta is the floating development version.
<leonardr> my only alternative is 'dev' or 'development'
<rockstar> leonardr, are we going to release an API version on every release of Launchpad?
<leonardr> rockstar: no, API versions will correspond roughly with ubuntu releases
<leonardr> and will be retired at the same rate as ubuntu releases
<rockstar> Ah, okay.
<rockstar> So maybe "dev" would be appropriate, since (at this point in time), it would correspond to the scripts in Lucid.
<leonardr> flacoste: ^- what do you think?
<leonardr> incidentally, flacoste, the multiversion lazr.restful work is complete as of yesterday
<flacoste> leonardr: i saw good progress on that, great! but complete? i guess we still need some changes in the client support, no?
<flacoste> leonardr: btw, i don't have an opinion on dev vs trunk
<flacoste> we could call it 'unstable'
<leonardr> flacoste: yes, we need to add multiversion to lazr.restfulclient
<leonardr> flacoste: that would be good in continuing the analogy to ubuntu
<jtv> gmb: I figure you owe me one, if a small one: https://code.edge.launchpad.net/~jtv/lazr-js/bug-427263-3/+merge/18882
<gmb> jtv: Sure
<jtv> thanks!
<gmb> jtv: From the diff it looks like you're setting the content attribute to ".".  But if I'm reading the mp right, you're setting it to zwnj... so is the diff just bong?
<jtv> gmb: looks like... hang on
<jtv> gmb: that's what you get when you prototype in a separate branch... this was an older approach.  Not hard to fix, but I'd better check that the fix works.  :)
<gmb> :)
<jtv> gmb: I just pushed the correction.
<gmb> jtv: Looks good. r=me.
<jtv> gmb: thanks... no "this is incredibly hackish, how did you even conceive of it"?  :)
<jtv> Because I'm quite proud of that part.  :-)
<gmb> jtv: I confess, I'm intrigued.
<gmb> So do tell :)
<gmb> (Though I would call it 'inspired' rather than hackish, but maybe that's just me.)
<jtv> at this point, my bluff called, I sorely wish I had an interesting story to go along with this patch
<jtv> It's just something I tried because nothing else seemed to work.
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: edwin || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> jtv: Heh, well, fair enough.
<jtv> Gotta work on that story...  :/
<gmb> jtv: May all your experiments prove successful.
<jtv> <ka-BOOM!>
<gmb> Indeed.
 * gmb calls it a day
<jtv> g'night!
<salgado> bac, can you review https://code.launchpad.net/~salgado/launchpad/kill-rogue-ec2-instances/+merge/18935 for me, once you're done with Edwin's?
<bac> salgado: yep
* salgado changed the topic of #launchpad-reviews to: on call: bac || reviewing: edwin || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> thanks bac!
<adeuring> bac: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-518746-faster-retrieval-of-patch-age/+merge/18936 ?
<bac> adeuring: yes.  please add to topic.
* adeuring changed the topic of #launchpad-reviews to: on call: bac || reviewing: edwin || queue [salgado, adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: salgado || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: sorry for the delay.  am starting on yours now.
<bac> salgado: you should've said it was only two lines long!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: salgado || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: adeuring || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> bac, nah, there really was no hurry to get this reviewed, so I didn't mind staying in the queue
<salgado> and thanks, btw. :)
<adeuring> thanks for your review, bac!
<bac> np abel
<leonardr> bac, can you add https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/shorten-cache-filename/+merge/18951 to the queue?
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: why is that branch marked private?
<leonardr> bac: not intentionally
<leonardr> bac: fixed, i don't know how that happened
<bac> leonardr: also, is it still possible to get a LP instance in lplib that does not use caching?  i looked at it yesterday and it seems there is no way.
<leonardr> bac: it's not possible, but i've never heard anyone express a desire for that before
<bac> leonardr: it used to be possible and i used it when testing against lp.dev.  probably not a strong use case
<bac> leonardr: and certainly not relevant to this review.
<leonardr> bac: i don't think that's a big problem
<bac> leonardr: done.  thanks.
<leonardr> thank you
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac hi
<bac> hello
<sinzui> I have a branch I created with deryck to migrate hwdb
<bac> ok
<sinzui> I gave it to abel, but could you review it?
<bac> sure, if i'm not duplicating abel's work
<sinzui> bac: I have serious doubts that use hardwaredb is clearer than hwdb
<bac> i think hwdb is pretty darn clear
<bac> and short
<sinzui> yes
<sinzui> I think I should rename it to hwdb
<EdwinGrubbs> bac: ping
<bac> hi EdwinGrubbs
<EdwinGrubbs> bac: I'm not so sure about moving the paths from bin/sprite-util into the Makefile, since there are a bunch of other variables that are dependent on them. For example:
<EdwinGrubbs> bac: The png that create-image makes needs to be referenced by the css file that create-css makes. Since the css is in icing/build and the image is in icing/, "../icon-sprites" is the relative path, but that is much less clear when some of the paths are in the Makefile.
<EdwinGrubbs> bac: passing in all the necessary info from the Makefile seems unwieldy.
<EdwinGrubbs> Alternatively, I could add commands to sprite-util that return the paths, so the makefile can set up variable names.
#launchpad-reviews 2010-02-10
<henninge> Hi noodles775!
<noodles775> Hi henninge :)
<henninge> ;)
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: could you please have a look at jtv's review UI-wise? https://code.edge.launchpad.net/~jtv/launchpad/translationimportqueueentry-info/+merge/18963
<henninge> noodles775: I just attached some screenshots for you to look at. Scroll down to after my code review. ;)
<noodles775> henninge: I can, but it'd be great to check first if sinzui was keen (afaiui, he's keen to build up ui reviews). I can then go through it after that if required.
<henninge> s/review/merge proposal/
<noodles775> But if it's urgent, I can do it today?
<henninge> noodles775: will sinzui not be available today? It could wait until he gets up ...
<henninge> noodles775: according to Matt's mail he should be here, so it can wait.
<henninge> noodles775: thanks
<henninge> Hi jpds! I have not yet been able to land your branch. The ec2 test runs kept breaking off.
<henninge> jpds: can you please merge the current devel into your branch and push it again?
<henninge> I am talking about lp:~jpds/launchpad/fix_518232, btw. ;-)
<jpds> henninge: I get http://pastebin.ubuntu.com/373098/
<jpds> henninge: On an up-to-date devel.
<henninge> jpds: sorry, that was the wrong way round ;-)
<jpds> Ah, I get Nothing to do.
<henninge> jpds: make sure to revert that merge on devel. "bzr revert" should do it if you did not commit.
<henninge> jpds: what I meant was this : http://pastebin.ubuntu.com/373099/
<jpds> I did bzr revert --no-backup
<al-maisan> hello allenap, I have an easy branch for you :)
<al-maisan> https://code.edge.launchpad.net/~al-maisan/launchpad/flip-the-switch-516545/+merge/18992
<allenap> al-maisan: Cool, on it :)
<al-maisan> allenap: thanks!
<jpds> henninge: All done.
<henninge> jpds: cool, I'll try landing again.
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> al-maisan: r=me, with one small comment.
<al-maisan> allenap: thanks, on the phone.
<henninge> jpds: ec2 test instance is running as usual this time. Looking good. ;)
<jtv> henninge: updated...  it did grow the diff some.
<henninge> jtv: ping
<mrevell> allenap, review a branch for me NOW
<allenap> mrevell: Aye aye.
<mrevell> allenap, https://code.edge.launchpad.net/~matthew.revell/launchpad/ppa-add-repo-help-492283/+merge/19010
<mrevell> ta
<bigjools> how to win friends and influence reviews, by mrevell
<mrevell> yessir
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: mrevell, - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> mrevell: rejected=me, incorrectly formatted cover note.
<mrevell> allenap, Are you complaing about my TPS reports?
<allenap> mrevell: Launchpad cannot compete in the marketplace without correct TPS reports. Please sign up for the TPS refresher programme.
<allenap> mrevell: How the (&&%$%^& do I see this new help on launchpad.dev?
<jtv> henninge: pong
<mrevell> allenap, good question, I take your point -- https://launchpad.dev/~cprov/+archive/ppa
* sinzui changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: mrevell, - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> hello jtv, would you be in a position to review a branch of mine?
<al-maisan> https://code.edge.launchpad.net/~al-maisan/launchpad/disabled-copyarch-519665/+merge/19019
<jtv> al-maisan: right after the reviewers meeting and team standup, sure
<al-maisan> ah
<al-maisan> oh
<al-maisan> I missed that..
* al-maisan changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: mrevell, - || queue [sinzui, al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: should I do sinzui's branch first?
<allenap> jtv: Okay, sounds good.
<jtv> al-maisan: so you're in line after curtis then
<al-maisan> yup
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: al-maisan, sinzui || queue [sinzui, al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: al-maisan, sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> allenap, jtv: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/forbidden-oops/+merge/19021 ?
<allenap> abentley: Sure.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: al-maisan, sinzui || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> allenap, thanks.
<jtv> allenap: I don't see sinzui's MP anywhere... do you?
<sinzui> jtv: https://code.edge.launchpad.net/~sinzui/launchpad/launchapd-apocalypse-1/+merge/18462
<allenap> jtv: There it is ;)
<jtv> oh, that review's already been claimed
<jtv> sinzui: do you want it handled by OCR or stick with Edwin/Abel?
<sinzui> jtv: I really want the branch landed, so please review it. It is very boring.
<jtv> sinzui: you lost me at "boring"
<jtv> but alright :)
<sinzui> jtv: the migrater script create large diffs of mechanical changes
<jtv> now if only we had a mechanical reviewer...
<jtv> sinzui: as for the renaming, personally I'm fine with "hwdb" as a prefix (which must be short) but "hardwaredb" as the module name (which must be reasonable but legible).
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: al-maisan, sinzui || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> sinzui: if test_views.py is new, not copied over, then please update the copyright year
<jtv> although this one looks familiar... if it's a copy-paste job, shouldn't we have a helper for this?
<sinzui> jtv: right, the migrater assumes we did what we claimed we would do in 2009
<jtv> sinzui: electric monk?
<jtv> We're finally writing software to believe things for us that we don't have time to believe ourselves now?
<sinzui> jtv:I think this is a case where many teams did not take this effort seriously. This issue looks like a large mass of uncompleted work
<EdwinGrubbs> abentley: I'm starting to review your forbidden-oops branch.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: al-maisan, sinzui, abentley || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> sinzui: you mean the migrator?
<abentley> EdwinGrubbs, allenap already agreed to review it.
<sinzui> jtv: canonical.launchpad should not exist.
<jtv> sinzui: your branch brings us a step closer.  Maybe file a foundations bug for the duplicate file?
<sinzui> jtv: it remains the primary cause of circular imports, code that does not follow our style, code that you must hunt to fine, code that you cannot test on the module level for the feature
<abentley> EdwinGrubbs, I'm fine with you reviewing it, just want to avoid two people reviewing it.
<EdwinGrubbs> makes sense
<sinzui> I started another branch that modifies the migrater to support services so that I can move them on mass.
<EdwinGrubbs> allenap: do you want me to take it.
<allenap> EdwinGrubbs: If abentley is happy with that, then sure. I'll start it in just a few minutes otherwise; just finishing al-maisan's review.
<jtv> sinzui: ambiguous parse...  sorry to be dense but is "support" a verb in that sentence?
<jtv> (I could read it either way)
<EdwinGrubbs> allenap, abentley: I'll start on it now.
<sinzui> jtv: many of the unmigrated code should be subtrees in lp/serivces. Each has its own interfaces and models directory
<jtv> sinzui: oic, that makes sense yesâa lot of the stuff that's still in l.c.l is there for that reason, I think
<jtv> sinzui: about the review... in lib/lp/hardwaredb/tests/test_doc.py, you're using our long-list notation style for an arguments list.  I didn't know you could have a trailing comma there!
<sinzui> jtv: I think pep8.py demands it to be there. The file was made by the migrater
<sinzui> of, no it wasn't, I had to edit it
<noodles775> Hi sinzui, would you have time for a small UI review? https://code.edge.launchpad.net/~michael.nelson/launchpad/509370-access-non-unique-ppa-name/+merge/19022
<jtv> sinzui: I didn't know pep8 said that... it's certainly not what we normally do.  Would you be willing to take it out?
<noodles775> sinzui: it's not urgent, so I can wait for someone else if not.
<noodles775> mrevell: Do you mind going over the wording change on the above MP too?
 * mrevell reads up
<mrevell> noodles775, would like to
<mrevell> meaning, yes, I'll look now
<noodles775> mrevell: thanks.
<jtv> sinzui: in the migrater, you're protecting the removal of the setup and teardown hooks against exceptions... is that specifically to guard against the case where they were never inserted?  If so, better to specify the exception type (and make sure they're in the safe order)
<sinzui> jtv: it does say that. one day I will replace my navel-lint script that I gave Launchpad with my  pure python lint that I switched to last year
<jtv> sinzui: what does say what?
<sinzui> jtv: One of the setup/teardowns was written oddly in the code and that killed the migrater. I added the try/except to see it complete. so that I could manually fix the issue. I discovered that the code was migrated correctly, so I left the try/except there
<sinzui> jtv: I would rather 1) remove the script, and 2) other teams complete the work we agreed they would do. I am just trying to finish their work
* deryck changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: al-maisan, sinzui, abentley || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> hi allenap, jtv, and EdwinGrubbs -- I put one on the queue if someone can spare the review.
<jtv> sinzui, deryck: otp
<jtv> sinzui: I don't suppose there's any chance of this messing up a migration, ever?
<jtv> the try/except clause, I mean?
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: deryck, sinzui, abentley || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> jtv:I can remove it since I am done with it. I usually do that, but I think that is also a contributing factor why engineers are not using the tool. Only jml has committed fixes to the script besides myself
<jtv> sinzui: no point in leaving it broken, I guessâespecially now that what's left is the modules that weren't migrated easily early on
<EdwinGrubbs> abentley: review sent
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: al-maisan, sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> sinzui: so if this solves a real problem, leave it in.  But a comment would be helpful either way.
<jtv> on call: allenap, jtv, Edwin || reviewing: al-maisan, -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: did you take muharem's branch?
<jtv> allenap: I just went over it as well, so if you're just starting, I can take it over from you
<allenap> jtv: I did one of his branches recently.
<jtv> allenap: the one about suspending jobs for disabled archives
<allenap> jtv: That's the badger.
 * jtv refreshes page
<jtv> oh right
<jtv> I do wonder what the pre-imp said about re-enabling archives
<jtv> I had a lot of trouble with that test function as wellâthe scope of the "not" in the name is ambiguous.  AFAICS it would've been as easy, but clearer, to write to separate checks
<allenap> jtv: Yeah... you should comment. I didn't think about that.
 * jtv comments
<jtv> allenap: done
<abentley> EdwinGrubbs, thanks for your review.
* allenap changed the topic of #launchpad-reviews to: on call: jtv, Edwin || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: good night!
* jtv changed the topic of #launchpad-reviews to: on call: jtv, Edwin || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> jtv: Cheerio :)
* jtv changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> EdwinGrubbs: you're on your own
<EdwinGrubbs> bye
<bac> sinzui: where is the branch you mentioned?  even if it isn't ready to review i'd like to look at it.
<sinzui> bac: indeed I was about to tell you to look at lp:~sinzui/launchpad/all-upstream-links . There is a test I need to update since beuno and I decided to present the table differently.
<bac> sinzui: is 'status' too weasly as in 'Upstream information status"
<sinzui> bac:umm...yes it is
<bac> yeah, i thought so
<sinzui> bac: This information is not necessarily about state. I think of these points as services, but I know that is not why we need to know them. This is really about the points where a project connects to different kinds of contributors
<bac> what if were just labeled "Upstream Connections"
<sinzui> bac: okay, I like that better than every other thought I have had....
<bac> just grasping at straws here
<sinzui> bac: I merged this information into the project information portlet. Maybe we can test this portlet name by naming a new portlet on the project page. <Community|Contributor> connections?
<bac> ok
<sinzui> bac: think the launchpad projects you work with. Would owners complete this information if it were presented with this name?
<sinzui> bac: is "Upstream contributor connections" a better name for your portlet?
<bac> i like the layout you've done.  but i think we need to make it easier to navigate to the action.
<bac> if i wanted to set the bug tracker where is it done?
<sinzui> bac: I think that is the next blueprint...
<sinzui> we really need to get this blueprints's UI on edge for critique while we work on the upstream side...
<bac> so links to the places to set the data will come later?
<sinzui> bac: There are several issues, that are too much for this release: 1) change permissions so that you can set the information, and 2) change the forms so that the information can be set in context
<bac> sinzui: ok.  that makes sense.
<sinzui> bac: Can you really set the series branch from a vocabulary? And as for the f&cking bug tracker. you need to go to the hidden page to register it, then find the field on the project to set it--we can solve this like the Create release link on series I hope
<bac> sinzui: do we want to macrotize the work you've done...or push the bool computations into a view mixin?
<sinzui> bac: I think so.
<bac> sinzui: which?  both?
<sinzui> bac: my branch introduces: <metal:yes-no define-macro="yes-no">
<bac> i'd say both.  i'd rather have the login in a view
<bac> yeah, the yes-no is cool
<bac> but i don't want to repeat  tal:define="bool not:series/translations_autoimport_mode/enumvalue:NO_IMPORT">
<bac> and the bug tracker logic is more complicated than you've shown b/c it has to take parent project group info into account
<sinzui> If the tables I created work for the portlet, I would push them into either in a macro or an view that exposes the four points
<bac> sinzui: ok, let me steal from your branch and see if it works.  we can refactor afterwards
<sinzui> bac: what? why can my project group have a bug tracker but not let me report a bug on the project group?
<bac> sinzui: if firefox.bugtracker is None and official_malone is None but mozilla has a bug tracker then it will inherit mozilla's
<bac> that's my understanding
<sinzui> I did not know that, and that make me very upset that I cannot report a bug against mozilla
<sinzui> bac: but given this, I favor a view that adapts a series or project that exposes the information. so that the template remains stupid
<bac> i'm all for stupid-ass templates
<bac> sinzui: but i think i've explained myself badly
<bac> or even poorly
<sinzui> bac: it should adapt the series because that is what is in the Packaging object
<bac> you can report a bug against mozilla and firefox could have its own bug tracker but if it doesn't it uses the parent's
<sinzui> bac: You cannot report a bug against launchpad-project...it defaults to blueprints. Most bugs reported in blueprints were from users who know launchpad is busted and they could not identify which componenet
<bac> i didn't know that
 * bac has never tried
<sinzui> project groups do get a working dupe finder though...much better than our retarded decision to ask users to report bugs against /launchpad. Most bugs reported there are dupes. At least the bugs reported against blueprints are not
<sinzui> bac: I updated my tests to check for "Upstream Contributor Connections" in the table. Are you working on a view+template to adapt the series to to two list layout?
<bac> sinzui: not yet.  i was going to do some hard code experiments to see what how i can format the existing portlet
<sinzui> bac: I was going to put my branch into review. I can instead extract the portable info to a view and template now
<bac> sinzui: that would be good
<sinzui> okay I will start
<EdwinGrubbs> bac: I just wanted to check if you had any concerns about my reply to your review since I had left some of the changes out due to complications.
<bac> nope
<bac> sorry i didn't comment
<bac> EdwinGrubbs: i did mark it as 'approved' after your reply...which was a subtle way to signal consent
<EdwinGrubbs> ok, thanks. I hadn't noticed that it was approved before, or I would have asked you earlier.
<sinzui> barry: land it
<barry> sinzui: rock
<sinzui> funny, that is almost the same as my review
<barry> sinzui: :)
<barry> sinzui: while i have you here, can you verify for me?  we only store oauth tokens for Persons not Accounts, right?
<sinzui> barry: it is on the Person
<barry> sinzui: that's what i thought.  what i don't know is whether we can limit reviews to folks who have lp accounts.  they might only have logins to ubuntu.com
<barry> if it's the latter, then the problem we have is that we need to verify who is rating an app, and i think without oauth we have no way to do that
<sinzui> barry: since lp profiles are created on demand when someone logins to lp, it is kind of moot
<barry> sinzui: hmm... so let's say they've never heard about lp, but they've bought something from the store or set up their u1 account...
<sinzui> barry: PersonSet.enrsurePerson('ubuntu-one-user') will create the user when he uses lp
<barry> sinzui: does that work if the first time they see lp is through an oauth request via launchpadlib?
<sinzui> I wouldn't know.
<barry> sinzui: okay, i'll try to hunt that down
<sinzui> auth is a foundations/isd matter. I see from bugs too many parts of launchpad try to woek with an account...the look up for a person fails the moment the email is accessed, but by using ensurePerson(), the conversion happens without pain
<barry> sinzui: it feels like we're really close to something usable.  essentially we just want to use lp to verify who the person is and link their review to a login.u.com account
<sinzui> barry: I think the crux of the issue is oauth. The steps are:
<sinzui> 1) user submits a review to lp
<sinzui> 2) lp gets user via ensurePerson()
<sinzui> 3) lp asks user to authorise the script
<barry> sinzui: yes, i think that's about it
<sinzui> barry: you could use anonymous, and it may be possible to insert the user who is using the client
<barry> sinzui: i don't follow
<sinzui> I do not know how it is possible to allow an anonymous script to do an insert, but if it could happen, the script passes the review and with the user the client *claims* is using it
<sinzui> I have never used anonymous access.
#launchpad-reviews 2010-02-11
<noodles775> Topic for #launchpad-reviews: on call: noodles775 || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> jtv: good morning, did you see the reply to your review email by any chance?
<jtv> al-maisan: going over there now
<al-maisan> jtv: thanks!
<jtv> al-maisan: shame there's always so little time at fosdem...  we should've gotten a place with internet and stay for a few extra days of buildfarm work.  :)
<al-maisan> jtv: at least a few days :)
<jtv> :)
<jtv> al-maisan, you are cleared to land
<al-maisan> jtv: oh, great, thanks!
<jtv> jml: could you have a look at this db change?  I'm going to ask for the operative statement to be run on production as well.
<jml> no.
<jml> sorry, not for another couple of hours.
<jtv> jml: I'll shop around a bit then... it's pretty minimal, and I don't see stub around
<jtv> BjornT: is this something you can review?  A "create index" to deal with timeouts on the Bugs front page.  https://code.edge.launchpad.net/~jtv/launchpad/db-bug-518965/+merge/19091
<deryck> jtv, I saw your bug about the need for the index, but I thought we removed the secondary sort on id.  I'm curious why this is still needed.
<jtv> deryck: that's just what I'm trying to figure out now.  The page loads in 9 seconds for me, so too short for a helpful oops but too long for human consumption.
<jtv> deryck: I thought gmb said that these were distinct problems, but when I actually looked I found I was mistaken.  There may be other queries with the exact same problem.
<jtv> hmm... closer to 19s this time.  Pretty bad.
<deryck> jtv, we were having other issues yesterday, which we don't normally have, we assume because we're only hitting the master dbs currently.  this could be related, perhaps?
<jtv> deryck: is that still the case?  Yes, that'd increase load by a lot.
<deryck> jtv, yes, that is the case.
<deryck> jtv, gary was reluctant to land the change to re-enable slaves with stub unavailable.
<jtv> deryck: we used to ratchet down the timeout for performance weeks (no idea why we stopped doing that), and this may have the same effect.
<deryck> jtv, yeah, makes sense to me.
<jtv> deryck: I'm now checking out edge oopses to see whether the problem is really solved.
<jtv> deryck: the answer is somewhere in the middleâthis particular bottleneck has been resolved (sorry for mixing metaphors) but other queries are still slow.
<jtv> I'm withdrawing my patch.
<deryck> jtv, ok, cool.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [noodles775] || This channel is logged:  http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: did you say intellectronica was going to do my UI review for https://code.edge.launchpad.net/~jtv/launchpad/translationimportqueueentry-info/+merge/18963 ?
<intellectronica> jtv: i'm not sure what the current ui review policy is, but unless it changed, i can only do a mentored review (which i'll be happy to do)
<noodles775> jtv: nope, I said that it'd be good to check with sinzui as afaik he's wanting to build up ui reviews.
<jtv> noodles775: ah, so I remembered the wrong person
<noodles775> If he doesn't want it, I can do it of course.
<jtv> intellectronica: sorry to have bothered you
<sinzui> I can do the review in an hour
<jtv> noodles775: I think it's a good mentoring review
<intellectronica> no worries
<jtv> sinzui: that'd be great, thanks
<sinzui> noodles775: has one ahead of your i think
<noodles775> well I think jtv's is older than mine, so I'm happy to wait ;)
<jtv> mine's about a day and a half old
<mrevell> hey noodles775, wanna do another UI pre-imp chat? It's a very small matter.
<noodles775> mrevell: sure, call or chat?
<mrevell> noodles775, Chat'll be fine
<noodles775> k
<leonardr> rockstar, when you come in, hopefully the last lazr.restful multiversion branch for a while
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/bump-version/+merge/19107
<rockstar> leonardr, I'm actually composing the "holy crap I'm still sick" email.
<leonardr> rockstar: want me to give it to someone else?
<rockstar> leonardr, yes please.
<leonardr> noodles775, can you take https://code.edge.launchpad.net/~leonardr/lazr.restful/bump-version/+merge/19107 ?
<noodles775> leonardr: I'm just on the phone, but I'll take a look as soon as I'm off.
<mrevell> Hello noodles775
<mrevell> When you have a moment, I wanted to ask you for some UI advice. At UDS, deryck and I identified various bits of the bugs interface that could benefit from a help pop-up.
<noodles775> yep
<mrevell> noodles775, One was the tags listing, which is also an ajax editing form.
<mrevell> I was thinking about adding a <small>(<a href="">?</a></small> just beside the pencil icon (or "Add tags" link, if there are no tags). It looks a little, well, spare. So I wondered about adding the helping to the "Tags" label. I'm not sure. Any thoughts?
 * noodles775 scans for other places where we've done something similar...
<noodles775> mrevell: so on the PPA index page, we have things like (in the context of uploading packages): "(Read about uploading)", but the page itself isn't as full as the bugs page...
<mrevell> noodles775, Yeah, and I think a full text link would detract from the tags themselves.
<noodles775> yep..., but that doesn't leave you many options?
<mrevell> Nope, hence this chat :)
<noodles775> We do have: https://edge.launchpad.net/@@/maybe on the +graphics page.
<noodles775> It looks much nicer than a text ?.
<mrevell> noodles775, Yeah, that's probably a good option. I prefer help links that include some context but not at the expense of cluttering up the page. I'll use that and see how it looks. Thanks.
<noodles775> np.
<noodles775> leonardr: on line 45 of the MP diff, is this really the earliest version, or the earliest *active* version? (which line 47 seems to imply?)
<leonardr> noodles775: inactive versions don't exist in the lazr.restful installation, only in peoples' memories, so the concepts are equivalent
<leonardr> i can change the wording
<noodles775> leonardr: in which case, why can len(config.active_versions) == 0, but we still have an earliest_version? or is that what you'll re-word?
<leonardr> noodles775: it's kind of confusing but i don't want to change the interface becuase that'll cause backward incompatibility problems
<leonardr> there are two fields in the interface
<leonardr> active_versions and latest_version_uri_prefix
<leonardr> active_versions is a list, the other is a stirng
<leonardr> lazr.restful serves web services for all of those strings
<leonardr> so if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel' there are three versions being served
<leonardr> if active_versions is [] and latest_version_uri_prefix is 'beta' then there is one version
<leonardr> though i don't recommend doing that
<noodles775> So if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel', why are we evaluating earliest_version = 'beta', is that right?
<noodles775> leonardr: ^^
<leonardr> noodles775: i'm having trouble parsing that sentence, but i can tell you that in that case earliest_version is 'beta'
<leonardr> latest_version_uri_prefix can only be the earliest version if active_versions is empty
<noodles775> OK, as the code implies, I'm just confused by the names.
<leonardr> noodles775: i'm kind of leaning towards treating the last item in active_versions as latest_version_uri_prefix
<leonardr> but that would be a separate job
<noodles775> Right, that would make sense. (do you mind adding an XXX?)
<leonardr> sure
<noodles775> leonardr: just a style question, is the comment on the empty line 69 intentional?
<leonardr> yeah, it's like a para break. i don't know if we allow that though
<noodles775> Yeah, I think a blank line (without the comment) serves just as well, but if the style guide doesn't say either way, whatever you prefer :)
<leonardr> style guide doesn't mention it
<leonardr> if i see a blank line i think the first comment was talking about the blank line and the second comment is different
<noodles775> leonardr: s/marker_interface/version_marker on line 72
<noodles775> sure
<leonardr> noodles775: i might as well make that latest_version_uri_prefix change because i haven't actually released a version that uses it yet. if i get rid of it now there will be no backwards compatibility
<leonardr> problem
<noodles775> leonardr: great
<noodles775> Two other small things:
<noodles775> See if you think it's worth creating a helper for the repeats of line 145-146 - the helper could even print out the ordered list items?
<noodles775> s/repeats/repititions sorry, bad day.
<noodles775> And s/an/a on line 180.
<leonardr> noodles775: i thought about putting that 145-146 stuff in the big helper method but decided it was easier to read if i spelled it out every time
<noodles775> leonardr: OK.
<noodles775> leonardr: another typo, s/in/In on 138
<leonardr> noodles775, fixed everything so far
<noodles775> leonardr: Great, I think the text for versions 3.0 is incorrect? The method has changed to by_value?
<noodles775> leonardr: and with that, r=me.
<leonardr> yes, you're right
<leonardr> noodles775: if you can move on to https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/multiversion/+merge/19110 i'll work on getting rid of latest_version_uri_prefix
<noodles775> leonardr: sorry, I need to leave asap now, but I've got one more question for the this MP:
<leonardr> ok
<noodles775> Is the change in tales.py tested?
<noodles775> I can't see an example where we we've removed an op in a later version?
<leonardr> noodles775: i don't have details off the top of my head, but it is tested in that the wadl.txt test will fail if you remove the code
<leonardr> noodles775: actually i can add a quick test that proves it works
<noodles775> OK, great. I'm out of here... 'night!
<leonardr> can i get someone else interested in the web service to review some more branches? maybe sinzui?
<noodles775> on call: - || reviewing: - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> leonardr: I have reviews ahead of you, but I can commit to doing it in a few hours
<leonardr> sinzui: ok, let me try to find someone else. EdwinGrubbs? gary? intellectronica?
<intellectronica> leonardr: you're asking for a review?
<leonardr> intellectronica: yes, if oyu have the time
<intellectronica> how much work is it?
<leonardr> not much if you only want to do one
<leonardr> i have a couple, but i'm taking reviewers as i can find them
<intellectronica> oright, i'll do one then
<leonardr> intellectronica: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/multiversion/+merge/19110
<intellectronica> leonardr: the diff looks fine to me
<leonardr> cool
<gary_poster> leonardr: I'll have some time late afternoon
<intellectronica> leonardr: i wonder if it would be good to extract the code that deals with building the URL to another method, so that a specialized class can override it?
<intellectronica> i mean if an implementation needs to build the URL using a rule other than base + '/' + version
<leonardr> intellectronica: i think yagni. if you want that you can subclass the client class and put your implementation in the constructor before calling the superconstructor
<intellectronica> fair enough. r=me
<leonardr> flacoste, maybe you want to review the branch i chatted with you about earlier?
<leonardr> shouldn't take long
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/no-latest-version/+merge/19122
<leonardr> or gary or sinzui, if you're free by now
<gary_poster> leonardr, I'll take it
<leonardr> gary: link above
<gary_poster> ack, looking
<gary_poster> leonardr: nice.  approved.
<leonardr> gary, thanks
<jtv> sinzui: do you think you'll get around to that UI review?
<jtv> there's a link to some screenshots in my last comment
<sinzui> jtv: I am just finishing the one before you
<jtv> cool
<sinzui> jtv: ping
<jtv> sinzui: hi
<jtv> sinzui: there are some rough edges, I know; I'm at your disposal
<sinzui> jtv: your edit links (imports/2) do not work in webkit.
<sinzui> I can see the edit icons in firefox, but not epiphany
<jtv> sinzui: that's a known bug; I already submitted a fix to lazr-js
<sinzui> fab. I can do the review now that I know where the link to your changes are
<jtv> (also, not a big problem here given how few people use this form)
<jtv> sinzui: one of those rough edges...  I put the extra information at the bottom of the page, below the form buttons.  I'm not sure if that's acceptable or not.
<sinzui> There are examples like that already
<jtv> phew :)
<jtv> the other thing is that the first <li> of the <ul> is getting a little too cozy with the line above the <ul>
<sinzui> jtv: but as you know, most forms would put that information about the form. How is this information secondary, or distractive that implies it should be below?
<sinzui> in webkit there is 300px between the form field and the submit buttons. I do not see where the markup error is yet
<jtv> sinzui: I didn't want to put them at the top because they would force scrolling on an admin who goes through the current workflow; and I didn't put them on the side because this is a generic form with a lot of JS to hide and show sections, and I'm not sure how safe it would be.
<jtv> sinzui: I'm not too worried about webkit, since we've really only got about 4 people who use this form regularly and perhaps the same number again who can access it.
<sinzui> jtv: the error usually indicates that there is a deep error the firefox is not showing you yet.
<sinzui> jtv: for example: mailing list moderate was broken for webkit, so it was ignored for 4 months, when I fix it, other submit errors in firfox were also fixed
<jtv> sinzui: in this case it may be something to do with the existing JS machinery in the form...  nobody would have noticed that, I think
 * jtv tries
<jtv> sinzui: definitely not 300 pixels for the form on staging... I'll try to reproduce on dev.  Meanwhile, try selecting a different filetype and watching the JS reshape the form.  It may tell you something.
<jtv> sinzui: oh!  On staging, in konqueror, I get those 300 px or so, right in the middle of the form.
<jtv> Depending on what I do with that "file type" dropdown
<jtv> sinzui: so that's a pre-existing bug, very low-priority because of the limited usage of this page.
<sinzui> right. The: markup is well formed, but the error implies the table is ill.
<jtv> It might just have something to do with the sluggy response I've been getting from firefox on this form.
<jtv> *sloggy
<sinzui> jtv: your changes are good too land. I have a minor dislike to the whitespace between the two columns you added. I do not see it normally because I keep my browser narrow. So long as you admins don't mind the large gutter of white-space, land away
<jtv> sinzui: yay!  thanks
<jtv> sinzui: no mentor approval needed?
<jtv> "sluggish," _that_ was the word
<sinzui> Anther UI is needed, but not necessarily noodles775. We are using the buddy system where two crazed developers can approve the UI is a suicide pact.
<bac> hi sinzui: https://code.launchpad.net/~bac/launchpad/bug-490521/+merge/19133
<sinzui> jtv; the broken edit icon is insane markup, not javascript
<jtv> sinzui: what went wrong?
<sinzui> The anchor is around a span that uses class="invisible-link". The CSS instruction means no not render. firefox is ignoring the instruction. The css does mean do not render because we use it to insert data for scraping, not seeing
<jtv> sinzui: wow, no idea how that got there... unless it was as a way to hide the icon from non-admins.
<sinzui> We created it for diagnostic testing of link rendering
<jtv> sinzui: hmm... afaics only the span *inside* the link is invisible-link.  There should still be an edit sprite, and I think that is the lazr-js bug
<sinzui> jtv: the issue is a little more complex that that. I will find the bug in rosetta and update it with the correct fix
<sinzui> jtv: I do not see any javascript
<sinzui> jtv: is something rewriting the link?
 * sinzui see the template and markup look like simple html
<jtv> sinzui: no, it's done in TAL after all... there's a doctest using the invisible link though.
<sinzui> jtv: oh, the fix is trivial, it is the white-space inside the anchor that is wrong
<sinzui> intellectronica: had me fix a similar issue two weeks ago
<sinzui> jtv: the <span> must abut the <a>:
<sinzui>                     <tal:block condition="entry/required:launchpad.Admin">
<sinzui>                       <a class="sprite edit"
<sinzui>                          tal:attributes="href entry/fmt:url"><span class="invisible-link">Change
<sinzui>                          this entry</span></a>
<sinzui>                     </tal:block>
<jtv> sinzui: DON'T tell me the CSS does something with :first here...
<sinzui> I am unsure what the issue bug the fix is trivial:
<jtv> no whitespace between the <a> and the <span>.  Gotcha.
<sinzui> dude!
<sinzui> span.invisible {left:-9999em;display:block;}
<jtv> ?
<sinzui> The link was pushed passed the phone on my desk
<sinzui> sorry. looking at the wrong entry: span.invisible-link {display:none;} is as I would expect
 * jtv was wondering since when css classes had inheritance  :-)
<jtv> sinzui: I'm filing a bug for this... thanks, because I can confidently say I would not have figured this out
<sinzui> bac: I think your template is missing the display of translations
<jtv> sinzui: was that meant for me?
<bac> jtv: i think it was for me
<sinzui> jtv: no. bac is working on a branch that shows what upstream information is missing for a source package : http://people.canonical.com/~bac/upstream-portlet.png
<bac> jtv: at least my bell went off
<jtv> ah ok, just checking because of the T word and proximity to conversation with YT.  :)
<sinzui> If the source package has translation templates, then we need to check if auto imports were enabled for the series branch
<sinzui> jtv: I pondered the confusion as I was typing it
<bac> sinzui: ok.
<sinzui> bac: this might work: http://pastebin.ubuntu.com/374223/
<jtv> sinzui: I don't suppose you'd also like to review this one?
<sinzui> your branch from a few weeks ago?
<jtv> sinzui: no, this fix you just gave me
<jtv> sinzui: mainly interesting for the before/after pictures: https://code.launchpad.net/~jtv/launchpad/bug-520659/+merge/19140
<sinzui> jtv r=me
<jtv> sinzui: thanks :)
<sinzui> jtv: I think we can ignore konquerer issues when Lucid is released because the default will be webkit-based
<jtv> sinzui: for this, that means we can start ignoring konqueror issues when lucid is in late beta.  :-)
<sinzui> jtv: there are about 20 such bugs we can mark as wont fix
<jtv> sinzui: so the support for konqueror is basically, "whatever is current in ubuntu"?
<sinzui> I think we need to take traffic in consideration...IE6 still haunts US. konqueror is small ans appears to follow the latest release
<sinzui> jtv: Chrome is webkit and Epiphany switched to webkit. so we are seeing a large increase in webkit; we once felt pretty comfortable saying we will not fix safari bugs quickly.
<jtv> sinzui: I'm surprised about epiphany
<sinzui> not me, gecko was always a pain to build and keep up with. gecok was created for the mozilla stack, not for embedding
<jtv> I suppose a mature but "fresh" engine without too much historical baggage has its attractions...
<jtv> as ddaa remarked the other day, things would be very different if xhtml had arrived in a time like this when browsers are actually evolving
 * jtv -> afk
<bac> sinzui: more better?  http://people.canonical.com/~bac/upstream-portlet.png
<sinzui> wow, yes
<sinzui> bac: you lost the edit link, and in reflection, I think that indicates that it was not very clear...
 * bac goes back to look at devel
<bac> sinzui: so you're talking about the edit link on the series, correct?
<sinzui> bac: I think we should move the link to edit the Packaging link to the horizontal list where we can use a clear label. Maybe "(/) Change upstream link"
<sinzui> bac: yes, that was editing the link, but it was not clear that you could change the project and series.
<sinzui> oh, and I lost the icon in a past revision. I suck
<bac> sinzui: so change "Show upstream links" to "Change upstream link" and put it to the right of "View changelog"?
<sinzui> umm, no
<bac> yeah, you've lost me then
<sinzui> The link in the horizontal list is goes to the right place, but it is missing an icon (i) -- surely that is my mistake
<sinzui> bac: Instead of restoring the link you lost in the diff, we should move it to this same horizontal ul and use "Change upstream link" to make it very clear that the user can change the project and series he knows it is wrong
<bac> sinzui: ring ring
<bac> the mic works
#launchpad-reviews 2010-02-12
* maxb changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles775, maxb<https://code.launchpad.net/~maxb/launchpad/devel-10306-to-db-devel-resolve-conflict/+merge/19154>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<maxb> I expect the next automatic merge from stable to db-devel to fail with a conflict. The branch I've just put in the topic is that merge with the conflict resolved
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [noodles775, maxb<https://code.launchpad.net/~maxb/launchpad/devel-10306-to-db-devel-resolve-conflict/+merge/19154>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: I don't see any branch from you that needs a review
<noodles775> adeuring: https://code.edge.launchpad.net/~michael.nelson/launchpad/509370-access-non-unique-ppa-name/+merge/19022
<noodles775> It's got a ui and text approval (I've got a minor ui tweak to do, bit it's just a letter change).
<noodles775> Thanks!
<adeuring> noodles775: argh, right. I just saw curtis's name and thought "that's reviewed"...
<noodles775> adeuring: oh, and btw, there are pre-imp discussions on the bug linked from the MP.
<adeuring> noodles775: r=me
<salgado> noodles775, where's your branch?
<noodles775> salgado: adeuring just approved it.
<salgado> oh, ok.  I'll remove it from the queue
* salgado changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [maxb<https://code.launchpad.net/~maxb/launchpad/devel-10306-to-db-devel-resolve-conflict/+merge/19154>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adeuring, have you started on maxb's branch as well?
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [maxb<https://code.launchpad.net/~maxb/launchpad/devel-10306-to-db-devel-resolve-conflict/+merge/19154>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> salgado: yes, I've started reh review of maxb's branch
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: maxb, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adeuring, I've commented there, but I think noodles775 has landed a branch doing that merge and solving the conflict already
<adeuring> salgado: yeah, I've started to look at the current revision of db-devel too ;)
<adeuring> maxb. I'm removing your branch from the queue, because another branch from noodles775 already resolved the conflicts. But thanks for your work nevertheless!
<maxb> np
* adeuring changed the topic of #launchpad-reviews to: call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> bigjools: Can you please have a look at https://code.edge.launchpad.net/~wgrant/launchpad/sprbu-columns-to-sprb/+merge/18995?
<bigjools> yep
<bigjools> wgrant: looks fine
<bigjools> you wanna add some model code with that?
<wgrant> bigjools: That's the next branch.
<bigjools> ok
<wgrant> I hear one is meant to split them up.
<bigjools> we'll wait for stub thenm
<bigjools> well
<bigjools> you don't really need to now we have db-devel
<wgrant> Is stub not around at the moment?
<wgrant> Ahh.
<bigjools> holiday
<bigjools> I am blocked on him too
<bigjools> my no-more-secpub apocalypse
<bigjools> I would go ahead and add the model code to this branch
<wgrant> Ah.
<wgrant> I already have the other branch up there.
<bigjools> unless you have a good reason not to
<wgrant> Shall I make an MP?
<bigjools> yep
<bigjools> oooo
<wgrant> Hm?
<bigjools> the js scrolls the MP window smoothly to the "add comment" section
<bigjools> nice
<wgrant> Gecko's interpretation of "smooth" does not meet most, unfortunately.
<bigjools> heh
<bigjools> seemed ok for me - perhaps KDE handles it better?  <hides>
<wgrant> Hm, some sprites are broken on edge.
<wgrant> eg. the 'Proposed branch' and 'Merge into' on MPs.
<bigjools> yeah I see that too
<wgrant> Both Chromium and Firefox as well.
<bigjools> and Konq
<jtv> adeuring, got time for this easy one?  https://code.launchpad.net/~jtv/launchpad/bug-411514/+merge/19168
<adeuring> jtv: sure
<jtv> thanks
<jtv> noodles775, have you got time for a buildfarm-related review?
<jtv> noodles775: it's fairly large: https://code.edge.launchpad.net/~jtv/launchpad/bug-507678/+merge/19131
<noodles775> jtv: not right now, but isn't it something the OCR could do?
 * noodles775 takes a quick look
<jtv> noodles775: I was hoping for someone from the Wellington sprint
<noodles775> hrm, text conflict on that MP?
<jtv> noodles775: yes... and I'm not getting it with my devel.
<noodles775> jtv: I'd be happy to look at it next week, but not today.
<jtv> noodles775: that's fine, thanks...  I'll see who's available later
<noodles775> ok
<adeuring> jtv: r=me
<jtv> adeuring: thanks!
<jtv> more oopses bite the dust... our only ones atm I think
<EdwinGrubbs> adeuring, salgado: can one of you review my branch. It looks big, but a large part was just indenting code, and the rest is very simple. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-513260-registry-js-module-names/+merge/19142
<adeuring> EdwinGrubbs: sure
<adeuring> EdwinGrubbs: r=me (1 nitpick)
<EdwinGrubbs> adeuring: thanks
<allenap> adeuring: Up for a short branch? https://code.edge.launchpad.net/~allenap/launchpad/launchpad-naughty-naughty-comment-sync-bug-499113/+merge/19181
<adeuring> allenap: sure
<allenap> adeuring: Thanks.
<gmb> adeuring, salgado-brb: Either of you available to review https://code.edge.launchpad.net/~gmb/launchpad/blobjob-cronscript-bug-513190/+merge/19184?
<adeuring> gmb: I'll do it, but have first to finish a review for Gavin
<gmb> adeuring: Brilliant, thanks. No huge rush.
<adeuring> gmb: The MP shows  a merge conflict. I assume the branch should land on devel, not db-devel?
<adeuring> allenap: r=me
<gmb> adeuring: Arse. No, it has to land on db-devel, but I think I originally branched from devel in a moment of craziness. I'll post a proper diff, hang on...
<adeuring> gmb: thanks!
<allenap> adeuring: Thank you!
<gmb> adeuring: Non-crazy diff posted.
<adeuring> gmb: Checking "uuid is None" is good -- but shouldn't we also add a notice when this happens. Something like "sorry, semthing went wrong with storing the debug data. The OOPS ID is..."?
<adeuring> gmb: argh, now I see it -- we _have_ these messages...
<adeuring> sorry for the noise
<adeuring> gmb: r=me
<gmb> adeuring: Thanks.
* adeuring changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-14
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/initial-mp-email-text-alignment/+merge/19303 if you have a minute
<mwhudson> thumper: done
<thumper> mwhudson: awesome, thanks
#launchpad-reviews 2011-02-09
<mrevell> rning all.
