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