| stub | I want to land https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/18198 today so I can turn on the new logging on edge on the weekend. | 07:00 |
|---|---|---|
| stub | rockstar: ^^ | 07:01 |
| stub | jtv1: And I even used real ISO8601 date formats without a / to be found! | 07:03 |
| jtv1 | stub: I'm so proud | 07:03 |
| jtv1 | But I suppose this is your way of asking for a review. | 07:04 |
| stub | I'm trying subtlety before resorting to abuse and tantrums. | 07:06 |
| jtv1 | Hmm... actually I think I'll hold off. I'd like to see the tantrums. | 07:07 |
| jtv1 | stub: I haven't reviewed code from you for a while... the difference in coding style is like night and day. | 07:49 |
| jtv1 | stub: a few small things though: line 60, in the list of date/time formats, we require a trailing comma. | 07:51 |
| stub | That is either coffee intake, or that this is greenfields rather than extending existing code. Most of the time I seems to be wading about the guts of something horrible. | 07:51 |
| stub | (ie. code I wrote 5 years ago) | 07:51 |
| jtv1 | Ah yes... as I keep trying to tell people, we can argue all we want about the intricacies of deterministic coding style, but existing code is a much stronger persuader than guideline #1728.317 sub b. | 07:52 |
| jtv1 | I once tried to implement my own optparser types as well, though wasn't very successful at it... it'd be really nice to have our own little library of option types, e.g. DistroSeries. | 07:54 |
| stub | That code is pretty much verbatim from the Python reference | 07:55 |
| jtv1 | ah so | 07:55 |
| stub | There is an example under 'extending optparse' | 07:55 |
| jtv1 | On an unrelated note: in pageperformancereport.py, everything needs docstrings. | 07:55 |
| stub | Can I leave them until later? Main goal of this particular landing is to get the URL logged in the trace logs | 07:56 |
| stub | (I can land just that change if necessary). | 07:56 |
| jtv1 | Later never comes, does it? | 07:56 |
| stub | I'm still tweaking that script - just added support for compressed logs and malformed lines. | 07:57 |
| stub | TypeError: A() takes exactly 4 arguments (149 given) | 07:57 |
| stub | hehe... | 07:57 |
| jtv1 | Very helpful... | 07:58 |
| jtv1 | The docstrings can be quite short, as long as they place things in context. F'r instance, Category could do with a one-liner "Category of <whatever it is you're categorizing>." | 07:59 |
| stub | Ok. I'll add docstrings. | 07:59 |
| jtv1 | Thanks. Also makes reviewing easier. | 07:59 |
| jtv1 | Another thing: scripts don't normally follow the entire python file template AFAIK... for one, they start out by importing _pythonpath. | 08:00 |
| jtv1 | BTW is it worth reporting the number of malformed lines, to help detect problems in recognizing record types? | 08:03 |
| jtv1 | Line 365, instead of "for i in range(0, len(histograms)): histogram = histograms[i]" use "for i, histogram in enumerate(histograms)" | 08:05 |
| stub | k | 08:11 |
| === 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 | ||
| jtv1 | stub: in "a list of Category, in report order", what does "in report order" mean? | 08:25 |
| stub | The order we want things displayed in the report. | 08:25 |
| jtv1 | stub: so that's why you wanted an order-preserving config? | 08:26 |
| stub | Yes. I need to add an order= parameter to the config or a different config format entirely so we can put important stuff first. | 08:27 |
| stub | And group similar areas etc. | 08:27 |
| jtv1 | I see... good call on leaving it out, I guess | 08:27 |
| jtv1 | stub: I think parse() would be more manageable if at least the loop for reading one file was in a separate function... less indentation, but also you won't need the "continue # skip to next line" and "break # skip to next file" stuff | 08:31 |
| stub | jtv1: Then I lose an entry that spans two log files | 08:32 |
| stub | (although maybe that doesn't matter for statistical analysis) | 08:32 |
| jtv1 | you can have entries that span two log files!? | 08:32 |
| jtv1 | Frankly though I don't see how you'd lose anything at all by doing it that way | 08:33 |
| jtv1 | Unless you've got some loop-carried variable definitions hidden in there, which would be icky. | 08:34 |
| stub | Your right. I don't lose. | 08:37 |
| jtv1 | btw I now see that "requests" only holds ongoing requests... it'd have helped to make that clear. | 08:37 |
| jtv1 | stub: what does the "$(" before the JS do? I don't think you're closing that parenthesis. | 08:43 |
| stub | jtv1: Some jquery thing I don't understand. The basic structure was cargo culted from examples. | 08:44 |
| jtv1 | stub: I'm getting old. I find myself tempted to ask "wouldn't it be easier to do this with XSLT?" | 08:46 |
| stub | The answer to that question is almost always 'no'. And when you are on a team where XSLT knowledge is nearly zero, it is almost certainly 'no'. | 08:47 |
| jtv1 | Maybe we're all getting old. | 08:48 |
| jtv1 | Anyway, please make sure that parenthesis is closed. | 08:48 |
| jtv1 | Oww, you're importing main into a script elsewhere. In that case, it should've been in the __all__ where you export it. | 08:49 |
| stub | k | 08:49 |
| stub | I need to replace parsedt with something that isn't broken (it fails when there are no milliseconds) | 08:50 |
| * jtv1 says Hail Mary in penance for assuming that it'd be called where it was defined. | 08:50 | |
| jtv1 | You need to test stuff as well. We live in Thailand; we don't want to fill our even virtual toolboxes with utensils that fall apart when we finally need them. | 08:51 |
| * jtv1 had another "most solid-looking in the shop" piece of crap spontaneously fall apart after about a week | 08:52 | |
| jtv1 | Made out of solid, chromed steel—except the moving parts which are chromed plastic. | 08:52 |
| jtv1 | stub: about the config... that's for production only? | 09:02 |
| stub | Its an example for now. The report is still a work in progress really. People will use their own configs for what reports they want to generate. | 09:03 |
| stub | But we need the logging bit landed so we can get some real data to test the report script against. | 09:04 |
| stub | Even without the URLs, I'm finding lots of issues with the parser I'm having to fix now. | 09:05 |
| stub | (using the existing trace files, sans URLs with virtual host information) | 09:05 |
| jtv1 | stub: I have to go do stuff now... should give you some time to do that. :-) | 09:20 |
| stub | I won't have time to sort that today. I want to land the logging change (the important bit I want to land so I have logs to test against on monday, allowing me to complete the report script) | 09:23 |
| === matsubara-afk is now known as matsubara | ||
| === kiko` is now known as kiko | ||
| === salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| stub | salgado: https://code.edge.launchpad.net/~stub/launchpad/trivial/+merge/18258 (trivial) | 12:07 |
| === mrevell is now known as mrevell-lunch | ||
| jtv | stub: re-doing the whole thing? | 13:08 |
| stub | The report will change enough. Changed a bit already. | 13:08 |
| stub | The report isn't the important thing though since it is a work in progress | 13:09 |
| stub | And there is little point polishing it until we can run some real data through it. I'm just working on memory issues now. | 13:09 |
| === mrevell-lunch is now known as mrevell | ||
| jtv | rockstar: "nag" | 13:47 |
| === matsubara is now known as matsubara-lunch | ||
| === salgado is now known as salgado-lunch | ||
| bac | beuno: we talked about this new portlet earlier in the week and it is my understanding you were happy with it. could you mark a UI review on it? https://code.edge.launchpad.net/~bac/launchpad/bug-490518-link-suggestion/+merge/18040 | 15:08 |
| beuno | bac, sure, ine sec | 15:16 |
| === matsubara-lunch is now known as matsubara | ||
| === salgado-lunch is now known as salgado | ||
| === EdwinGrubbs is now known as Edwin-lunch | ||
| === gary_poster is now known as gary-lunch | ||
| === jml is now known as joneaux | ||
| bac | beuno: got a sec yet? :) | 18:36 |
| === joneaux is now known as jml | ||
| beuno | bac, ah | 18:37 |
| beuno | it seems I lied | 18:37 |
| beuno | really opening up the MP this time | 18:37 |
| bac | stretched the truth | 18:37 |
| bac | had good intentions | 18:37 |
| beuno | bac, any screenshots I can look at? | 18:38 |
| bac | beuno: just the same old ones from earlier. let me dig them up | 18:38 |
| bac | http://people.canonical.com/~bac/linkup-multiple-ff.png | 18:38 |
| bac | http://people.canonical.com/~bac/linkup-none.png | 18:39 |
| bac | http://people.canonical.com/~bac/linkup-one.png | 18:39 |
| bac | beuno: recall the spacing issues are a webkit problem and a bug has been filed for it | 18:39 |
| beuno | ah | 18:39 |
| beuno | I remember now | 18:39 |
| beuno | approved\ | 18:40 |
| bac | you the rock | 18:40 |
| beuno | I think it's more of a balad today | 18:40 |
| === salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | salgado, A review. I am needing one. | 19:16 |
| salgado | rockstar, how big is it? I'll need to leave soon | 19:17 |
| rockstar | salgado, it's not very big, but is a big priority. | 19:18 |
| salgado | rockstar, ok, where is it? | 19:18 |
| rockstar | salgado, it's the branch to fix the problems detailed in abentley's Root Cause Analysis email. | 19:18 |
| rockstar | salgado, finishing letter now. | 19:18 |
| rockstar | salgado, https://code.edge.launchpad.net/~rockstar/launchpad/scanner-events/+merge/18281 | 19:24 |
| === gary-lunch is now known as gary_poster | ||
| salgado | rockstar, care to tell me a bit about IBranchScanJobSource and how it is tied together to the rest of the branch scanning system? | 19:36 |
| rockstar | salgado, so there is the IBranchScanJob and IBranchScanJobSource - I'm not sure why we have two interfaces, but one is for instances usually, and the other is for classmethods. | 19:40 |
| rockstar | IBranchScanJobSource specifically gives you all the methods you need to get all the BranchScanJobs, etc. | 19:40 |
| abentley | rockstar: The two interfaces are because stateful things don't really work as utilities. | 19:41 |
| deryck | salgado, I have one for you, as you have time, please. | 19:41 |
| === deryck changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| salgado | deryck, I won't have time today, sorry. reviewing one for rockstar now and I'll have to leave after that | 19:42 |
| deryck | salgado, no worries. | 19:42 |
| deryck | salgado, thanks anyway! | 19:42 |
| rockstar | All BranchJob types have a wrapper around them that provides extra information. BranchScanJob is the class in this case, and it has no extra information needed. | 19:42 |
| === deryck changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| deryck | yo rockstar ! | 19:42 |
| rockstar | deryck, I was just about to offer. | 19:42 |
| rockstar | :) | 19:42 |
| deryck | :-) | 19:42 |
| deryck | you da man! | 19:42 |
| deryck | rockstar, https://code.launchpad.net/~deryck/launchpad/not-implemented-errors/+merge/18283 | 19:42 |
| === salgado 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 | ||
| salgado | rockstar, ISTM that .run() now expects self.server to be not-None, but do we have anything to make sure contextManager() is called before run(), to make sure self.server is set? | 19:44 |
| rockstar | salgado, it's actually cls.server, but yes, the JobCronScript deals with all of that. | 19:45 |
| rockstar | salgado, it even uses the context manager stuff that barry posted to the list about. | 19:45 |
| salgado | rockstar, would it be worth having an "assert self.server is not None" in run(), just to make it clear? | 19:46 |
| rockstar | salgado, well, run is called in a with statement, so it would bomb out before then. | 19:47 |
| salgado | also, what do you think of a test showing that all the expected subscribers are actually registered for the event? the existing one only shows (implicitly) that the subscriber for generating diffs is registered, but not the others | 19:48 |
| rockstar | AIUI, the with is what handles the contextManager call, and knows how to clean up after itself. | 19:48 |
| rockstar | salgado, well, I don't think this fix is the way forward. abentley, thumper, and I are all in agreement there. It's the how we're not quite sure of. | 19:49 |
| abentley | salgado: This isn't a nice fix, this is a quick, minimally-invasive fix. | 19:49 |
| salgado | ok, maybe that'll be clear for someone reading the whole thing and not just the diff, but maybe a comment explaining how we know self.server will be set before run() is called would be nice | 19:49 |
| rockstar | salgado, maybe, but this isn't the only place that uses the contextManager. | 19:50 |
| salgado | fair enough, I leave that up to you | 19:50 |
| rockstar | If you're implementing JobCronScript, you need to know about contextManager and how it works. | 19:50 |
| salgado | but if you're just reading the code, it might take you longer than necessary to understand it | 19:51 |
| rockstar | deryck, r=me | 19:51 |
| deryck | rockstar, you rock! thanks | 19:51 |
| salgado | rockstar, well, since this is more like a quick fix, then, I guess I'm happy with it. my only real concern was with the test, but I see you guys plan to address that | 19:53 |
| salgado | r=me | 19:53 |
| rockstar | salgado, yes, we definitely plan to address it. We'll need to chat with gary about it though. I'm sure he'll have some good insights. | 19:54 |
| * gary_poster likes chatting :-) | 19:55 | |
| abentley | gary_poster: I think this is what jml wanted to talk with you about, too. Basically, we want to make sure we're using Zope events the best way for us. | 19:56 |
| rockstar | gary_poster, you're probably also hating packing right now. :) | 19:56 |
| gary_poster | abentley: gotcha. rockstar: eh, I'll throw whatever in a suitcase. More worried that the predicted 10 inches of snow will bring NC to a standstill tomorrow. :-) | 19:57 |
| rockstar | gary_poster, 10 in is NC? You bet it will. | 19:57 |
| rockstar | Maybe you can convince bac to give you a ride in one of his Land Rovers. :) | 19:57 |
| gary_poster | yeah. I'm hoping it won't really do that. If it does, I may have an extra day at home | 19:57 |
| gary_poster | heh | 19:57 |
| bac | gary_poster: i'll come get you! | 19:59 |
| bac | rockstar: you taunt. you know only one works... | 20:00 |
| rockstar | bac, yea, and it sounds like the floor might be suspect. :) | 20:00 |
| bac | rockstar: nah, the floor is mostly fine. it's the frame. :( | 20:00 |
| === salgado is now known as salgado-afk | ||
| gary_poster | bac, ha, don't offer too often or I might take you up on it ;-) I've been wondering if there were a we'll-drive-in-snow taxi service around here. Depending on how bad things look I might look into it. Not keen on piling the family in to a NC snow drive to the airport if things look bad. | 20:02 |
| bac | ah, i forgot you have to fly out | 20:03 |
| bac | gary_poster: the new terminal looks very comfortable for spending the night if you get snowed in | 20:04 |
| gary_poster | bac, thanks ;-) | 20:05 |
| === matsubara is now known as matsubara-afk | ||
| === Edwin-lunch is now known as EdwinGrubbs | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
