| bac | nhandler: perhaps. let me look. | 00:05 |
|---|---|---|
| bac | nhandler: i'll try, if i can come up with the proper way to invoke ec2 | 00:08 |
| mwhudson | bac: ec2 land should figure that out for you :-) | 00:12 |
| bac | nhandler: looks like i've figured it out. it's off to ec2 and will land if the tests pass | 00:12 |
| bac | mwhudson: yep, i just discovered. damn, it's nice. | 00:12 |
| mwhudson | yes | 00:13 |
| bac | mwhudson: we should keep jml. | 00:13 |
| mwhudson | heh | 00:14 |
| mwhudson | i think we did | 00:14 |
| nhandler | Thanks a lot bac | 00:20 |
| mwhudson | thumper: want to review a branch that upgrades us to Twisted 9.0.0 (from an egg, no less) | 03:05 |
| mwhudson | ? | 03:05 |
| thumper | aye | 03:05 |
| mwhudson | (hardly any changes, in fact) | 03:05 |
| mwhudson | thumper: should have mail and a diff now | 03:11 |
| mwhudson | thumper: thanks | 03:26 |
| === 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 | ||
| jml | bac, keep me where? | 10:24 |
| gmb | allenap: Morning. Could you review https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-487015/+merge/15238 for me please? | 10:42 |
| allenap | gmb: Sure :) | 10:42 |
| gmb | Ta | 10:42 |
| === allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb || queue [abel] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === salgado-afk is now known as salgado | ||
| === matsubara-afk is now known as matsubara | ||
| bac | jml: the context was a discussion of how cool 'ec2 land' was. i meant "keep you around" doing cool stuff. all good. | 11:52 |
| jml | :D | 11:52 |
| === jtv changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: gmb, - || queue [abel] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | gmb: needs-information on your branch. I have to go out now, but we can talk about it later. | 12:09 |
| === allenap changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: lunch, - || queue [abel] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | allenap: Okay, thanks. | 12:11 |
| gmb | allenap: Ah, hell, I've completely misunderstood the use of subscription_class. | 12:12 |
| gmb | Cocking cock cockity cock. | 12:12 |
| * gmb goes for lunch, will rethink this over a chicken salad. | 12:14 | |
| * jtv suddenly understands why gmb was going on about cocks | 12:23 | |
| * jtv chokes back a bad ripoff of the "poultry in motion" pun | 12:24 | |
| jtv | adeuring: the topic says you have a review in the queue, but I don't see it | 12:26 |
| adeuring | jtv: that's a private branch, ca. 400 lines diff. Want to review it? | 12:27 |
| jtv | adeuring: sure | 12:27 |
| adeuring | jtv: thanks! | 12:28 |
| === jtv changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: lunch, abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === mrevell is now known as mrevell-lunch | ||
| === salgado_ is now known as salgado | ||
| gmb | allenap: Reply sent. | 13:23 |
| === jtv changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: lunch, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | gmb: how was your cock salad? | 13:23 |
| gmb | jtv: Bit salty. | 13:23 |
| gmb | Ah... | 13:23 |
| gmb | We're getting into mneptok territory here. | 13:24 |
| jtv | well, you started it. | 13:24 |
| jtv | not that it's a bad thing... | 13:24 |
| jtv | (19:10:12) gmb: Cocking cock cockity cock. | 13:24 |
| jtv | (19:12:27) ***gmb goes for lunch, will rethink this over a chicken salad. | 13:24 |
| gmb | Quite so. | 13:24 |
| === mrevell-lunch is now known as mrevell | ||
| === danilos-out is now known as danilos | ||
| === gary_poster is now known as gary-call | ||
| === gary-call is now known as gary_poster | ||
| === allenap changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === jtv 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 | ||
| allenap | gmb: Can you push your subscribers-timeout branch? | 15:12 |
| gmb | allenap: FAIL. | 15:12 |
| allenap | :) | 15:13 |
| gmb | allenap: Pushed. | 15:13 |
| allenap | gmb: Fanks. | 15:13 |
| === salgado is now known as salgado-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| abentley | gary_poster: are you free to look at https://code.edge.launchpad.net/~abentley/launchpad/errorlog-context/+merge/15217 today? | 16:02 |
| gary_poster | abentley: yes | 16:02 |
| gary_poster | It's on my list | 16:03 |
| abentley | gary_poster: Cool, thanks. | 16:03 |
| gary_poster | np | 16:03 |
| gary_poster | will try to get it sooner rather than later | 16:03 |
| gary_poster | already started, but other bits intervened | 16:03 |
| gmb | allenap: Ta for the review. | 16:05 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | gmb: You're welcome dude :) | 16:06 |
| === salgado-lunch is now known as salgado | ||
| allenap | EdwinGrubbs: https://code.edge.launchpad.net/~edwin-grubbs/lazr-js/activator-ie-fixes/+merge/14969 has been approved by mars, but there's an outstanding review request for ~launchpad. Is that bogus? | 16:23 |
| EdwinGrubbs | allenap: yes, that's bogus. | 16:24 |
| allenap | EdwinGrubbs: I've marked the proposal as approved. | 16:26 |
| === matsubara-lunch is now known as matsubara | ||
| abentley | bigjools: I'm getting test failures in buildd-slavescanner.txt, with a pristine stable and up-to-date sourcecode. Do you know why? http://pastebin.ubuntu.com/327796/ | 16:34 |
| bigjools | abentley: it looks like it merged a conflict wrong | 16:34 |
| bigjools | the test is right, the output is wrong | 16:35 |
| bigjools | al-maisan: can you take a look at that please? --^ | 16:36 |
| abentley | bigjools, al-maisan: I don't understand how failing tests got into stable. I thought maybe it was a difference between karmic and hardy. | 16:36 |
| bigjools | yeah it's odd | 16:36 |
| * al-maisan looks | 16:36 | |
| al-maisan | this is very odd indeed | 16:37 |
| al-maisan | abentley: is this a devel or db-devel branch? | 16:38 |
| al-maisan | I take it it's devel | 16:38 |
| abentley | al-maisan: No, it's stable. | 16:38 |
| al-maisan | ah, OK | 16:39 |
| === beuno is now known as beuno-lunch | ||
| al-maisan | abentley: what's the best way to fix stable? Fix it in devel? | 16:47 |
| * al-maisan checks the devel branch | 16:47 | |
| abentley | al-maisan: Yes. Stable is just the last revision of devel that has passed the tests. | 16:47 |
| al-maisan | aye | 16:48 |
| al-maisan | abentley: so, devel does not have the issue you observed. | 16:59 |
| al-maisan | abentley: I guess the sample data in stable got skewed somehow -- that's why the error occurs. | 16:59 |
| * al-maisan looks at the stable branch now | 17:00 | |
| === jamalta is now known as jamalta|bot | ||
| === jamalta|bot is now known as jamalta | ||
| abentley | al-maisan: devel and stable seem to have the same revno right now, so if stable is affected, devel must be affected. | 17:03 |
| === beuno-lunch is now known as beuno | ||
| al-maisan | abentley: but that test passes on my system | 17:05 |
| abentley | al-maisan: I have now reproduced the issue on devel. | 17:05 |
| al-maisan | abentley: aha, how did you go about it? | 17:05 |
| al-maisan | abentley: did you do a "make schema" before running the test in question? | 17:06 |
| abentley | al-maisan: No, I didn't do make schema. | 17:06 |
| al-maisan | but that's important here | 17:06 |
| al-maisan | because the flaw is mostly buried in the sample data | 17:06 |
| al-maisan | s/mostly/most likely/ | 17:06 |
| abentley | al-maisan: I see, but I don't normally run make schema if I can avoid it. | 17:07 |
| al-maisan | well, you can't avoid it now :P | 17:07 |
| al-maisan | abentley: so, "make schema; bin/test -vv -t buildd-slavescanner.txt" should pass in devel, it does for me. | 17:09 |
| abentley | al-maisan: Yes, I'm running make schema now. I'll let you know how it turns out. | 17:10 |
| al-maisan | abentley: "make schema; bin/test -vv -t buildd-slavescanner.txt" passes in stable as well! | 17:11 |
| abentley | al-maisan: confirmed: running make schema fixed it. Thanks for your help. | 17:11 |
| al-maisan | abentley: you are welcome | 17:12 |
| abentley | al-maisan: Would it have been possible to land the sampledata update in db-devel rather than devel? That would have avoided the issue for me. | 17:13 |
| al-maisan | abentley: iirc wgrant introduced the sample data "clean-up" in a devel branch and unrelated to db schema changes | 17:14 |
| al-maisan | we mandate db-devel for schema changes | 17:15 |
| al-maisan | but not for sample data fixes afaik | 17:15 |
| al-maisan | anyway, I am out of here :) | 17:16 |
| al-maisan | Good night! | 17:16 |
| === henninge_ is now known as henninge | ||
| sinzui | EdwinGrubbs: ping | 18:01 |
| gary_poster | abentley: hi. I have several questions. I should start by saying that I like the general functionality, your clean ups, and the introduction of "with" to Launchpad :-) . I do have a few design questions that may simply indicate a request to add some docs/comments, or may suggest some changes. | 18:02 |
| gary_poster | Before I get into that, let me check my understanding. Am I right that this munges request variable information with information that we think might be helpful into the same OOPS bucket? | 18:02 |
| abentley | gary_poster: Yes. | 18:03 |
| gary_poster | abentley: ok, cool. glad I understood. | 18:03 |
| abentley | gary_poster: That was where we were already putting that kind of info, via the ScriptRequest. | 18:03 |
| gary_poster | abentley: I'm not familiar with that, but saw that class in the tests. So, you mean, we were already (argualy ab-)using the request variable data using some other mechanism? | 18:04 |
| abentley | gary_poster: Yes. | 18:05 |
| === allenap changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gary_poster | abentley: OK, cool. So the first thought (of three that I know of) is that, while I'm fine with this mechanism being continued as an expedient choice, I'd prefer an XXX (and bug) saying that we would prefer a branch of OOPS tools that supported this sort of behavior explicitly, in a separate bucket that we could then add to our oops generation machinery. | 18:11 |
| gary_poster | I'd also like to make sure that our API to support this general kind of functionality (that is, essentially annotating an oops with arbitrary information) not be tied to the current mechanism of storing and sending them. IOW, I'd like to think about what we want, and then make an API for it, and then use this underlying implementation as an expedient solution for now. | 18:11 |
| gary_poster | I have some thoughts on what it is I think we want, but can you go along with me so far, or do you have some concerns or thoughts? | 18:11 |
| gary_poster | That was not said very clearly, sorry. | 18:12 |
| abentley | gary_poster: I'm fine with the XXX/bug. | 18:12 |
| gary_poster | cool. OK, moving on. | 18:12 |
| abentley | I'm not sure I understand about tying the annotation functionality to the current mechanism. | 18:13 |
| abentley | gary_poster: ^ | 18:15 |
| gary_poster | abentley: right. what I mean is, let's think about what we want on the error tool, divorced from the fact that we are going to implement it by stuffing it in request variables for now. | 18:16 |
| gary_poster | Specifically, is this functionality really only for "Error Variables"? It seems like that's one possible usage of many. You could also store arbitrary status messages. | 18:16 |
| gary_poster | I almost see this more as a logger behavior, where the API should be to pass a string, with optional kw values to be interpolated into the string if it is turned into an OOPS. We're annotating the OOPS with some information. | 18:16 |
| gary_poster | Then, within the OOPS request variables, the location of the invocation of the context (i.e., the frame, as with a traceback) could be the first value, and the message would be the second. This would suggest a different name for the function, of course. ``contextAnnotation``? ``contextLog``? ``contextMessage``? | 18:16 |
| abentley | gary_poster: I could see that maybe we'd want to do that, but YAGNI. | 18:19 |
| abentley | gary_poster: (I tried to avoid the word 'context', especially because of the association of context managers with "with" statements) | 18:21 |
| bac | EdwinGrubbs: could you do a review for me? | 18:27 |
| EdwinGrubbs | bac: sure | 18:28 |
| bac | EdwinGrubbs: great. i've sent it off but it hasn't appeared yet | 18:28 |
| bac | EdwinGrubbs: https://code.edge.launchpad.net/~bac/launchpad/bug-487965/+merge/15251 | 18:33 |
| gary_poster | abentley: sorry for taking so long, but you reminded me that you are duplicating functionality that is already provided. I was getting the details for you. I'd be happy if you used this mechanism. It is even less work, because it should work now. If you are willing to use this, the branch should be reduced to simply adding the desired information to the job code, and moving on. | 18:46 |
| gary_poster | Our tracebacks are actually generated a zope exceptions package (http://svn.zope.org/zope.exceptions/trunk/src/zope/exceptions/exceptionformatter.py?view=auto fwiw). If you put a variable named "__traceback_info__" with a string (or a value that can be cast as a string, such as a dict of strings) then it will be included in the traceback generated by the error tool. Alternatively or additionally, if you put a variable | 18:46 |
| gary_poster | "__traceback_supplement__" with a tuple/list containing a callable first, followed by any args to call the callable with, then the output will be included in the traceback. | 18:46 |
| gary_poster | I think this would easily satisfy what you need, and would do so without polluting the request variables, and would do so clearly indicating the frame in which the information was defined, and would do so without new code. | 18:46 |
| abentley | gary_poster: It seems like these variables need to be globals, correct? | 18:51 |
| gary_poster | abentley: they can be locals | 18:51 |
| abentley | gary_poster: So you define them in the caller, and if the callee oopses, it gets access to them? | 18:52 |
| gary_poster | abentley: not sure what the caller and the callee are in your question. you define the variables wherever you know about them. If there is an exception, and the error reporting utility is asked to render it, it generates a traceback including these variables in the output. The tests for this are all unit tests; a doc test would actually have been nice. Let me pastebin an example modified from the unit tests. | 18:57 |
| abentley | gary_poster: In this case, the caller would be JobRunner.runAll, and the callee would be Diff.fromFile. | 19:00 |
| gary_poster | abentley: http://paste.ubuntu.com/327911/ the callee doesn't look at the values. the error reporting utility does, if it is rendering the traceback. in this example, the print statement is essentially what the error reporting utility is doing. Notice that the traceback has the interesting information in the middle of it. This is what you should see in the OOPS tool. | 19:05 |
| EdwinGrubbs | bac: review sent | 19:06 |
| gary_poster | I'd be fine with sugar to make this prettier, but the results are better, I think. And it is already part of what we have. | 19:08 |
| gary_poster | (sugar: e.g., a callable that dumps the magic name in the locals or somthing. I don't like the frame tricks, but that would keep you from misspelling __traceback_info__ without a warning) | 19:08 |
| bac | thanks EdwinGrubbs. nice catches, especially the cmp logic error that has been in production for a very long time | 19:10 |
| abentley | gary_poster: I'm not getting good results, but it looks like the test suite doing stupid stuff to the traceback. I'll let you know. | 19:14 |
| gary_poster | abentley: ok cool | 19:14 |
| leonardr | random zope question for gary or anyone really | 19:33 |
| leonardr | i don't understand why these two lines of code are not equivalent: | 19:33 |
| abentley | gary_poster: any idea why the tracebacks I generate in the test suite are only a single line? | 19:33 |
| leonardr | getMultiAdapter((context, request), ICollection) | 19:33 |
| leonardr | ICollection((context, request)) | 19:33 |
| gary_poster | leonardr: because in the second case you are trying to adapt a tuple. There's a discussion about that on the zope list today as a matter of fact | 19:35 |
| gary_poster | abentley: I'm afraid not | 19:35 |
| gary_poster | abentley: Happy to futz if you want to give me a branch or a patch | 19:36 |
| leonardr | gary: i'll look on the zope list if that'll answer my question more easily, but why is getMultiAdapter(tuple, interface) not 'adapting a tuple'? | 19:37 |
| gary_poster | leonardr: oh! sorry | 19:37 |
| gary_poster | leonardr: oh, no, I was right the first time | 19:38 |
| abentley | gary_poster: I think it's because I'm raising and catching the exception in the same function. The traceback doesn't include the callstack of the function that catches the exception. | 19:38 |
| gary_poster | leonardr: because that's the api | 19:38 |
| gary_poster | leonardr: these are equivalent: | 19:38 |
| leonardr | i guess that's good enough for me, i was just curious | 19:38 |
| gary_poster | getMultiAdapter((context,), IFoo) | 19:39 |
| gary_poster | IFoo(context) | 19:39 |
| gary_poster | (mostly equivalent; there is some stuff that I consider legacy that happens in the second case, but it shouldn't affect anything) | 19:39 |
| leonardr | so if i did ICollection((context, request),) it might work? not that i'm going to try that, at that point it's ugly | 19:40 |
| gary_poster | abentley: oh, so this is a case in which the zope tools are insufficient, and we should go back to your original idea? | 19:40 |
| gary_poster | leonardr: no | 19:41 |
| abentley | gary_poster: I think this system only works when the exception handler is lower in the callstack than the method embedding the info. | 19:41 |
| leonardr | gary: i see why not. ok | 19:41 |
| gary_poster | abentley: well, you can pass the exception info explicitly to the reporter. is this the last bit of your original diff? looking. | 19:42 |
| gary_poster | abentley: I assume what you are trying is in lib/lp/services/job/runner.py . could I see a pastebin of what you are trying? | 19:43 |
| abentley | gary_poster: http://pastebin.ubuntu.com/327933/ | 19:45 |
| gary_poster | looking thx | 19:45 |
| abentley | gary_poster: The JobRunner bit: http://pastebin.ubuntu.com/327936/ | 19:46 |
| gary_poster | abentley: http://pastebin.ubuntu.com/327937/ | 19:48 |
| gary_poster | or use a repr | 19:48 |
| abentley | gary_poster: But runAll won't appear in the traceback. | 19:49 |
| gary_poster | I mean, what you originally had was ``__traceback_info__ = repr(dict(job.getOopsVars()))``. Maybe that is necessary for your keys or values, don't know | 19:49 |
| gary_poster | huh, ok... http://paste.ubuntu.com/327911/ this seemed to imply that it would be fine. Trying equivalent in interpreter. | 19:50 |
| abentley | gary_poster: That's because the traceback includes the stack frame where the exception is handled. It doesn't include the stack frame of the caller of the exception handler, AFAICT. | 19:52 |
| gary_poster | abentley: fwiw, http://paste.ubuntu.com/327940/ | 19:52 |
| gary_poster | oh wait, maybe I misunderstand | 19:53 |
| gary_poster | reviewing your code again | 19:53 |
| gary_poster | abentley: oh! you are saying, because you catch in runJob and reraise, this is hosed, right? Sorry for not following before | 19:53 |
| abentley | gary_poster: No, I didn't mean that. | 19:54 |
| gary_poster | ok, still not following then. | 19:54 |
| abentley | gary_poster: You seem to have disproved my theory about why I'm only gettting a single line of traceback. | 19:54 |
| gary_poster | ah, ok | 19:55 |
| gary_poster | abentley: I think what I said might be the case though | 19:55 |
| abentley | gary_poster: I'm not looking at the rereaise case. I'm looking at the case where Diff.fromFile handles an exception and does not reraise it. | 19:56 |
| gary_poster | abentley: ok, will try to find that. Was about to suggest http://pastebin.ubuntu.com/327944/ | 19:57 |
| abentley | gary_poster: I was simultating that with the handleError function. | 19:57 |
| gary_poster | abentley: ah-ha. with handleError, yes, your frame annotation will never get a chance to be called. Is that a real example? That is, do you need to be able to handle a job that calls reporter.handling itself? If so, you can either use the mechanism I described within the job itself, or we need to go back to your original approach, and my other comments for it. | 20:00 |
| gary_poster | s/That is, do you need to be able to handle a job that calls reporter.handling itself?/That is, do you need a job to be able to call reporter.handling itself?/ | 20:01 |
| abentley | gary_poster: Yes, I need a job to be able to reporter.handling itself. That was the motivating case. | 20:01 |
| abentley | I'm not sure how we could propagate that info into Diff.fromFile without doing something similar to what I proposed. | 20:03 |
| gary_poster | abentley, ok, chalk that one to the perils of communication then. Sorry fr not understanding your use case well enough. So, back to your original design, yes. We ought to document why and when it is appropriate as opposed to annotating a frame. And I'd prefer for this to be a "give me something I can str" API for now, rather than a key-value pair API. So to cut to the chase, my requests on your original branch: | 20:05 |
| abentley | gary_poster: The reason I'm not just passing the vars into raising via the ScriptRequest is that Diff.fromFile is high in the callstack (that is, has many indirect callers) and has no information about the job, or any reason to expect it's being called from a job. | 20:05 |
| gary_poster | abentley: I like your API generally; I'm already sold on the basic idea | 20:05 |
| gary_poster | - add a bug/XXX saying that we want to store the information from this API someplace else other than polluting the request info | 20:06 |
| gary_poster | - change the API to accept something that is or can be cast into a string | 20:07 |
| gary_poster | - stuff the value into the request variables with some nasty key so it is clear where it is coming from (i.e., not the request) | 20:07 |
| gary_poster | - move on | 20:07 |
| gary_poster | abentley: you ok with that? happy to explain rationale if needed | 20:08 |
| gary_poster | abentley: forgot one: change the name to something appropriate to this behavior of recording a string | 20:08 |
| abentley | gary_poster: Where do you want the "give me something I can str?" If you want that in contextErrorVariables, that doesn't lend itself to multiple uses. | 20:11 |
| gary_poster | abentley: yes, that was my intent (though with a different name, as I said). why not? The end goal is to produce something that can be output to an error report--a string. | 20:13 |
| abentley | gary_poster: In my design, you can do contextErrorVariables(a=b) in a caller, and contextErrorVariables(c=d) in a callee, and they're all nicely combined together. | 20:13 |
| === sinzui changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gary_poster | abentley: not sure why that is an advantage over errorAnnotation(('foo', 'bar')) and errorAnnotation(('baz', 'bing')). That can produce request vars of '__error_annotation__': "('foo', 'bar')" and '__error_annotation__': "('baz', 'bing')" | 20:15 |
| abentley | gary_poster: You end up with multiple strings that way, one for every invocation of contextErrorVariables. Then you have to combine them somehow, and you lose alphabetical sorting, so you probably have to put in separators so people know that the sorting restarts. | 20:15 |
| gary_poster | that is, the nesting and duplicate values is fine | 20:16 |
| gary_poster | abentley: I don't understand why the sorting is valuable. | 20:16 |
| sinzui | EdwinGrubbs: can you review my fix for the milestone table. | 20:16 |
| EdwinGrubbs | sinzui: sure | 20:16 |
| abentley | gary_poster: Sorting makes it easy to find something in a long list. | 20:16 |
| gary_poster | abentley: Sure; I'm fine with sorting request variables. Sorting these things makes the order unknown. If you make the modification I'm suggesting, you can find these annotations easily one place in the request variables for now. You could even make the key have a suffix based on the nesting depth of the annotation. | 20:19 |
| gary_poster | I really don't like the fact that you can't tell what came from the request and what came from your addition. I also don't think key-value pairs are something to cook into the API. | 20:19 |
| abentley | gary_poster: I wasn't arguing for sticking them in with the request variables. | 20:19 |
| gary_poster | abentley: oh...not cleat then, sorry | 20:20 |
| gary_poster | clear | 20:20 |
| abentley | gary_poster: I don't understand what's wrong with a key/value API. I like to keep my variables machine readable until it's time to format them. | 20:22 |
| abentley | I don't understand why sorting key/value pairs based on their key name would make their order unknown. | 20:23 |
| gary_poster | abentley: I don't understand why you don't understand, which usually means I have a base misunderstanding. I think we should try to bring this to a close. Do you want to try Skype, to move more quickly? | 20:25 |
| gary_poster | or SIP is fine | 20:26 |
| abentley | gary_poster: let's try SIP. | 20:27 |
| thumper | gary_poster: at least for our use case, the order doesn't matter | 20:27 |
| gary_poster | ok | 20:27 |
| thumper | gary_poster: in what situations are you thinking that the order might matter? | 20:27 |
| abentley | gary_poster: I am sip:7369@canonical.com | 20:29 |
| === salgado is now known as salgado-afk | ||
| gary_poster | thumper: I believe Python's sort now maintains ordering within equivalent values, so if that's the case then it's a matter of readability, because you can't immediately tell what values come from the request, what come from one of these error log annotations, and what comes from a lower level of error log annotations. When I look at the results, I want to be able to see what values come from where, without having to loo | 20:31 |
| gary_poster | sourcecode. | 20:31 |
| gary_poster | Maybe abentley will convince me of the error of my ways; about to call. :-) | 20:31 |
| === matsubara is now known as matsubara-afk | ||
| === wgrant_ is now known as wgrant | ||
| gary_poster | abentley: does this jibe with your understanding? http://paste.ubuntu.com/328032/ | 22:03 |
| abentley | gary_poster: That's great. | 22:10 |
| gary_poster | cool, thanks. submitting | 22:10 |
| EdwinGrubbs | sinzui: review sent | 22:40 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| sinzui | thanks | 22:40 |
| === 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 | ||
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/allow-admin-to-set-branch-privacy/+merge/15259 pretty please | 23:34 |
| thumper | this will make our losas happy | 23:34 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!