[02:53] <mwhudson> thumper: want to review this code import branch?
[02:53] <thumper> mwhudson: ok
[02:54] <mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/code-import-optimizations/+merge/23206
[02:54] <mwhudson> thumper: the stacking idea didn't work
[02:55] <thumper> ok
[05:49] <thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/fix-schema/+merge/23209
[05:49] <thumper> I have another too that I've been meaning to fix for ages:
[05:49] <thumper> https://code.edge.launchpad.net/~thumper/launchpad/scanner-awesomeness/+merge/23211
[06:02] <mwhudson> thumper: "ackward" is a typo
[06:02] <mwhudson> (second one)
[06:02] <thumper> :)
[06:03] <mwhudson> thumper: also assertFoundRevisionNumber and test_not_found are separated by two lines, should be one
[06:04] <mwhudson> + self.assertIs(None, iterator.iterator)
[06:04] <mwhudson> seems to be testing an implementation detail really
[06:04] <thumper> it is
[06:04] <thumper> but I used to to confirm that it was broken
[06:04] <thumper> not working as intended
[06:04] <thumper> working, but weirdly
[06:05] <thumper> I can remove it
[06:05] <mwhudson> thumper: i think it would be slightly better
[06:05] <mwhudson> (the test principle is "go in by the front door" or something)
[06:06] <thumper> better to remove it?
[06:10] <mwhudson> please
[06:10] <mwhudson> thumper: some other small comments on the merge proposal
[06:10] <thumper> mwhudson: thanks
[06:18] <thumper> W
[06:18] <thumper> T
[06:18] <thumper> F
[06:18] <thumper> :(
[06:20] <thumper> ✁☹
[06:32] <thumper> mwhudson: parallel iteration fails
[06:32] <thumper> mwhudson: should I just document and ignore?
[06:36] <thumper> mwhudson: or I could make it more like the recipe at http://code.activestate.com/recipes/576941-caching-iterable-wrapper/
[06:37] <mwhudson> thumper: hmm
[06:37] <mwhudson> thumper: does it fail noisily?
[06:37] <thumper> mwhudson: no just very incorrectly
[06:37] <mwhudson> thumper: :(
[06:38] <mwhudson> oh i guess self.data is shared between the iterators
[06:38] <thumper> no I understand the activestate recipe more
[06:38] <thumper> it is licenced under MIT
[06:38] <thumper> does it really matter?
[06:39] <mwhudson> i'm a bit torn
[06:39] <mwhudson> because clearly for your usecase it doesn't matter, no
[06:39] <mwhudson> but leaving bombs in the source like this to unexpectedly blow up at people doesn't sound good
[06:40] <thumper> make it similar?
[06:40] <mwhudson> thumper: i think that activestate recipe is still a bit more complicated than you need
[06:40] <mwhudson> (e.g. pickling support)
[06:40] <thumper> yeah, I didn't care about that
[06:40] <thumper> nor the __new__ optimisation
[06:41] <thumper> but the indexing is useful
[06:41] <mwhudson> well, the indexing stuff is what makes it work in parallel isn't it?
[06:42] <thumper> uh ha
[06:42] <mwhudson> um, maybe not
[06:45] <thumper> actually we can't make it threadsafe
[06:45] <thumper> but that shouldn't matter
[06:45] <mwhudson> that's ok i think
[06:46] <mwhudson> because if you're using threads, you should expect your program to be confusing anyway
[06:46]  * mwhudson runs away
[06:46] <thumper> heh
[06:46] <thumper> ok, I have parallel bits working
[06:48] <thumper> mwhudson: http://pastebin.ubuntu.com/412936/
[06:49] <mwhudson> thumper: "# Defer to the generator" <- i think you mean iterable?
[06:50] <thumper> mwhudson: yeah
[06:50] <mwhudson> thumper: otherwise looks fine
[07:42] <jtv> Anybody willing to review a slightly oversized buildfarm branch?
[07:42] <jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-544237/+merge/23214
[07:43] <jtv> morning henninge!
[07:49] <henninge> HI jtv! ;)
[07:51] <jtv> henninge: looking at your review changes now.  Had to get an oversized branch finished first.
[07:51] <henninge> jtv: another one? ;-)
[07:51] <jtv> Yes, slave build cookies.
[07:53] <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."
[07:54] <jtv> (Makes the scoping clearer)
[07:56] <jtv> henninge: I see you cut down this assertion:
[07:56] <jtv> 53        # Productseries and distroseries will be asserted in getSubset but
[07:56] <jtv> 54        # not sourcepackagename.
[07:56] <jtv> 55        assert (distroseries is None and sourcepackagename is None or
[07:56] <jtv> 56                distroseries is not None and sourcepackagename is not None), (
[07:56] <jtv> 57                "Please specify distroseries and sourcepackagename together.")
[07:57] <henninge> yes ... ?
[07:57] <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)"?
[07:57] <jtv> BTW lots of good changes here, thanks.
[07:58] <henninge> jtv: I am not sure that "Noneness" concept is clearer to read.
[07:58] <jtv> You could even just say "distroseries is None or sourcepackagename is not None"
[07:58] <henninge> that I like ;)
[07:59] <jtv> It's not quite as tight, but I don't think the case that it ignores matters much.
[07:59] <henninge> hm, which one is that?
[08:00] <jtv> Where the caller specifies a sourcepackagename but not a distroseries.
[08:00] <jtv> (Sounds bad, but combined with the checks in the utility...  anyway, these are assertions, not supposed to trigger at all)
[08:01] <henninge> well, in that case the assertion in SubSet will trigger.
[08:02] <jtv> Right, combined with the checks in the utility it doesn't matter.
[08:02] <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."
[08:29] <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?
[09:56] <jtv> henninge_: did you see my notes about "i.e." vs. "e.g." and about len_potemplateset?
[09:57] <henninge> jtv: yes, all incorporated
[09:57] <jtv> henninge: great, thanks
[09:57] <henninge> Notes Inc.
[09:57] <henninge> ;-)
[09:59] <jtv> heh
[10:00] <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.
[10:05] <jtv> wgrant: the big problem now is to get a reviewer for the cookie branch.
[10:06] <noodles775> intellectronica: are you around for reviews today?
[10:06] <wgrant> jtv:  :(
[10:06] <wgrant> It's only a little oversized.
[10:06] <wgrant> And then only be s/ID/Cookie/..
[10:06] <wgrant> s/be/by/
[10:07] <jtv> wgrant: yes, but the people I've approached so far wouldn't have had time for 700 lines either.
[10:07] <noodles775> jtv: put it in the queue now :)
[10:07] <intellectronica> noodles775: sure
[10:08] <jtv> intellectronica: got one for ya!
[10:08] <intellectronica> noodles775: which branch?
[10:08] <jtv> It's slightly oversized though: https://code.edge.launchpad.net/~jtv/launchpad/bug-544237/+merge/23214
[10:08] <noodles775> intellectronica: I was actually trying to get jtv to ask you to review his branch :)
[10:08] <jtv> (Then again, 888 is a very lucky number according to the Chinese)
[10:08] <noodles775> intellectronica: thanks.
[10:08] <jtv> for which, noodles775, thank you.  :)
[10:08] <intellectronica> jtv, noodles775: np
[10:08] <jtv> \o/
[10:08] <jtv> thanks
[10:53] <intellectronica> jtv: r=me
[10:54] <jtv> intellectronica: wonderful, thanks!
[11:56] <jtv> henninge: done with your branch.  Phew!
[11:56] <henninge> jtv: great, thanks!
[13:15] <adiroiban> intellectronica: hi. can you please send this branch to ec2-test https://code.edge.launchpad.net/~adiroiban/launchpad/bug-548999/+merge/22271? thanks!
[13:15] <intellectronica> adiroiban: sure
[14:22] <jelmer> t/opic
[14:22] <jelmer> argh
[15:23] <deryck> intellectronica, abentley -- I have a branch for review when one of you has availability.
[15:24] <intellectronica> deryck: i can review
[15:24] <deryck> intellectronica, great, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/enable-test-inline-subscriber/+merge/23230
[15:27] <intellectronica> deryck: r=me
[15:27] <deryck> intellectronica, great, thanks!
[16:15] <leonardr> intellectronica or abentley, please review https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/user-agent/+merge/23237
[16:15] <intellectronica> leonardr: on the phone. will have a look later if abentley doesn't pick it up
[16:15] <leonardr> all right
[16:22] <abentley> leonardr, looking.
[16:28] <abentley> leonardr, have you considered using standard http header parameters instead of your own syntax?
[16:28] <leonardr> abentley: you mean like matrix parameters? or is there something specific for headers?
[16:29] <leonardr> oh, i bet you're talking about the parameters that go eg. with an uploaded file
[16:30] <leonardr> afaik that's only used in Content-Disposition, but i could be wrong and that might be more readable
[16:31] <abentley> I mean like encoding in "Content-type: text/plain; encoding=utf-8"
[16:31] <leonardr> ok, yes, it's in a number of places
[16:32] <abentley> leonardr, (actually, a correct example is "Content-Type: text/html; charset=ISO-8859-4")
[16:32] <abentley> leonardr, yes it's used in a number of places, and IIRC, Python supports querying it.
[16:33] <leonardr> abentley: how about this:
[16:33] <leonardr> User-Agent: launchpadlib 1.6.1, lazr.restfulclient 1.0.0; oauth_consumer=apport; +cache-control
[16:33] <leonardr> or cache-control=true
[16:34] <abentley> leonardr, that looks good.  I think I prefer cache-control=true.
[16:34] <leonardr> ok
[16:34] <leonardr> might get lengthy
[16:35] <abentley> leonardr, do you actually need cache-control, or could you infer it from the lazr.resfulclient version?
[16:36] <leonardr> abentley: i would rather not infer it for two reasons
[16:37] <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
[16:37] <leonardr> 2. other people are writing clients (eg. in java) and i want them to be able to use these features
[16:38] <leonardr> i don't want a situation where they pretend to be lazr.restfulclient, the way all browsers pretend to be "Mozilla"
[16:38] <abentley> leonardr, it sounded like you were working around a bug in earlier versions of lazr.restfulclient.  Am I wrong?
[16:42] <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
[16:42] <leonardr> but the bug is only triggered if the server sends certain information
[16:42] <leonardr> so the server needs to distinguish clients that it can send the information to, from clients that will choke on it
[16:44] <abentley> leonardr, so it seems unlikely that an arbitrary client (e.g. in Java) would have the same bug.
[16:45] <abentley> leonardr, but you're suggesting that all implementations which don't have that bug should assert that they don't.
[16:45] <leonardr> abentley: here's the problem
[16:45] <leonardr> old versions of lazr.restfulclient, already installed on thousands of computers, will not send any special user-agent
[16:46] <leonardr> the server's response to 'no special user-agent' needs to be 'don't send the data'
[16:46] <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
[16:47] <leonardr> i could send the special data to any user-agent that's not 'python/httplib2'
[16:47] <leonardr> but even then, someone else writing a non-lazr.restfulclient httplib2 user agent would like to be able to get the special data
[16:48] <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
[16:49] <leonardr> but if something similar happens in the future i will insist on something like cache-control=true
[16:50] <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?
[16:51] <leonardr> abentley: what specifically seems unfair?
[16:51] <leonardr> don't send the data unless a client specifically requests it?
[16:51] <leonardr> that's why i proposed some alternate solns
[16:53] <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.
[16:53] <abentley> until the existing implementations update their version strings to say that their implementation isn't broken.  Which might be incorrect.
[16:54] <leonardr> i don't want to get into the business of parsing version numbers in referer headers, especially of software i don't control
[16:55] <abentley> What do you mean by referrer headers?  I thought we were talking about user-agents?
[16:55] <leonardr> sorry, user-agents
[16:55] <leonardr> i was thinking of another time a lot of clients broke because of a change involving Referer
[16:56] <leonardr> If version 1.5, and i have no idea when it's going to be fixed, how do i do the restriction?
[16:56] <leonardr> (if version 1.5 is broken)
[16:56] <abentley> leonardr, so the alternative is disabling features that were previously enabled, until clients are updated to say they support them, right?
[16:57] <leonardr> i don't think a feature that was previously enabled would ever become disabled
[16:57] <leonardr> cache-control is a new feature that exposed an old bug
[16:58] <abentley> leonardr, I see.  I didn't get that before.
[16:58] <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
[17:00] <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
[17:00] <abentley> leonardr, Ah, okay.
[17:00] <leonardr> and we can take up a more general solution the next time this happens
[17:01] <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
[17:01] <leonardr> once the bug is fixed i can go back and change that, but that's a pain
[17:02] <abentley> leonardr, I think it's the best course though.
[17:03] <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.
[17:05] <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
[17:06] <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
[17:06] <leonardr> ok
[17:09] <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?
[17:18] <james_w> leonardr: is this about caching the wadl?
[17:18] <leonardr> james_w: yes
[17:18] <leonardr> abentley, sure
[17:20] <james_w> leonardr: may I have a look at the merge proposal?
[17:20] <leonardr> james_w: sure. https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/user-agent/+merge/23237
[17:20] <james_w> please subscribe me to the source branch, that page is private
[17:20] <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
[17:21] <leonardr> well, it shouldn't be
[17:21] <leonardr> ok, it's public now
[17:23] <james_w> I'm not sure this is worth the effort of a workaround is it?
[17:24] <james_w> I'm not sure what the "special data" is though
[17:24] <james_w> I thought it was fixed by removing Cookie from Vary in webservice requests?
[17:25] <leonardr> james_w: if there's no workaround, then all existing installations of launchpadlib break when i upgrade lazr.restful
[17:25] <leonardr> this is separate from Vary. look at the httplib2 bug
[17:25] <james_w> oh, none of this is in the bug report it seems
[17:26] <leonardr> james_w: i don't follow. which bug report are you refering to?
[17:27] <james_w> the lazr.restfulclient one
[17:28] <leonardr> james_w: if it will help i'll file a separate lazr.restfulclient bug saying 'work around httplib2 bug 97'
[17:28] <mup> Bug #97: can't mark a bug assigned <Launchpad Bugs:Fix Released by daf> <https://launchpad.net/bugs/97>
[17:29] <james_w> leonardr: no, it's ok, I was just working from a stale cache (fnar)
[17:29] <leonardr> ok
[17:29] <james_w> so you want to introduce cache-control in lazr.restful?
[17:30] <leonardr> james_w: yes. and httplib2 gives inconsistent behavior when cache-control is combined with conditional requests
[17:30] <leonardr> inconsistent behavior that crashes lazr.restfulclient
[17:30] <james_w> right
[17:30] <james_w> I can see why you need the workaround now
[17:31] <james_w> sorry for the noise
[17:32] <leonardr> sure
[17:56] <sinzui> abentley, can you review https://code.edge.launchpad.net/~sinzui/launchpad/series-branch-target/+merge/23056
[17:57] <sinzui> abentley, This branch is a complete reimplementation of a solution that had a fatal flaw
[18:07] <leonardr> abentley, ready for you to take another look
[18:25] <abentley> leonardr, r=me
[18:35] <adiroiban> intellectronica: any new about my branch sent to ec2 ?
[18:37] <intellectronica> adiroiban: it dies mysteriously and i had to resend it, so it will be another 2-3 hours until it finishes
[18:38] <adiroiban> intellectronica: ok. then most probably there are some errors
[18:38] <adiroiban> and to see them you will need to keep the session open
[18:38] <adiroiban> as no email will be dispached
[18:39] <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
[18:40] <adiroiban> thanks!
[18:51] <sinzui> thanks abentley
[18:51] <abentley> sinzui, np.
[20:02] <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.
[20:03] <mars> you know, I would really like to use Bash colour escape codes in there...
[20:03] <abentley> mars, did you consider just doing make schema?
[20:04] <abentley> mars, rather than erroring, I mean.
[20:04] <mars> abentley, I thought about that, but the problem is that you don't always want 'make schema' stomping on the database.  The scenario:
[20:05] <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
[20:05] <mars> if "make schema" ran automatically on the mismatch, it would stomp my original dev instance
[20:06] <abentley> mars, so what do you do instead of running make schema to do the bugfix?
[20:07] <mars> abentley, find the original instance, make sure I can shut it down, do so, then spin up the bugfix instance and db.
[20:07] <mars> abentley, ugly, but this is a quick-fix that will save a few annoying minutes of my life.
[20:08] <abentley> mars, does the check itself take very long?  make run seems to be taking longer and longer...
[20:08] <mars> maybe auto-running 'make schema' would work, too, but I did not want to spend too much time on it.
[20:08] <mars> abentley, the check takes almost no time, especially compared to other stuff.  'make jsFOO' takes much longer.
[20:08] <mars> guess what is next on the hit-list
[20:09] <mars> :)
[20:09] <abentley> mars, cool.
[20:09] <mars> small improvements!  One step at a time.
[20:10] <abentley> mars, does the exit status propagate to the make exit status?  If so, it would be nice to use a unique exit status.
[20:10] <mars> abentley, I don't know.
[20:11] <abentley> mars, is it deliberate that there's no newline at the end of utilities/check-db-revision.py ?
[20:13] <mars> abentley, nope.  My editor must not add them
[20:13] <abentley> mars, weird.  Okay, r=me.
[20:14] <mars> abentley, great, thank you!
[20:25] <bac> hi abentley, can i get a review?  https://code.edge.launchpad.net/~bac/launchpad/bug-559200/+merge/23256
[20:26] <abentley> bac, looking
[20:32] <abentley> bac, these doctests look a lot like unit tests.
[20:33] <abentley> bac, why is the new transaction.commit needed?
[20:33] <abentley> (patch line 128)
[20:34] <bac> abentley: without the commit the branch conflict does not occur
[20:35] <abentley> bac, r=me
[20:35] <bac> abentley: thanks
[20:36] <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...
[20:37] <abentley> bac, I thought it had been agreed that the doctests should be documentation that had tests.
[20:41] <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
[20:45] <abentley> leonardr, are If-Modified-Since and If-None-Match the only ways of getting a 304?
[20:48] <abentley> leonardr, it's generally not a good idea to sleep in tests.  Can you modify the apparent time seen by CookbookWebServiceClient instead?
[20:49] <leonardr> abentley: yes, except for a case with intermediate caches which doesn't apply here
[20:49] <leonardr> do you have any suggestions on how to modify the apparent time?
[20:49] <leonardr> i don't have direct access to the code that checks the time, it's in httplib2
[20:53] <abentley> leonardr, then I guess it can't be helped.
[20:54] <abentley> leonardr, what's the motivation for checking whether the client performed a conditional request?
[20:55] <leonardr> abentley, look at the end of the diff
[20:55] <leonardr> where it used to make a conditional request and expect an exception
[20:55] <leonardr> now it gets a special value and i don't use an exception for a non-exceptional case
[20:58] <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?
[20:59] <leonardr> abentley: no, they don't. that's why if it were to happen it woudl be an exceptional situation
[20:59] <leonardr> basically, if for some reason it does happen
[21:00] <leonardr> i don't want lazr.restfulclient to say 'oh, the object's new representation is NOT_MODIFIED'
[21:00] <leonardr> i want it to crash
[21:00] <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.
[21:00] <bac> oops, i missed some important punctuation in that first run-on sentence.
[21:01]  * bac tries again
[21:01] <bac> abentley: it depends on what you call a doctest.   in registry/docs we have doctests that are as you'd expect.
[21:02] <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.
[21:03] <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)
[21:03] <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_
[21:09] <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?
[21:11] <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.
[21:12] <bac> abentley: unless or until we make a launchpad-wide decision mandating one way or the other.
[21:15] <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.
[21:16] <abentley> bac, I'll add it to the reviewer meeting agenda.
[21:16] <bac> abentley: ok
[21:36] <EdwinGrubbs2> abentley: can you review a fairly simple branch for me?
[21:36] <abentley> EdwinGrubbs, sure.
[21:36] <EdwinGrubbs> abentley: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-555734-coc-oops/+merge/23268
[21:40] <abentley> EdwinGrubbs, r=me.
[21:40] <EdwinGrubbs> thanks
[22:35]  * 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