#launchpad-reviews 2010-04-12
<mwhudson> thumper: want to review this code import branch?
<thumper> mwhudson: ok
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/code-import-optimizations/+merge/23206
<mwhudson> thumper: the stacking idea didn't work
<thumper> ok
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/fix-schema/+merge/23209
<thumper> I have another too that I've been meaning to fix for ages:
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/scanner-awesomeness/+merge/23211
<mwhudson> thumper: "ackward" is a typo
<mwhudson> (second one)
<thumper> :)
<mwhudson> thumper: also assertFoundRevisionNumber and test_not_found are separated by two lines, should be one
<mwhudson> + self.assertIs(None, iterator.iterator)
<mwhudson> seems to be testing an implementation detail really
<thumper> it is
<thumper> but I used to to confirm that it was broken
<thumper> not working as intended
<thumper> working, but weirdly
<thumper> I can remove it
<mwhudson> thumper: i think it would be slightly better
<mwhudson> (the test principle is "go in by the front door" or something)
<thumper> better to remove it?
<mwhudson> please
<mwhudson> thumper: some other small comments on the merge proposal
<thumper> mwhudson: thanks
<thumper> W
<thumper> T
<thumper> F
<thumper> :(
<thumper> ââ¹
<thumper> mwhudson: parallel iteration fails
<thumper> mwhudson: should I just document and ignore?
<thumper> mwhudson: or I could make it more like the recipe at http://code.activestate.com/recipes/576941-caching-iterable-wrapper/
<mwhudson> thumper: hmm
<mwhudson> thumper: does it fail noisily?
<thumper> mwhudson: no just very incorrectly
<mwhudson> thumper: :(
<mwhudson> oh i guess self.data is shared between the iterators
<thumper> no I understand the activestate recipe more
<thumper> it is licenced under MIT
<thumper> does it really matter?
<mwhudson> i'm a bit torn
<mwhudson> because clearly for your usecase it doesn't matter, no
<mwhudson> but leaving bombs in the source like this to unexpectedly blow up at people doesn't sound good
<thumper> make it similar?
<mwhudson> thumper: i think that activestate recipe is still a bit more complicated than you need
<mwhudson> (e.g. pickling support)
<thumper> yeah, I didn't care about that
<thumper> nor the __new__ optimisation
<thumper> but the indexing is useful
<mwhudson> well, the indexing stuff is what makes it work in parallel isn't it?
<thumper> uh ha
<mwhudson> um, maybe not
<thumper> actually we can't make it threadsafe
<thumper> but that shouldn't matter
<mwhudson> that's ok i think
<mwhudson> because if you're using threads, you should expect your program to be confusing anyway
 * mwhudson runs away
<thumper> heh
<thumper> ok, I have parallel bits working
<thumper> mwhudson: http://pastebin.ubuntu.com/412936/
<mwhudson> thumper: "# Defer to the generator" <- i think you mean iterable?
<thumper> mwhudson: yeah
<mwhudson> thumper: otherwise looks fine
<jtv> Anybody willing to review a slightly oversized buildfarm branch?
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-544237/+merge/23214
<jtv> morning henninge!
<henninge> HI jtv! ;)
<jtv> henninge: looking at your review changes now.  Had to get an oversized branch finished first.
<henninge> jtv: another one? ;-)
<jtv> Yes, slave build cookies.
<jtv> henninge: another comma issue (though this time just a suggestion): instead of ":return:Â theÂ entryÂ orÂ NoneÂ ifÂ processingÂ failed" it would be clearer to say ":return:Â theÂ entry,Â orÂ NoneÂ ifÂ processingÂ failed."
<jtv> (Makes the scoping clearer)
<jtv> henninge: I see you cut down this assertion:
<jtv> 53Â Â Â Â Â Â Â Â #Â ProductseriesÂ andÂ distroseriesÂ willÂ beÂ assertedÂ inÂ getSubsetÂ but
<jtv> 54Â Â Â Â Â Â Â Â #Â notÂ sourcepackagename.
<jtv> 55Â Â Â Â Â Â Â Â assertÂ (distroseriesÂ isÂ NoneÂ andÂ sourcepackagenameÂ isÂ NoneÂ or
<jtv> 56Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â distroseriesÂ isÂ notÂ NoneÂ andÂ sourcepackagenameÂ isÂ notÂ None),Â (
<jtv> 57Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "PleaseÂ specifyÂ distroseriesÂ andÂ sourcepackagenameÂ together.")
<henninge> yes ... ?
<jtv> The comment is useful for reviewers, but I wouldn't bother with it in real-world code.  May I suggest simplifying the condition to "(distroseries is None) == (sourcepackagename is None)"?
<jtv> BTW lots of good changes here, thanks.
<henninge> jtv: I am not sure that "Noneness" concept is clearer to read.
<jtv> You could even just say "distroseries is None or sourcepackagename is not None"
<henninge> that I like ;)
<jtv> It's not quite as tight, but I don't think the case that it ignores matters much.
<henninge> hm, which one is that?
<jtv> Where the caller specifies a sourcepackagename but not a distroseries.
<jtv> (Sounds bad, but combined with the checks in the utility...  anyway, these are assertions, not supposed to trigger at all)
<henninge> well, in that case the assertion in SubSet will trigger.
<jtv> Right, combined with the checks in the utility it doesn't matter.
<jtv> Also in the approver, "BecauseÂ aÂ genericÂ pathÂ (i.e.Â messages.pot)Â doesÂ notÂ provide [...]"âthere are other generic paths besides messages.pot, right?  So use "e.g." not "i.e."
<jtv> henninge: I'm off for a quick bite.  See comment above.  Also, could you give len_templateset a name that can stand on its own?
<jtv> henninge_: did you see my notes about "i.e." vs. "e.g." and about len_potemplateset?
<henninge> jtv: yes, all incorporated
<jtv> henninge: great, thanks
<henninge> Notes Inc.
<henninge> ;-)
<jtv> heh
<jtv> henninge: I'm going through your changes one commit at a time, so it's possible that I'll see some things that you've already changed.
<jtv> wgrant: the big problem now is to get a reviewer for the cookie branch.
<noodles775> intellectronica: are you around for reviews today?
<wgrant> jtv:  :(
<wgrant> It's only a little oversized.
<wgrant> And then only be s/ID/Cookie/..
<wgrant> s/be/by/
<jtv> wgrant: yes, but the people I've approached so far wouldn't have had time for 700 lines either.
<noodles775> jtv: put it in the queue now :)
<intellectronica> noodles775: sure
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> intellectronica: got one for ya!
<intellectronica> noodles775: which branch?
<jtv> It's slightly oversized though: https://code.edge.launchpad.net/~jtv/launchpad/bug-544237/+merge/23214
<noodles775> intellectronica: I was actually trying to get jtv to ask you to review his branch :)
<jtv> (Then again, 888 is a very lucky number according to the Chinese)
<noodles775> intellectronica: thanks.
<jtv> for which, noodles775, thank you.  :)
<intellectronica> jtv, noodles775: np
<jtv> \o/
<jtv> thanks
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> jtv: r=me
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> intellectronica: wonderful, thanks!
<jtv> henninge: done with your branch.  Phew!
<henninge> jtv: great, thanks!
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: {lunch} || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> intellectronica: hi. can you please send this branch to ec2-test https://code.edge.launchpad.net/~adiroiban/launchpad/bug-548999/+merge/22271? thanks!
<intellectronica> adiroiban: sure
<jelmer> t/opic
<jelmer> argh
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> intellectronica, abentley -- I have a branch for review when one of you has availability.
* deryck changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, - || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> deryck: i can review
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: deryck, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> intellectronica, great, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/enable-test-inline-subscriber/+merge/23230
<intellectronica> deryck: r=me
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> intellectronica, great, thanks!
<leonardr> intellectronica or abentley, please review https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/user-agent/+merge/23237
<intellectronica> leonardr: on the phone. will have a look later if abentley doesn't pick it up
<leonardr> all right
<abentley> leonardr, looking.
<abentley> leonardr, have you considered using standard http header parameters instead of your own syntax?
<leonardr> abentley: you mean like matrix parameters? or is there something specific for headers?
<leonardr> oh, i bet you're talking about the parameters that go eg. with an uploaded file
<leonardr> afaik that's only used in Content-Disposition, but i could be wrong and that might be more readable
<abentley> I mean like encoding in "Content-type: text/plain; encoding=utf-8"
<leonardr> ok, yes, it's in a number of places
<abentley> leonardr, (actually, a correct example is "Content-Type: text/html; charset=ISO-8859-4")
<abentley> leonardr, yes it's used in a number of places, and IIRC, Python supports querying it.
<leonardr> abentley: how about this:
<leonardr> User-Agent: launchpadlib 1.6.1, lazr.restfulclient 1.0.0; oauth_consumer=apport; +cache-control
<leonardr> or cache-control=true
<abentley> leonardr, that looks good.  I think I prefer cache-control=true.
<leonardr> ok
<leonardr> might get lengthy
<abentley> leonardr, do you actually need cache-control, or could you infer it from the lazr.resfulclient version?
<leonardr> abentley: i would rather not infer it for two reasons
<leonardr> 1. i'd rather write server-side code to look for a string rather than parse a version number and compare against some known version number
<leonardr> 2. other people are writing clients (eg. in java) and i want them to be able to use these features
<leonardr> i don't want a situation where they pretend to be lazr.restfulclient, the way all browsers pretend to be "Mozilla"
<abentley> leonardr, it sounded like you were working around a bug in earlier versions of lazr.restfulclient.  Am I wrong?
<leonardr> abentley: i'm working around a current bug in httplib2. so in that sense, old versions of lazr.restfulclient will have a problem and new versions will not
<leonardr> but the bug is only triggered if the server sends certain information
<leonardr> so the server needs to distinguish clients that it can send the information to, from clients that will choke on it
<abentley> leonardr, so it seems unlikely that an arbitrary client (e.g. in Java) would have the same bug.
<abentley> leonardr, but you're suggesting that all implementations which don't have that bug should assert that they don't.
<leonardr> abentley: here's the problem
<leonardr> old versions of lazr.restfulclient, already installed on thousands of computers, will not send any special user-agent
<leonardr> the server's response to 'no special user-agent' needs to be 'don't send the data'
<leonardr> a java client will not have the same bug, but it needs to be able to send a special user-agent to get the special data
<leonardr> i could send the special data to any user-agent that's not 'python/httplib2'
<leonardr> but even then, someone else writing a non-lazr.restfulclient httplib2 user agent would like to be able to get the special data
<leonardr> anyway, i'm willing to get rid of cache-control=true because the bug fix for it coincides with the introduction of a real user-agent system
<leonardr> but if something similar happens in the future i will insist on something like cache-control=true
<abentley> leonardr, that seems unfair to pre-existing implementations that don't have the bug.  Would disable a feature for all installed lazr.restfulclient versions because a Java implementation's support for it was broken?
<leonardr> abentley: what specifically seems unfair?
<leonardr> don't send the data unless a client specifically requests it?
<leonardr> that's why i proposed some alternate solns
<abentley> It seems unfair to disable, say, http pipelining to all implementations that have already been using it successfully because it later turns out one implementation is broken.
<abentley> until the existing implementations update their version strings to say that their implementation isn't broken.  Which might be incorrect.
<leonardr> i don't want to get into the business of parsing version numbers in referer headers, especially of software i don't control
<abentley> What do you mean by referrer headers?  I thought we were talking about user-agents?
<leonardr> sorry, user-agents
<leonardr> i was thinking of another time a lot of clients broke because of a change involving Referer
<leonardr> If version 1.5, and i have no idea when it's going to be fixed, how do i do the restriction?
<leonardr> (if version 1.5 is broken)
<abentley> leonardr, so the alternative is disabling features that were previously enabled, until clients are updated to say they support them, right?
<leonardr> i don't think a feature that was previously enabled would ever become disabled
<leonardr> cache-control is a new feature that exposed an old bug
<abentley> leonardr, I see.  I didn't get that before.
<leonardr> so it's more a matter of not giving clients a feature until they prove they're not one of the clients that crashes on it
<leonardr> anyway, if you want the point is moot because i can simply have the server deny this feature to python/httplib2 clients in general
<abentley> leonardr, Ah, okay.
<leonardr> and we can take up a more general solution the next time this happens
<leonardr> but i'm going to be denying this feature to _all_ python/httplib2 clients, even future ones that have the bug fixed, because i don't control httplib2 and i don't know when the bug will be fixed
<leonardr> once the bug is fixed i can go back and change that, but that's a pain
<abentley> leonardr, I think it's the best course though.
<abentley> leonardr, I agree that it can make sense for clients to specifically request features.  I wouldn't want to see the mechanism used to address implementation bugs, though.
<leonardr> clients generally request features by sending http headers. the only reason to do it in the user-agent instead is if there's an agent-specific problem
<abentley> leonardr, btw, the python support for header parameter is get_param and set_param, described here: http://docs.python.org/library/email.message.html
<leonardr> ok
<abentley> So I'm going to do a needs-fixing review and recommend using standard parameters and just blacklisting python/httplib2 clients for cache-control.  Sound reasonable?
<james_w> leonardr: is this about caching the wadl?
<leonardr> james_w: yes
<leonardr> abentley, sure
<james_w> leonardr: may I have a look at the merge proposal?
<leonardr> james_w: sure. https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/user-agent/+merge/23237
<james_w> please subscribe me to the source branch, that page is private
<leonardr> it has nothing directly to do with the wadl, it's just creating the ocnditions under which i can land a lazr.restful branch without breaking everything
<leonardr> well, it shouldn't be
<leonardr> ok, it's public now
<james_w> I'm not sure this is worth the effort of a workaround is it?
<james_w> I'm not sure what the "special data" is though
<james_w> I thought it was fixed by removing Cookie from Vary in webservice requests?
<leonardr> james_w: if there's no workaround, then all existing installations of launchpadlib break when i upgrade lazr.restful
<leonardr> this is separate from Vary. look at the httplib2 bug
<james_w> oh, none of this is in the bug report it seems
<leonardr> james_w: i don't follow. which bug report are you refering to?
<james_w> the lazr.restfulclient one
<leonardr> james_w: if it will help i'll file a separate lazr.restfulclient bug saying 'work around httplib2 bug 97'
<mup> Bug #97: can't mark a bug assigned <Launchpad Bugs:Fix Released by daf> <https://launchpad.net/bugs/97>
<james_w> leonardr: no, it's ok, I was just working from a stale cache (fnar)
<leonardr> ok
<james_w> so you want to introduce cache-control in lazr.restful?
<leonardr> james_w: yes. and httplib2 gives inconsistent behavior when cache-control is combined with conditional requests
<leonardr> inconsistent behavior that crashes lazr.restfulclient
<james_w> right
<james_w> I can see why you need the workaround now
<james_w> sorry for the noise
<leonardr> sure
* sinzui changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> abentley, can you review https://code.edge.launchpad.net/~sinzui/launchpad/series-branch-target/+merge/23056
* intellectronica changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> abentley, This branch is a complete reimplementation of a solution that had a fatal flaw
<leonardr> abentley, ready for you to take another look
<abentley> leonardr, r=me
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: On call: abentley || reviewing: sinzui || queue: [adiroiban(bug-561355)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> intellectronica: any new about my branch sent to ec2 ?
<intellectronica> adiroiban: it dies mysteriously and i had to resend it, so it will be another 2-3 hours until it finishes
<adiroiban> intellectronica: ok. then most probably there are some errors
<adiroiban> and to see them you will need to keep the session open
<adiroiban> as no email will be dispached
<intellectronica> adiroiban: i'll try a run with a terminal open if i don't get a response and will let you know if there are errors
<adiroiban> thanks!
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: adiroiban(bug-561355) || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> thanks abentley
<abentley> sinzui, np.
* abentley changed the topic of #launchpad-reviews to:  On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> abentley, ping, would you be comfortable reviewing https://code.edge.launchpad.net/~mars/launchpad/check-db-revision-target/+merge/23255 ?  It is a small Makefile change.
<mars> you know, I would really like to use Bash colour escape codes in there...
<abentley> mars, did you consider just doing make schema?
<abentley> mars, rather than erroring, I mean.
<mars> abentley, I thought about that, but the problem is that you don't always want 'make schema' stomping on the database.  The scenario:
<mars> I have a devel instance running over here: switch contexts to fix a bug over there.  Need to start a dev instance, so I try "make run" in the bugfix branch
<mars> if "make schema" ran automatically on the mismatch, it would stomp my original dev instance
<abentley> mars, so what do you do instead of running make schema to do the bugfix?
<mars> abentley, find the original instance, make sure I can shut it down, do so, then spin up the bugfix instance and db.
<mars> abentley, ugly, but this is a quick-fix that will save a few annoying minutes of my life.
<abentley> mars, does the check itself take very long?  make run seems to be taking longer and longer...
<mars> maybe auto-running 'make schema' would work, too, but I did not want to spend too much time on it.
<mars> abentley, the check takes almost no time, especially compared to other stuff.  'make jsFOO' takes much longer.
<mars> guess what is next on the hit-list
<mars> :)
<abentley> mars, cool.
<mars> small improvements!  One step at a time.
<abentley> mars, does the exit status propagate to the make exit status?  If so, it would be nice to use a unique exit status.
<mars> abentley, I don't know.
<abentley> mars, is it deliberate that there's no newline at the end of utilities/check-db-revision.py ?
<mars> abentley, nope.  My editor must not add them
<abentley> mars, weird.  Okay, r=me.
<mars> abentley, great, thank you!
<bac> hi abentley, can i get a review?  https://code.edge.launchpad.net/~bac/launchpad/bug-559200/+merge/23256
<abentley> bac, looking
<abentley> bac, these doctests look a lot like unit tests.
<abentley> bac, why is the new transaction.commit needed?
<abentley> (patch line 128)
<bac> abentley: without the commit the branch conflict does not occur
<abentley> bac, r=me
<bac> abentley: thanks
<bac> abentley: the testing style is consistent with the rest of the registry browser view tests.  each application has settled on a different testing style...
<abentley> bac, I thought it had been agreed that the doctests should be documentation that had tests.
<leonardr> abentley: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/httplib-97-workaround/+merge/23263 , though you might want to wait for my change to the lazr.restful branch
<abentley> leonardr, are If-Modified-Since and If-None-Match the only ways of getting a 304?
<abentley> leonardr, it's generally not a good idea to sleep in tests.  Can you modify the apparent time seen by CookbookWebServiceClient instead?
<leonardr> abentley: yes, except for a case with intermediate caches which doesn't apply here
<leonardr> do you have any suggestions on how to modify the apparent time?
<leonardr> i don't have direct access to the code that checks the time, it's in httplib2
<abentley> leonardr, then I guess it can't be helped.
<abentley> leonardr, what's the motivation for checking whether the client performed a conditional request?
<leonardr> abentley, look at the end of the diff
<leonardr> where it used to make a conditional request and expect an exception
<leonardr> now it gets a special value and i don't use an exception for a non-exceptional case
<abentley> I understand that.  I just don't understand why you would ever want a 304 to give an exception.  Do servers really return it when they shouldn't?
<leonardr> abentley: no, they don't. that's why if it were to happen it woudl be an exceptional situation
<leonardr> basically, if for some reason it does happen
<leonardr> i don't want lazr.restfulclient to say 'oh, the object's new representation is NOT_MODIFIED'
<leonardr> i want it to crash
<bac> abentley: it depends on what you call a doctest in registry/docs that are as you'd expect.  we also have these view tests in browser/tests that use the doctest framework but are not primarily meant to be documentation.
<bac> oops, i missed some important punctuation in that first run-on sentence.
 * bac tries again
<bac> abentley: it depends on what you call a doctest.   in registry/docs we have doctests that are as you'd expect.
<abentley> leonardr, that's a very different situation from a 302 or a 500.  You're now talking about bugs in the server or a proxy.  Still, if you think it's important to do that, you can.
<leonardr> abentley: i think it's important insofar as i want to preserve the old behavior (raise an exception whenever the response is not 200)
<leonardr> i don't want to carve out a general exception for 304, just an exception for 304 _when i know how to handle it_
<abentley> bac, I don't understand why you would make an exception for certain doctests.  Is there something about the doctest framework that makes it superior to unit tests for views?
<bac> abentley: i'm just trying to explain the history of how we got to where we are.  the code team has a strong preference for unit tests.  other teams prefer the doctest framework.  i think the most important thing to do is to be consistent within the prevailing subculture.
<bac> abentley: unless or until we make a launchpad-wide decision mandating one way or the other.
<abentley> bac, I think it's also important to respect the decisions that have already been made.  I thought there was consensus that we should only use the doctest framework for testable documentation.
<abentley> bac, I'll add it to the reviewer meeting agenda.
<bac> abentley: ok
<EdwinGrubbs2> abentley: can you review a fairly simple branch for me?
<abentley> EdwinGrubbs, sure.
<EdwinGrubbs> abentley: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-555734-coc-oops/+merge/23268
<abentley> EdwinGrubbs, r=me.
<EdwinGrubbs> thanks
 * maxb has 2 trivial branches if anyone's still reviewing - https://code.edge.launchpad.net/~maxb/launchpad/ - but I'm assuming EOD must be nowish for Americas
#launchpad-reviews 2010-04-13
<mwhudson> maxb: looks like 3 to me, i guess you'd like me to ec2 land them all?
<maxb> it's 3 now :-)
<maxb> And yes, please land any you approve, thanks
* 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
* adiroiban changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [adiroiban(bug-525992)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mwhudson> maxb: defeated by testfix
<mwhudson> sigh
<mwhudson> anyone here?
<maxb> Thanks for trying, on the strength of that I can probably get them resubmitted direct-to-pqm later
<mwhudson> i'm on it
<stub> https://code.edge.launchpad.net/~stub/launchpad/pending-db-changes/+merge/23116
<stub> https://code.edge.launchpad.net/~stub/launchpad/testsuite/+merge/23282
<stub> The first is mainly deletions and improves staging rebuild for any reviewers interested in that.
<stub> The second removes all the now unnecessary commit's from the testsuite factory
<mwhudson> stub: woooooooo
<mwhudson> stub: does your testsuite branch make the test suite notably faster?
<stub> Dunno
<stub>  Total: 25933 tests, 0 failures, 0 errors in 181 minutes 57.297 seconds.
<stub> Is that faster from ec2?
<mwhudson> Total: 25929 tests, 0 failures, 0 errors in 229 minutes 18.851 seconds.
<mwhudson> looks like a 'yes'
<mwhudson> 1585	+# Explicitly state 'no permissions on these objects' to silence
<mwhudson> 1586	+# security.py warnings.
<mwhudson> yay!
<mwhudson> stub: both done
<stub> Ta
 * stub waits for his system to stop thrashing so he can land.
<mwhudson> stub: why is https://code.edge.launchpad.net/~stub/launchpad/testsuite private?
<stub> old, old branch
<mwhudson> ah
<mwhudson> want me to make it public?
<stub> Sure. Or I will in a tick.
<mwhudson> done
<mwhudson> no i want to experiment and see how many tests fail when we don't reset sequences between tests
<mwhudson> *now
<mrevell> Hello! Anyone care to take a look at a tour update branch for me? https://code.edge.launchpad.net/~matthew.revell/launchpad/10.04-tour-updates/+merge/23245
<wgrant> mrevell: "Git, Subversion or CVS" <- no Mercurial?
<mrevell> wgrant, Sorry, on the phone. Mercurial support is experiemental isn't it?
<wgrant> mrevell: It's been on production since February, and it isn't indicated in the UI as being experimental.
<mrevell> wgrant, Thanks for the heads-up. The code guys haven't mentioned it to me as being announceable. jelmer, is the Mercurial import stable enough to be publicly announced?
* sidnei changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [adiroiban(bug-525992), sidnei (bug-562173)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> mrevell: no, it's not ready to be announced yet
<jelmer> mrevell: the launchpad side of it works well, but bzr-hg itself has a couple of open issues that affect most of the imports
* jelmer changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [adiroiban(bug-525992), sidnei (bug-562173), jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [adiroiban(bug-525992), sidnei (bug-562173), jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Oh, it's Tuesday, isn't it?
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [adiroiban(bug-525992), sidnei (bug-562173), jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [sidnei (bug-562173), jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Oh, HI adiroiban! I just commented on your proposal and danilo on the bug ;-)
<danilos> adiroiban, I find the positioning horrible fwiw :) my suggestion is on the bug, but if you can come up with something better, I'd be happy to listen :)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lunch || queue: [sidnei (bug-562173), jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos: yes, it is not very visible. How about this: http://bayimg.com/LalBkAaco ?
<danilos> adiroiban, I can live with that if you provide a help pop-up as well
<adiroiban> danilos: hm... help popup for the first time a user click it?
<danilos> adiroiban, no, next to it ("What's this?")
<adiroiban> aha
<adiroiban> ok
<adiroiban> I'll do it
<danilos> adiroiban, also, do not change the default for the page, because that will bring us some annoyed people
<adiroiban> yes
<adiroiban> the default is not changed
<danilos> adiroiban, i.e. make it default to 'reviewer mode'
<danilos> adiroiban, cool, thanks
<danilos> adiroiban, also, it'd be good to check with our UI experts if green link is ok for this (it has immediate effect, but stores nothing in the DB; I am not exactly sure of the semantics, and it looks right, but please check with someone like noodles :)
<adiroiban> danilos: should I leave the bootom notice âWorking in translator modeâ ?
<danilos> adiroiban, I think it's useless to have it, and besides, nobody scrolls that far down anyway (except perhaps you :P)
<adiroiban> danilos: sure, as soon as you are ok with it I will ask an UI expert :)
<danilos> adiroiban, ideally, you'd be getting a completely different user interface (i.e. exactly what translators get today), but I am sure that's quite complex to implement considering how templates are crafted today
<adiroiban> danilos: it's ok if this code will be someday no longer needed.. but until then, it should help a few reviewer
<danilos> adiroiban, sure
<noodles775> Hi adiroiban, if the green link is just the .js-action class, then I think that's fine (eg. we use it already for expand/collapse sections that are only available via JS).
<adiroiban> noodles775: hi. it is just a span, with cursor:pointer
<adiroiban> it is not a real link
<noodles775> adiroiban: let one of us know when you're ready for a full UI review (in particular, rockstar and intellectronica are trying to build up their number of UI reviews).
<noodles775> adiroiban: Just like an expand/collapse segment?
 * noodles775 finds one.
<adiroiban> clicking on it will just set a cookie
<noodles775> adiroiban: oh, it doesn't update the page at all indicating the mode switch?
 * noodles775 hasn't read the bug.
<adiroiban> noodles775: http://bayimg.com/nAlBfaAcO
<adiroiban> this is the initial page
<adiroiban> and after clickin on it
<adiroiban> it will turn http://bayimg.com/NALBHAACo
* henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: lunch || queue: [sidnei (bug-562173), jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * noodles775 is glad the 'What's this?' is there, as that's exacly what he'd be clicking on ;)
<noodles775> adiroiban: so, assuming it's a js function, you should simply use the js-action class (which will style it green automatically). But grab one of the guys above for a full review when you're ready.
<adiroiban> noodles775: ok. thanks
* henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: sidnei || queue: [jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: sidnei || queue: [jelmer, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sidnei> heya henninge
<henninge> sidnei: Hi! just a quick question because I am not too sure about YUI
<sidnei> shoot
<henninge> sidnei: does "one" really work with queries that return more than one node?
 * henninge is probably just thinking of storm and database queries ...
<sidnei> henninge, one returns one node, all returns a nodelist. not sure what happens if you use one with a selector that matches more than one node. i suspect it returns the first one
<henninge> I hope it does ;-)
<henninge> sidnei: but I guess transforming query to one is a recommended migration path?
<sidnei> henninge, yes, query was renamed to one. queryAll was renamed to all
<sidnei> henninge, for reference: https://pastebin.canonical.com/30484/
<henninge> ah, just a renaming. That's easy then ... ;-)
<henninge> cool
<henninge> sidnei: r=me
<sidnei> henninge, thanks!
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: jelmer || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jelmer: which of the two reviews you have there do you want to have done (first) ?
<henninge> oh, one is ovesized
<henninge> ;)
<jelmer> henninge: :-)
<jelmer> henninge: The process-accepted-robust one please
<jelmer> henninge: that's the more important one at least, though it would be nice to get the other one reviewed too at some point :-)
<henninge> jelmer: I am just looking at the shorter one and see that it needs stormifying ... ;-)
<henninge> jelmer: also please see the style guide about truth conditionals
<henninge> https://dev.launchpad.net/PythonStyleGuide#Truth%20conditionals
<jelmer> henninge: oh, that's yet another one I think..
<jelmer> henninge: thanks
<henninge> jelmer: I added a comment https://code.edge.launchpad.net/~jelmer/launchpad/bug113563/+merge/23232
<henninge> jelmer: on to the big one.
<jelmer> henninge: is that the process-accepted-robust one?
<henninge> jelmer: yeah, just realized that's not it ...
<henninge> where is the merge proposal? Is it not for Launchpad?
<henninge> jelmer: ah, found it. At the bottom
<henninge> jelmer: is there a bug for this one? No branch without a bug! ;)
<henninge> needed for qa now
<jelmer> there is a related critical bug (about one item failing and breaking process-accepted completely) but not about this specific issue I think; I'll file one
<henninge> well, if it fixes the critical bug ....
<henninge> just link it
<henninge> jelmer: ^
<jelmer> henninge: ok, will do
<henninge> jelmer: what's special about the "ubuntutest" distribution?
<jelmer> henninge: not much, it's what's usually used for testing in soyuz
<jelmer> henninge: and we need to use the same distribution anywhere in those tests, hence my changing it
<henninge> jelmer: you mean in the whole TestCase or just the one testRobustness method?
<henninge> using the same distribution
<jelmer> henninge: in the whole testRobustness method we need to use the same distribution, but it shares a utility function with the other test on that testcase so I just opted to change that as well
<jelmer> alternatively I could've modified it to take a distribution argument and pass in a different value in each test
<henninge> jelmer: the reason behind my questions is that we try to cut down on the usage of sample data in the test and rather create what we need right there.
<jelmer> henninge: ah, ok
<henninge> if there is nothing special about "ubuntutest", you might as well just create a distribution from the factory on the spot
<henninge> or in the setUp
<gmb> henninge, Can I stick https://code.edge.launchpad.net/~gmb/launchpad/show-bwa-on-bw-pages-bug-558409 on your queue for a code review please?
<henninge> gmb: you can, Brad should be around soon, too, so it should get picked up.
<gmb> henninge, Ah, of course. Thanks.
* gmb changed the topic of #launchpad-reviews to: On call: henninge || reviewing: jelmer || queue: [wgrant, gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> intellectronica, mrevell: Could I trouble you two gents, as UI reviewers, to take a squizz at https://code.edge.launchpad.net/~gmb/launchpad/show-bwa-on-bw-pages-bug-558409/+merge/23308?
<mrevell> gmb, intellectronica is probably the better person to look right now as I've been out of the UI review loop for a few weeks.
<intellectronica> gmb: sure thing. i'll ask sinzui to mentor the review too
<gmb> mrevell, Fair enough.
<gmb> intellectronica, Thanks. I'll request reviews on the MP.
<intellectronica> gmb: any chance of screenshots?
<gmb> intellectronica, Sure. Coming right up.
<intellectronica> thanks
<jelmer> henninge: I'll see if that's possible, though I suspect the SoyuzTestPublisher still relies on sampledata too much
<henninge> jelmer: ah, so ubuntutest *is* special ... ;-)
<henninge> jelmer: in any case, you should just do that in setUp, either get ubuntutest or create one from the factory.
<jelmer> henninge: ok
<henninge> jelmer: if it is too much hassle to use one from the factory, go with ubuntutest but don't repeat it throughout the tests.
 * gmb hand-cranks launchpad.dev
<gmb> intellectronica, http://devpad.canonical.com/~gbinns/bwa-screenshot-1.png and http://devpad.canonical.com/~gbinns/bwa-screenshot-2.png
<intellectronica> gmb: lovely, thanks
<intellectronica> gmb: i think it can be improved by highlighting the last (current) status, but since this design is identical to the code imports ui, i'm not sure if we should change it at all
<gmb> intellectronica, Yeah, I think that the general thinking is that we should keep the same design.
<intellectronica> gmb: did you discuss that already with anyone before doing it?
<gmb> intellectronica, When I talked to deryck yesterday he mentioned that someone (jml, I think) was keen to see the code design re-used, and this has been mentioned in previous discussions.
<intellectronica> gmb: yes, i think it makes sense, and in that case, assuming we're not going to change the design on code pages too, i agree that leaving it like this is the pragmatic choice.
<gmb> Cool
<intellectronica> i still think it's worth finding a way of highlighting the last activity (both in code and in bugs). maybe later, though
<intellectronica> gmb: so ui*=me. let's see what sinzui thinks
<gmb> intellectronica, RIghto, thanks.
 * sinzui is pulling the branch now
<jml> gmb: yeah, it was me.
<gmb> jml, FTR, I was already planning to steal their design ;)
<jml> gmb: good good :)
* bac changed the topic of #launchpad-reviews to: On call: henninge, bac || reviewing: jelmer || queue: [wgrant, gmb] || 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: henninge, bac || reviewing: jelmer,wgrant || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jelmer: review sent
<jelmer> henninge: thanks
* henninge changed the topic of #launchpad-reviews to: On call: henninge, bac || reviewing: -,wgrant || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi wgrant -- you still around?
<wgrant> bac: Just.
<bac> wgrant:  i'll make this snappy then
<bac> i'm looking at your buildsequencer deletion branch
<wgrant> Yep, thanks.
<bac> i see there are still a lot of symbols in the config files.  shouldn't they go away too?
<wgrant> bac: See the commit message -- we can't remove them until they are removed from the production configs.
<wgrant> (yes, forgot to note that in the description...)
<bac> wgrant:  ok.  yeah, that's pretty subtle...   :)
<bac> wgrant:  r=bac and i'll land it for you.  have a good evening
<wgrant> bac: Thanks.
<jelmer> bac: Hi
<jelmer> bac: can I add another review request to your queue?
<bac> jelmer:  yes, of course
<jelmer> https://code.edge.launchpad.net/~jelmer/launchpad/refactor-permissions/+merge/23216
* jelmer changed the topic of #launchpad-reviews to: On call: henninge, bac || reviewing: -,wgrant || queue: [gmb, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: bac || reviewing: wgrant || queue: [gmb, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bac: I have to bow out already. See you tomorrow!
<bac> henninge:  ok, ttyl
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: gmb || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> gmb, ping
<gmb> sinzui, Hi
<sinzui> gmb, the bugwatch need a BreadCrumb adapter
<sinzui> is there a bug reported for that?
 * sinzui is searching now
<gmb> sinzui, Not AFAIK.
<sinzui> gmb: I think your change is fine, but we need bug reporting that the page is lost since it has no heading or breadcrumbs
<gmb> sinzui, Sure, I'll file one.
<sinzui> gmb: I do not think they get editing very often since I do not see bugs about it
<gmb> sinzui, They don't, except by us.
<sinzui> oh good
<sinzui> gmb, ui=me. I'll note we agreed to report a bug about the missing heading and breadcrumbs...there is no obvious parent for this
<gmb> sinzui, Thanks.
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: jelmer || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bac, Thanks for the review.
<bac> gmb:  have you put tuaw.com on your RSS reader now?  :)  i only kid b/c i'm jealous.
<gmb> bac, Nope, I spotted the news on Gizmodo. Shame the 13" isn't getting an i5, but I'm more concerned about RAM and HDD space than I am about processor speed.
<bac> hi jelmer
<jelmer> hi bac
<bac> jelmer:  could you merge your branch and fix the conflict?  i'm getting it when i try to merge though it doesn't show on the MP
<jelmer> bac: okay
<bac> thx
<jelmer> bac: I also have another branch up for review, https://code.edge.launchpad.net/~jelmer/launchpad/oops-on-pool-overwrite-error/+merge/23328
<jelmer> which is a bit smaller
<bac> jelmer:  ok
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: jelmer || queue: [jelmer] || 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: jelmer || queue: [jelmer-oops] || 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: jelmer-permissions || queue: [jelmer-oops] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> allenap, can i get you to take one more look at https://code.edge.launchpad.net/~leonardr/lazr.restful/cache-service-root/+merge/22948 ?
<leonardr> i'd also like you to review the corresponding lazr.restfulclient branch if you have time
<allenap> leonardr: Sure.
<leonardr> so i can get one person to look over the whole system
<leonardr> i'm working on the l.rc branch now
<allenap> leonardr: Cool, I'll do it when it's ready. I'm OCR tomorrow anyway.
<leonardr> should just be a few minutes ,i just have to write the mp
<leonardr> actually i already got it reviewed, but maybe you want to take a quick look
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/httplib-97-workaround/+merge/23263
<allenap> leonardr: What does the .replace("\xc3\xa7", "c") bit do?
<leonardr> allenap: it's a mistake that i removed
<leonardr> i added it a while back to get a test failure to show up
<leonardr> i thought i pushed that update
<leonardr> give me 1 minute, i'm testing a slight refactoring and i'll push that too
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: jelmer-oops || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> leonardr: Ah, yes, you did; I was only looking at r130.
<leonardr> allenap, slight refactoring has been pushed (r132)
<allenap> leonardr: In the restfulclient branch, for the original request to be a conditional request wouldn't httplib2 have to have the representation cached already, and so return the cached version?
<leonardr> allenap: yes, the problem is there's a bug in httplib2 where the cached version is cached incorrectly
<leonardr> it returns the cached version and lazr.restfulclient chokes on it
<allenap> leonardr: Is that different to #97, or am I misremembering it?
<leonardr> allenap, that is #97
<allenap> leonardr: Okay.
<adiroiban> rockstar, intellectronica do you have time for an UI review https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285 ?
<rockstar> adiroiban, sure
<adiroiban> rockstar: I have added links to some screenshots in the last comment
<rockstar> adiroiban, okay.
<bac> hi jelmer
<jelmer> bac: hi
<bac> jelmer i'm looking at your oops branch.  it looks good but i have a question
<bac> jelmer:  at line 70 of the diff you remove a log message and just let the exception bubble up.
<bac> jelmer:  why did you determine that error message was of no use?
<jelmer> bac: the error message is still of use, but it's logged in the OOPS now
<jelmer> as well as by the caller of that method
<jelmer> previously the function itself would log the error and then re-raise the exception to its caller, which would ignore it and not mark the record it was processing as published
<jelmer> Sorry, my MP description was a bit terse
<bac> jelmer: i'm confused b/c one change is to ArchivePublisherBase and the other to FilePublishingBase and they seem to not be related.  is that right?
<bac> so the caller to the method on FilePB is the one that is already handling the OOPS and has always done so?
<jelmer> bac: they are somewhat related. ArchivePublisherBase.publish calls publish() on the elements in its "files" member that are derived from FilePublishingBase
<jelmer> bac: yep
<bac> jelmer: please consider using abentley's very nice bzr plugin lp-send to creating MPs.  the skeleton makes for much better, more informative, easier to review merge proposals.
<bac> jelmer:  ok.  thanks for the info
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: lunch || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> jelmer, actually the lpreview_body plugin will work with "bzr send" and "bzr lp-propose" (recommended).
<jelmer> bac: Thanks!
<rockstar> adiroiban, you'll need to fix the jslint issues before you can land this.
<adiroiban> rockstar: ok. Do I a need a documentation review from mrevell, or the help page is ok?
<rockstar> adiroiban, you should have him look at it.
<jelmer> abentley: ah, ok
<rockstar> adiroiban, when one enters "translator mode," why is "Someone should review this translation" not checked?
<adiroiban> rockstar: do you mean to check "Someone should ..." for the current new translations ?
<adiroiban> I mean for the previouly entered translations?
<rockstar> adiroiban, if I understand this correctly, when a translator is also a reviewer but just trying to translate, they always check "Someone should review this translation"  Correct?
<rockstar> adiroiban, and so this branch makes it so that they don't have to check that when they are in translation mode.
<adiroiban> rockstar: that is correct
<rockstar> adiroiban, so, when someone enters "translator mode" there's a cookie set using javascript, correct?
<adiroiban> yes
<rockstar> adiroiban, so you should also use javascript to check the box next to "Someone should review this translation"
<adiroiban> hmm.. there is a javascript for that
<adiroiban> is it not working?
<adiroiban> I have tested it on Karmic using Chromium and Firefox
<adiroiban> and there is also a Windmill test for that
<rockstar> adiroiban, well, this is based on your screenshots.  I'm pulling your branch now.
<adiroiban> rockstar: ah. ok
<adiroiban> yes
<rockstar> adiroiban, I'm not really looking at your code specifically.  You'll need to get a code review for that.  All I'm looking at is the UI currently.
<adiroiban> rockstar: yes. Henning will do the code review
<rockstar> adiroiban, so, your current screenshots are wrong.  noodles will probably pull the branch as well when he mentors my review, but you might want to update those, since they indicate something incorrect.
<adiroiban> rockstar: why are they wrong ? :)
<rockstar> adiroiban, because when you're in "translator mode," then "Someone should review this translation" should be checked.
<adiroiban> rockstar: ah... not always
<adiroiban> you can still manually uncheck them
<adiroiban> is just when you type a new translation, the checkbox will be checked
<rockstar> adiroiban, when you make screenshots, they should indicate the default.
<adiroiban> rockstar: ok. I have updated the screenshot from the description
* 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
<gary_poster> abentley: can you give me a review of the testfix please? http://pastebin.ubuntu.com/413823/
<abentley> gary_poster, r=me.
<gary_poster> thanks abentley
<jml> hello
<jml> may I please get a review of some fairly simple (and hopefully non-behavioural) changes to the codehosting ssh server?
<jml> https://code.edge.launchpad.net/~jml/launchpad/extract-ssh-server/+merge/23350
<jml> also, https://code.edge.launchpad.net/~jml/launchpad/subunit-by-default/+merge/18449
<jml> (wtf, there have been ~4000 merge proposals since then?)
#launchpad-reviews 2010-04-14
<mwhudson> sigh
<mwhudson> anyone want to review a branch?
<mwhudson> https://code.edge.launchpad.net/~mwhudson/launchpad/puller-import-hack/+merge/23370
* henninge 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
<jml> hi
<jml> would someone please review https://code.edge.launchpad.net/~jml/launchpad/extract-ssh-server/+merge/23350
<jml> it's a small patch that shouldn't change any behaviour
* 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
<maxb> I would like to submit this rather boring and small branch for review: https://code.edge.launchpad.net/~maxb/launchpad-cscvs/use-hashlib
<danilos> allenap, hi, got a review ready for you :) https://code.edge.launchpad.net/~danilo/launchpad/bug-562450/+merge/23383 (diff is being regenerated)
* danilos changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> danilos: I will take a break now. I am not sure I will make the call on time, so don't wait for me. Sorry.
* wgrant changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [danilo,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* maxb changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [danilo,wgrant,maxb(cscvs)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> allenap: interested in a branch for review?
<intellectronica> oh wow, your queue is long
<intellectronica> maybe we can find another kind soul
<deryck> intellectronica, I have something for review, too.  I could swap you.
<intellectronica> deryck: excellent. just creating an mp. send me yours
<deryck> intellectronica, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/bug-count-filters-503222/+merge/23386
<deryck> intellectronica, it will need UI review, too.  If you want to do both.  But I'm making screenshots now for the UI.
<intellectronica> deryck: sure, i'll do the ui review (with sinzui mentoring)
<wgrant> Two of the branches are trivial, though...
<jml> I've just reviewed & approved maxb's branch
<maxb> and ec2ed? :-)
<jml> maxb: not yet. anyway, I think launchpad-cscvs is a pqm-tested branch
<maxb> oh, sorry
<maxb> pqm-submitted, then
 * maxb notes that the cscvs tests don't actually _pass_ anyway
<jml> really?
<intellectronica> deryck: https://code.edge.launchpad.net/~intellectronica/launchpad/bug-544794/+merge/23389
<maxb> I verified my changes didn't make them fail any worse
<jml> groan
<jml> it's not in 2a format
<deryck> intellectronica, got it.  thanks.
<intellectronica> deryck: in the template, why do you need to protect against inprogress_bug_count not existing?
<intellectronica> and does it behave correctly in case of 0?
<deryck> intellectronica, this was the form kfogel used because somewhere this template is used where those names don't exist, IIRC.  and yeah, it does work right for 0.
<deryck> intellectronica, so basically I'm following all the other filters there.
<intellectronica> deryck: cool. r=me for the code, then.
<deryck> kfogel heard us talking about him, it seems :-)
<deryck> intellectronica, thanks!
<kfogel> deryck: that was coincidence -- my client was off until a second ago.
<kfogel> deryck: what's up?
<intellectronica> deryck: invite me for a ui review when you're ready with that
<jml> ffs
<deryck> kfogel, nothing, just kidding.  intellectronica had asked about why I did something, and I said "copying kfogel." :-)
<intellectronica> kfogel: it's easy to be a victim in the blame game when your irc client is off ;)
<deryck> intellectronica, screenie is up.  I can't request you as a UI reviewer.  It changes the current review from code to ui if I do.
<intellectronica> deryck: i'm not sure about the wording of "N In-progress bugs"
<intellectronica> ot1h it fits the template of all other lines, otoh it reads a bit funny
<deryck> intellectronica, ok.  any suggestions?
<intellectronica> "N bugs in progress" is the obvious alternative, but I don't know if it's actually better
<intellectronica> deryck: let's leave it like that. changing the order will just make it harder to read.
<intellectronica> deryck: ui=me*
<intellectronica> sinzui: would you like to take a look? ^^^^^
<deryck> intellectronica, thanks.
<sinzui> intellectronica, I can in a short while
<deryck> intellectronica, concerning your MP.... does the change from security_related to setSecurityRelated have any impact on the js widget for security setting?
<intellectronica> deryck: no, that's handled by the mutator in the api
<intellectronica> it's only necessary to adjust the callsites internally
<deryck> intellectronica, so if someone changes the security from the web ui, this method will be called and heat will change?
<deryck> ah, I see now.  sorry for dumbness.
<intellectronica> deryck: yes, just like with bug.private, if you set bug.security_related from the api, bug.setSecurityRelated will be called
<intellectronica> that's what the @mutator_for decorator in the interface does
<deryck> intellectronica, I see now.  Sorry for being dumb.  I had never paid attention to or used mutator_for decorator myself.
<deryck> that's handy
<jml> maxb: done, I think.
* jml changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [danilo,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> would someone please review https://code.edge.launchpad.net/~jml/launchpad/extract-ssh-server/+merge/23350
<maxb> jml: Excellent, thanks
<intellectronica> deryck: yes, would be even nicer if we could use something like that internally. i tried using property(), but it became quite awkward quickly, so i figured adjusting the existing call sites is better
<jml> maxb: my pleasure
<deryck> intellectronica, yup.  looks good to me.  r=me.
<intellectronica> deryck: thanks!
* sinzui changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [danilo, wgrant, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<maxb> Could someone r or rs this as appropriate? It's purely a sourcedep revno bump. https://code.edge.launchpad.net/~maxb/launchpad/use-cscvs-r432/+merge/23395
* noodles775 changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [danilo, wgrant, sinzui, noodles] || 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: allenap || reviewing: - || queue: [danilo, wgrant, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<maxb> Thanks noodles775. Will you land it?
<noodles775> maxb: yep, it's off now :)
<maxb> woot, every day, python 2.6 gets a liiiiittle bit closer :-)
<noodles775> Great!
<stub> How close? It is applicable to https://bugs.edge.launchpad.net/launchpad-foundations/+bug/562828 (which is a trivial fix with Python 2.6)
<mup> Bug #562828: Librarian garbage collector does not cope with symlinks <Launchpad Foundations:New> <https://launchpad.net/bugs/562828>
<wgrant> Not pre-Lucid, since we don't have 2.6 on Hardy.
<wgrant> But it's not far.
<sinzui> deryck, did my suggestion terrify you?
<deryck> sinzui, haven't looked yet, sorry.  let me look....
<deryck> sinzui, I like your suggestion and am fine to do that.
<daniloff> allenap, hi, did you get any chance to restart reviewing?
<allenap> daniloff: Erm, not yet; very busy :) I'll get to it as soon as I can. Edwin normally starts sometime soon too.
<daniloff> allenap, cool, thanks; EdwinGrubbs, I'll be getting some food, but should be around later as well for some real OCRing :)
<EdwinGrubbs> daniloff: I'll be able to start on that in a few minutes.
<abentley> allenap, EdwinGrubbs could you please review https://code.edge.launchpad.net/~abentley/launchpad/recipe-security/+merge/23357 ?
* abentley changed the topic of #launchpad-reviews to:  On call: allenap || reviewing: - || queue: [danilo, wgrant, sinzui, abentley] || 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 || reviewing: adeuring || queue: [danilo, wgrant, sinzui, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> (He asked me a while back, sorry, forgot to update the topic.)
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: allenap, Edwin || reviewing: adeuring, abentley || queue: [danilo, wgrant, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: sure
<abentley> EdwinGrubbs, thanks.
<allenap> adeuring: Is there going to be another branch (or two) to add a date_expired field to IBugTask?
<adeuring> allenap: argh... i forgot completely about that...
<adeuring> yes, there _should_ be a branch...
<allenap> adeuring: I think the bug number in the branch name is wrong.
<adeuring> allenap: right...
<allenap> adeuring: Line 75, could you login(product_owner) to avoid the removeSecurityProxy() call?
<allenap> adeuring: Or even login_person(product_owner)
<adeuring> allenap: in theory, yes. Problem that things become then more complicated: the owner can't assign another person as the supervisor, so I need to create a team, let another person join that team and finally run the test with that other user
<adeuring> allenap: I was too lazy to do all that ;)
<allenap> adeuring: Argh. Leave it then :)
<adeuring> allenap: OK ;)
<allenap> adeuring: Perhaps the removeSecurityProxy() hack could be put in LaunchpadObjectFactory.makeProduct(..., bug_supervisor=None).
<adeuring> allenap: right, good idea!
* leonardr changed the topic of #launchpad-reviews to:  On call: allenap, Edwin || reviewing: adeuring, abentley || queue: [danilo, wgrant, sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> allenap, edwin: pushing https://code.edge.launchpad.net/~leonardr/lazr.restful/561521/+merge/23404 onto the queue
<allenap> adeuring: Line 124, the bug supervisor or the product owner should be able to call transitionToStatus(), so consider using that rather than removing the proxy.
<adeuring> allenap: right
<allenap> adeuring: 136s/Epired/Expired/
<adeuring> allenap: fixed
<allenap> adeuring: One last thing. There's probably some doc test out there that does a similar thing to TestBugTaskEditViewStatusField. If you know where, consider removing it.
<adeuring> allenap: I did not find anything like that...
<adeuring> that's why I added it ;)
<allenap> adeuring: Heh :)
* allenap changed the topic of #launchpad-reviews to:  On call: allenap, Edwin || reviewing: danilo, abentley || queue: [wgrant, sinzui, leonardr] || 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, Edwin || reviewing: wgrant, abentley || queue: [sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> wgrant: Want me to land lp:~wgrant/launchpad/always-use-sourcepackagename.name for you?
* allenap changed the topic of #launchpad-reviews to:  On call: allenap, Edwin || reviewing: sinzui, abentley || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> wgrant: I've sent it off to land via ec2.
<allenap> sinzui: In http://people.canonical.com/~curtis/project-without-package-suggestions.png it might work even better if no radio buttons (or the title) appeared, just the button.
<sinzui> allenap, there will also be an option to say it is not packaged.
<sinzui> allenap, The previous UI proposal did have button...
<allenap> sinzui: Okay. Also, can_show_portlet() can return True if there are sourcepackages even when user is None. Is that right?
<sinzui> Correct, that is what you see now...https://edge.launchpad.net/gedit
<sinzui> The feature we are refining suggests packages when we do not know about them
<sinzui> There can be a list like this: https://edge.launchpad.net/gdp
<sinzui> allenap, https://edge.launchpad.net/gedit-class-browser shows just the link to choose another package, but the correct answer is Not packaged.
<sinzui> allenap, The UI to support a list or packages, choose another package, or not packaged became very complicated and abentley, james_w, bac, and I agreed that we should try a single list and button
<allenap> sinzui: Okay, that makes sense, thanks.
 * sinzui has to rethink the model to support Not Packaged
<allenap> sinzui: Line 43 in the diff, is it safe to use 'OTHER_PACKAGE' because package.name can never be upper-case? If so, can you add a tiny comment to explain.
<sinzui> hmm
<sinzui> You are correct, but I think I got lucky in my name. I did not think about it
<james_w> name can never have an underscore
<sinzui> allenap, I was cargo culting a style I have reviewed in the past when Object() was used to create a unique and use all-caps to define the field name
<sinzui> james_w, you meana source package name cannot contain underscores?
<james_w> correct
<james_w> for Debian/Ubuntu at least
<james_w> "Package names (both source and binary, see Package, Section 5.6.7) must consist only of lower case letters (a-z), digits (0-9), plus (+) and minus (-) signs, and periods (.). They must be at least two characters long and must start with an alphanumeric character."
<sinzui> ah yes, I recall someone wanted to create a gentoo package with an '/' in it
<sinzui> That was a crucial moment where is it was clear that  launchpad cannot support other distros, so should stop pretending that it does
* allenap changed the topic of #launchpad-reviews to:  On call: allenap, Edwin || reviewing: -, abentley || queue: [leonardr] || 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: Edwin || reviewing: abentley || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs, rockstar: can either you you comment on the UI of my branch https://code.edge.launchpad.net/~sinzui/launchpad/project-packages-portlet-ui/+merge/23360
<rockstar> sinzui, looking
<rockstar> sinzui, that's pretty straightforward.  How many package suggestions can a project have?
<sinzui> That's a good question. that I do not know the answer to
<sinzui> I would say too much because I think I have seen 10 and I tuned out
<sinzui> rockstar, 20
<sinzui> rockstar, the number and test are easy to adjust
<rockstar> sinzui, I fear what it might look like with 10 different suggestions.
 * sinzui think 10 is the limit
<sinzui> rockstar, no need to speculate: https://edge.launchpad.net/launchpad
<sinzui> We will alway have to an option to choose another package, or say it is not packaged. I think 8 is the most we should suggest
<rockstar> sinzui, I agree.  Is there code to limit the suggestions currently?
<sinzui> yes, 20. The value and test are trivial fixes I can do now
 * sinzui has them open
<EdwinGrubbs> abentley: r=me
<rockstar> sinzui, great.  With those changes, ui=me*
<sinzui> rockstar, thanks. tests pass after the change
<rockstar> sinzui, noodles still has to mentor me.
 * rockstar dreams of a day where he can be trusted to do UI reviews all by himself
<rockstar> Also, Launchpad just shit the bed for me.
<deryck> sinzui, this kind of thing:  https://devpad.canonical.com/~deryck/filters-split-up.png   Or actual new portlets each in their own border outline?
<sinzui> deryck, that looks great
<deryck> sinzui, cool, thanks.
<sinzui> I think borders would make me think too much about the filters. your pictures is good to land
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs, thanks.
<abentley> EdwinGrubbs, could you please review https://code.edge.launchpad.net/~abentley/launchpad/revert-enums/+merge/23416 ?
* abentley changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: leonardr || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: I can do that after lunch
<abentley> EdwinGrubbs, thanks.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gary, maybe you'd like to take a quick look at https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/no-lazr-restful/+merge/23424 ? see if i did things correctly?
<gary_poster> leonardr: looking
<gary_poster> leonardr: what do you think of bin/test running both?
<leonardr> gary: bin/test picks up all the tests, not just the standalone ones
<gary_poster> leonardr: oh I see.  cool
<gary_poster> leonardr: niggle: "Throughout this file, we avoid importing anything from lazr.restful..." we avoid importing anything in the module level namespace...whatever, the current wording is close enough, I guess.
<leonardr> i can fix it up
<gary_poster> ok cool
<gary_poster> so leonardr, the only standalone test is authorizer ATM?  How much of a workout is that--does that make your feel reasonably comfortable that it is at least importing all pertinent modules?
<leonardr> gary: yes, definitely. it is creating a ServiceRoot object and parsing wadl
<gary_poster> ok cool
<gary_poster> leonardr: approved
<leonardr> great
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> EdwinGrubbs: still around?
 * james_w would love a review of https://code.edge.launchpad.net/~james-w/launchpad/pipe-lint/+merge/23437
<james_w> abentley: ^ might interest you
* james_w changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: abentley || queue: [james_w(lp:~james-w/launchpad/pipe-lint)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> james_w: I'll look at that.
<james_w> thanks
<james_w> tiny branch
<EdwinGrubbs> james_w: I thought pipelines were replacing looms.
<james_w> nope
<james_w> not at this point anyway
<EdwinGrubbs> james_w: s/pipline/pipeline/
<EdwinGrubbs> james_w: r=me
<james_w> thanks
* EdwinGrubbs 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-04-15
* maxb changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [maxb(trivial-bad-httpcaller-constructor-invocation)]|| This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* maxb changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [maxb(trivial-bad-httpcaller-constructor-invocation,py2.6-warnings-monkeypatching)]|| This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> james_w, cool.
<maxb> Hi, if I add an XXX comment, is it really mandatory that I file a bug, just to track the fact that the XXX comment exists?
<maxb> I have difficulty understanding the rationale
<abentley> maxb, the bug is tracking the fact that the underlying issue exists.
<maxb> even when the 'issue' is simply 'this code jumps through extra hoops to support both karmic and lucid' ?
<abentley> maxb, XXX comments don't have a lot of visibility, and tracking all our issues in one place has definite advantages.
<abentley> maxb, perhaps it would help if you regarded the XXX comment as optional.
<mwhudson> maxb: you could file a bug that says "there is code that jumps through extra hoops to support both karmic and lucid", and then when karmic is not supported, someone can see the bug and grep for all places that can now be simplified
<mwhudson> you certainly wouldn't need one bug per XXX
<abentley> maxb, but I don't think anyone has reviewed the XXX policy for a while.  Maybe you'd like to bring it up at the next reviewer meeting?
<maxb> I guess I can see some
<maxb> oops
<maxb> I guess I can see partially where your arguments are coming from. However I contrast that with things like bug 116048, which I am now fixing because I've come across the XXX in the code, and am closing the referenced bug as an afterthought
<mup> Bug #116048: Remove test_ftparchive.py hack when pqm/lpnet moves to feisty <tech-debt> <test-system> <Launchpad Foundations:In Progress by maxb> <https://launchpad.net/bugs/116048>
<mwhudson> maxb: come to think of it, i think the useful part of the policy is that the bug number lets me find _other_ places that can be fixed
<mwhudson> this definitely happened when bzr upgrades weren't done in lock step
<mwhudson> so you'd end up with code all over to work with 1.17 and 1.18 (say), then a while later you'd find one of the XXXs
<mwhudson> it was nice to be able to find all the others that could be cleaned up at the same time
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [maxb(trivial-bad-httpcaller-constructor-invocation,py2.6-warnings-monkeypatching),wgrant(emailauthentication.txt-2.6-fix)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* maxb changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [maxb(trivial-bad-httpcaller-constructor-invocation,py2.6-warnings-monkeypatching),wgrant(emailauthentication.txt-2.6-fix),maxb(tolerate-lucid-apt-ftparchive)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<maxb> poor OCR tomorrow morning
<maxb> still, most of those are fairly trivial
<abentley> mwhudson, you should also be able to use root_transport on line 35 of the preview diff.
<mwhudson> abentley: oh right, i got one of them but not the other
<abentley> mwhudson, r=me
<mwhudson> abentley: thanks
<wgrant> Hmm, I wonder if this will make the topic too long.
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [maxb(trivial-bad-httpcaller-constructor-invocation,py2.6-warnings-monkeypatching),wgrant(emailauthentication.txt-2.6-fix),maxb(tolerate-lucid-apt-ftparchive),do-not-shallow-copy-os.environ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [maxb(trivial-bad-httpcaller-constructor-invocation,py2.6-warnings-monkeypatching),wgrant(emailauthentication.txt-2.6-fix),maxb(tolerate-lucid-apt-ftparchive),wgrant(do-not-shallow-copy-os.environ)] || 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: noodles775 || reviewing: maxb || queue: [maxb(py2.6-warnings-monkeypatching),wgrant(emailauthentication.txt-2.6-fix),maxb(tolerate-lucid-apt-ftparchive),wgrant(do-not-shallow-copy-os.environ)] || 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: noodles775 || reviewing: maxb || queue: [wgrant(emailauthentication.txt-2.6-fix),maxb(tolerate-lucid-apt-ftparchive),wgrant(do-not-shallow-copy-os.environ)] || 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: noodles775 || reviewing: wgrant || queue: [maxb(tolerate-lucid-apt-ftparchive),wgrant(do-not-shallow-copy-os.environ)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> wgrant: you linked to the (partially) fixed issue in python on your MP, but is there another python issue for the remaining bug?
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: maxb || queue: [wgrant(do-not-shallow-copy-os.environ)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* danilos changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: maxb || queue: [wgrant(do-not-shallow-copy-os.environ), danilo] || 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: noodles775 || reviewing: wgrant || queue: [danilo,jml,sinzui] || 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: noodles775 || reviewing: danilos || queue: [jml,sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> danilos: which branch? (it's not listed on active reviews?)
<danilos> noodles775, oh, it seems I forgot to press the 'propose merge'
<danilos> noodles775, anyway, I am first interested in some discussion on it
<noodles775> Great.
<danilos> noodles775, https://code.edge.launchpad.net/~danilo/launchpad/bug-553093/+merge/23458
<danilos> noodles775, what I am struggling with is coming up with a decent test
<danilos> noodles775, basically, POST fails because we are snapshooting entire Language object, and translators property can have more than 1000 Person objects in there
<danilos> noodles775, I've crafted a start of a test (-r-2.., pasted in the MP), but because of all the interaction (I need to add karmacache entries, and then coming back from 'karma' DB user I can't get the views to be constructed properly because of some missing Zope init)
<danilos> noodles775, so, considering it took ~2 minutes on my computer to get all 1001 person objects with karma constructed, I'd rather not even have a test for this
<noodles775> !
<noodles775> (at the 2 minutes)
<noodles775> ok, I'll have a look and see if there's anything I could suggest (but guessing you've thought through most options).
<danilos> noodles775, thanks
<mrevell> wgrant, Thanks for mentioning Mercurial imports the other. I spoke to Jelmer and he'd rather we didn't announce them just yet.
<noodles775> danilos: sorry, just coming back to your MP. Would it make any sense to you to instead test the interface (ie. that the translators field provides IDoNotSnapshot)?
<noodles775> It's not a test reproducing the real issue, but it will ensure (and document) the issue?
<noodles775> s/ensure/ensure that the fix remains
* maxb changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: danilos || queue: [jml,sinzui,maxb(use-pygpgme-r49)] || 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: noodles775 || reviewing: jml || queue: [sinzui,maxb(use-pygpgme-r49)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> noodles775: I do not know.
<danilos> noodles775, sure, I can do that
<danilos> noodles775, sorry (had a call and all)
<noodles775> danilos: I added another idea on the MP too. See what you think.
<noodles775> wgrant: ok.
<wgrant> noodles775: I'll have a hunt now.
<noodles775> Thanks.
<danilos> noodles775, I'll see how complicated your second idea is and if I get lost in it, I'll just test the interface :)
<noodles775> danilos: great.
<jpds> jml: Hi there, I have a small db patch that needs reviewing; when you have time. :) https://code.launchpad.net/~jpds/launchpad/mirrorprober-next-probe-schema/+merge/23467
<jml> jpds, can you please get BjornT to review it?
<jpds> jml: Sure, I've reassigned the review request.
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: sinzui || queue: [maxb(use-pygpgme-r49)] || 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: noodles775 || reviewing: sinzui || queue: [] || 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: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> noodles775, need a tiny review of https://code.edge.launchpad.net/~leonardr/launchpad/test-new-lazr-restful/+merge/23470
<noodles775> leonardr: heh, we really need a rubber-stamp for some requests... unless there is something I'm missing that I can actually review?
 * noodles775 approves
<leonardr> no, it's a rubber-stamp
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> noodles775, btw, I've updated https://code.edge.launchpad.net/~danilo/launchpad/bug-553093/+merge/23458 (used IDoNotSnapshot); will you have a chance to take another look?
<noodles775> danilos: sure.
<danilos> thanks
<danilos> noodles775, I am out for some food, should be back shortly
<noodles775> danilos: I just approved, but enjoy lunch :)
* bigjools changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [wgrant, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> noodles775: are you on call? :)
<noodles775> bigjools: I was fo the first 4.5hrs, but due to ui reviews throughout the week, was going to finish to catch up on other work :)
<bigjools> allrighty then :)
<noodles775> bigjools: but given the trivial line counts of yours and wgrants....
<bigjools> yeah mine's a 1 line fix and 1 line test change :)
<noodles775> yep, it's approved.
<bigjools> fanks
<wgrant> noodles775: It passes if you run it a few times.
<wgrant> Sometimes only if it's on its own.
<wgrant> This is probably why they're not run by default.
<noodles775> wgrant: Yep, thanks. It's on ec2 to land.
<wgrant> Thanks.
* noodles775 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
<maxb> noodles775: Hi, did you send ~maxb/launchpad/use-pygpgme-r49 to ec2?
<noodles775> maxb: yep, it's still running the tests.
<intellectronica> sinzui: i've invited you to mentor a ui review i did for gmb. note that we've already agreed on some improvements which he's working on.
<sinzui> fab
<gmb> sinzui, intellectronica: Where would you rather have the button? With the activity log or next to the "next check" date?
<intellectronica> gmb: i thought the activity log
<gmb> intellectronica, I think so to - it's consistent with the code imports ui then - but I was just checking :)
<intellectronica> cool
* jml changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> https://code.edge.launchpad.net/~jml/launchpad/extract-ssh-server-auth/+merge/23482 fwiw
<intellectronica> jml: if it's something i can fit in 15m i'm happy to volunteer. is it?
<jml> intellectronica, it's almost entirely moving code around, so I think so.
<intellectronica> oright, on it
<jml> intellectronica, thank you
* intellectronica 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
<gmb> intellectronica, sinzui I'm tempted to say that the "Reschedule" button should actually be labelled "Update now" like it is on code. What do you think?
<intellectronica> gmb: yes, that's _much_ better
<gmb> Cool
<sinzui> gmb, yes, the former implies I can set a date
<gmb> sinzui, Exactly, that's what I was thinking.
<intellectronica> jml: there are lots of places where the moved code doesn't exactly conform to our coding conventions. is that something you want to deal with at all?
<intellectronica> it's fine by me if not
<intellectronica> but there are bonus points if you do
<jml> intellectronica, yes please
<intellectronica> jml: "yes, please, tell me about all these places so that i can correct them" or "yes, please, let's skip this for now, the code was already there, and it works"?
<jml> intellectronica, the former. sorry for the ambiguity.
<intellectronica> jml: avatarId --> avatar_id, for example
<intellectronica> i think there are similar cases
<intellectronica> userDict --> user_dict
<jml> intellectronica, ok. I'll clean those up.
<intellectronica> jml: is StringTransportWith_setTcpKeepAlive an idiom for eventhandlers or something?
<intellectronica> ok, maybe i exaggerated a bit when i said "lots". i meant "more than one".
<intellectronica> ah, nm, it makes sense in a test
<intellectronica> jml: everything else looks fine. r=me.
<jml> intellectronica, cool, thanks.
<jml> intellectronica, did you notice anything other than userDict and avatarId?
<intellectronica> jml: i didn't, actually.
<gmb> intellectronica, sinzui: How does this text look: https://devpad.canonical.com/~gbinns/reschedule.png ? I'm not sure whether it should be "at n of the last m attempts".
<gmb> Or something else
<intellectronica> gmb: this looks perfect to me.
<gmb> intellectronica, Cool.
<sinzui> gmb, Button label style says the words should be English title case [Update Now]
<gmb> sinzui, Right, will fix that.
<gmb> sinzui, intellectronica: Any other changes I need to make or is that good to go?
<sinzui> gmb, intellectronicaI am happy with the changes and the feature.
<gmb> Brill.
<gmb> intellectronica, ping
<intellectronica> gmb: yo
<gmb> intellectronica, Hi. Are you happy for my branch to land?
<intellectronica> gmb: very happy. let me mark the mp.
<gmb> intellectronica, Awesome, thanks.
<adiroiban> danilos: hi. Can you help me with the landing of this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-561355/+merge/23249 or should I ask someone else? It needs an ec2 test/land.
<danilos> adiroiban, sure, I can do it
<adiroiban> thanks
<danilos> adiroiban, thanks for working on a fix
<jml> hello
<jml> I have another branch to review
<jml> https://code.edge.launchpad.net/~jml/launchpad/codehosting-to-services/+merge/23491
<jml> it's perhaps even simpler than my last branch.
<danilos> jml, you are having too much fun coding, go strategize a bit ;)
<jml> danilos, once this branch is landed, I'm done
<jml> danilos, and yes, way too much fun.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar (although also packing for a trip, so latency expected) || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jml, all of the codehosting-to-services branch is moves right?  No real changes?
<jml> rockstar, pretty much. except for what I mention in my comment.
<rockstar> jml, okay, so basically just docstrings and test comments got changed (outside of imports)
<jml> rockstar, that sounds right.
<rockstar> jml, okay.  Looking now.
<rockstar> jml, your comment says you had to copy keys over.  Why couldn't you move them?
<jml> rockstar, they are also used by the codehosting acceptance tests.
<rockstar> jml, hm.  Do you really think it's wise to duplicate those keys?
<jml> rockstar, why would it be unwise?
<rockstar> jml, duplication.  It's not code, this I know.  But duplication in general can become unfun.
<jml> rockstar, you want me to generate a new RSA SSH key pair?
<jml> rockstar, it's pretty easy. there's even a cool website that'll do it for you!
<rockstar> jml, no, we can use the same one, but I'm wondering if we could symlink or something.
<rockstar> jml, that sounds like a very silly website.  :)
<jml> rockstar, it is â http://sshkeygen.com/
 * rockstar groans
<jml> rockstar, anyway, I can make them symlinks if you want. it doesn't matter much either way to me
<rockstar> jml, actually, it doesn't matter much to me either.  The question was more or less one of Kiko's "exploratory" questions.  If you're okay with it, I am.
<jml> rockstar, cool.
<rockstar> jml, r=me
<jml> rockstar, thank you
<rockstar> jml, no, thank you for cleaning up technical debt.
<daniloff> adiroiban, just to let you know, your branch won't land tonight because someone else has broken the build; I am letting the full test suite finish just so we see if there are any other problems; out now, cheers
<adiroiban> daniloff: that's ok
 * rockstar goes afk for a bit
#launchpad-reviews 2010-04-16
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/code-review-comment-tweak/+merge/23522
<thumper> mwhudson: trivial
<mwhudson> thumper: done
<thumper> mwhudson: ta
<danilos> adiroiban, I am resubmitting your branch for landing, hopefully it goes through this time
<adiroiban> danilos: hi. ok :)
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<StevenK> adeuring: Hi! Could you look at https://code.edge.launchpad.net/~stevenk/launchpad/fixes-bug-451396/+merge/21706 ?
<adeuring> StevenK: sure
<adeuring> streview sent
<adeuring> StevenK: review sent
<StevenK> adeuring: Thank you!
<gmb> adeuring, Can I get a code review for https://code.edge.launchpad.net/~gmb/launchpad/make-bugwatch-errors-less-obscure-bug-530113/+merge/23541 please?
<gmb> rockstar, Are you available to give me a trivial UI review for https://code.edge.launchpad.net/~gmb/launchpad/make-bugwatch-errors-less-obscure-bug-530113/+merge/23541?
<adeuring> gmb: sure
<gmb> adeuring, Thanks
<adeuring> gmb: rapproved
<gmb> adeuring, Danke.
<adeuring> gmb: welcome :)
<sinzui> gmb: the tooltip looks good
<gmb> sinzui, Thanks.
<sinzui> gmb: I think I stepped on intellectronica or rockstar by just approving it.
<intellectronica> and it hurts...
<gmb> sinzui, Yeah, intellectronica said he'd review it, sorry.
<intellectronica> but i would have given the exact same reply as you anyway, looks excellent :)
<gmb> intellectronica, Thanks.
<sinzui> intellectronica, this is your chance to prove I am incompetent. maybe the tooltip target is too small on your machine so you can never make it appear
<gmb> Gah! I submitted it against db-devel, not devel. Oh well, old-fashioned ec2 test it is then.
<gmb> intellectronica, sinzui: NO, IT'S FINE.
<intellectronica> ha ha
<sinzui> gmb: but lp does think you have a merge conflict
<gmb> sinzui, That's because it's submitted against the wrong branch.
<gmb> sinzui, The mp is for merging into db-devel, but I want it to land on devel.
<sinzui> I suspected that
<gmb> I'll check that it's not actually a problem, though.
<bigjools> adeuring: really trivial branch winging its way to you as soon as the email gets to LP :)
<adeuring> bigjools: ok, I#ll take a trivial look ;)
<bigjools> adeuring: danke :)
<adeuring> bigjools: r=me
<bigjools> adeuring: cheers
<EdwinGrubbs> adeuring: hi, can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-554153-claim-profile-bug/+merge/23550
<adeuring> EdwinGrubbs: sure
<noodles775> Hi adeuring, will you have time for another one? https://code.edge.launchpad.net/~michael.nelson/launchpad/delegate-to-buildfarmjob/+merge/23553
* noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: I am a bit tired, but I'll try my best ;)
<noodles775> adeuring: no problem if you don't get to it... it'll get picked up on Monday. Thanks either way.
<adeuring> EdwinGrubbs: w=me
<EdwinGrubbs> thanks
* adeuring 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
