=== Ursinha-afk is now known as Ursinha === rockstar changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [00:56] wallyworld_, I did a ui review for the link-bugs-in-merge-proposal, but I didn't do a code review. [00:57] rockstar: thumper did the code review so it should be good. i'm going to double check that he is happy with it before i land it [00:57] rockstar: the change was just a simple pt file edit [00:58] wallyworld_, okay. [00:58] rockstar: so you don't think we need a light grey horizontal separator between the linked bugs and comments sections? [00:58] wallyworld_, not really. [00:59] rockstar: np. thanks for reviewong [01:34] mwhudson: are you on the phone? [01:35] lifeless: no longer [01:35] mwhudson: are you @ lunch ? [01:35] lifeless: not yet [01:35] https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/35053 [01:35] 2 character patch [01:35] save hundreds of unnecessary DB calls. [01:35] no diff yet [01:35] +++ lib/canonical/launchpad/database/librarian.py 2010-09-10 00:32:25 +0000 [01:36] @@ -236,7 +236,7 @@ [01:36] [01:36] @property [01:36] def deleted(self): [01:36] - return self.content is None [01:36] + return self.contentID is None [01:37] oh right [01:38] mwhudson: yeah. [01:38] lifeless: reviewed [01:38] a bit of a facepalm moment, I've seen the queries in the logs for weeks and just hadn't stopped to think about it. [01:39] also, yay for orms i guess [01:40] ye [01:40] yes [01:40] and yay for metaprogramming === gary_poster_ is now known as gary_poster [07:45] https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/35060 [08:43] jtv: hi [08:43] lifeless: hi [08:43] * jtv looks at channel name [08:43] jtv: interested in reviewing a small refactoring ? [08:43] ah, you wish for a review? [08:43] sure [08:43] +2 total lines, no tests (because it just refactors) [08:44] jtv: https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/35060 [08:44] lifeless: I see 211 lines, 4 files [08:45] yes; measured that way :) [08:45] diffstat is what I was referring too :) [08:45] doc/timeout.txt | 2 - [08:45] webapp/adapter.py | 85 +++++++++++++++++++++++++++++++------------------- [08:45] webapp/configure.zcml | 2 - [08:45] webapp/servers.py | 21 ------------ [08:45] 4 files changed, 56 insertions(+), 54 deletions(-) [08:46] lifeless: "if no timeouts are enabled, this returns None"—do you mean if there is no time left, or that this function is useless if no_timeouts == True? [08:46] its not useless [08:46] it returns None [08:46] thats the existing defined behaviour that wasn't previous documented [08:47] I can rephrase [08:47] perhaps [08:47] 'If timeouts are disabled, None is returned.' [08:48] I think the confusion stems from the new name. [08:48] the root cause is that we check for timeouts outside of requests [08:48] this is bogus [08:48] but we do it because 'request' is only well defined in principle, not well defined in reality. [08:48] It's bewildering. [08:49] same root issue I was referring to the other week, that makes things complex [08:49] ISTM this function combines two different things: getting the time budget, and checking for timeout. [08:49] I'm sneaking up on fixing it by consolidating all the awkward code [08:50] jtv: yes, but its less code this way (I tried the alternative) [08:51] Does this mean we're not going to get any time information where timeouts are disabled? [08:51] no, this doesn't affect duration and start checks [08:52] I hope you shoot the lot of these functions and replace them all with one. :) [08:52] I want Request.annotations.start_time and Request.annotations.timeout_time [08:52] But given the dual role (compute remaining time and check for timeout), maybe this should be called check_* instead of get_*? [08:52] and then we can avoid recalculating stuff all the time [08:53] jtv: I think either is fine [08:54] I find it hard to figure out from the docstring what this is meant to do. Under what circumstances will this return exactly 0 if the time budget has expired? [08:54] no_exceptions=True [08:55] But that's documented as returning a negative number. [08:55] oh [08:55] stale docs - I found more things to cleanup as I went [08:55] let me tweak [08:55] just delete that paragraph in your mind [08:55] I've just deleted it [08:56] excuse me while I go buy a keg of beer [08:56] lol sure [08:56] now let's hope I hit the braincells with that particular paragraph in them pretty soon, or this could get ugly [08:58] lifeless: maybe note in the set_launchpad_default_timeout docstring that this is triggered from the start event or whatever? [08:59] Wow, this diff has some weird effects… is _check_expired left empty but for the docstring? [09:00] ah, I see that you merely moved set_launchpad_default_timeout [09:00] hey danilos [09:01] hey [09:02] hi henninge! [09:02] lifeless: approved [09:06] jtv: thanks, will add that note === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:26] mwhudson: 8 second time reduction from that LFA patch I did [11:42] adeuring: hello! I have a small branch for your delight. [11:42] https://code.edge.launchpad.net/~julian-edwards/launchpad/disable-arch-bug-633139/+merge/35072 [11:42] bigjools: I'll look [11:42] thanks === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: bigjools || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:55] Hi adeuring, when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/627957-differences-schema-update/+merge/35073 [11:55] I'm requesting the db-reviews too. [11:55] noodles775: sure [11:57] Ta. [11:58] bigjools: r=me === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:01] adeuring: thanks! [12:23] noodles775: _updateType() does not return anything. So, "return type_updated or status_updated" in update() looks a bit odd ;) [12:49] adeuring: thanks, yeah, it should be returning whether it updated the model. [13:03] noodles775: that's worth a test, I think [13:04] Indeed... I'd thought I had tested that, but it must have just been testing the return value of the other _updated helper method. [13:10] noodles775: the IPropertyCacheManager(ds_diff).clear() in a test look a bit odd: If this is necessary in a test, wouldn't it be necessary in "real life"? [13:16] adeuring: It's necessary there because the test creates a new difference and then publishes a new version of the package, before then updating the difference. [13:16] so no, it's not something that would happen in the duration of a request. [13:17] noodles775: could you add comments with this explanation to the cache.clear() calls? [13:18] adeuring: to each one? (I think there are 8... how about at the top of the test case?) [13:18] noodles775: yes, that should be enough ;) [13:46] noodles775: ok, with these changes, r=me [13:47] good morning adeuring. just checking in. i'll be along shortly to help. [13:47] bac: morning bac === matsubara-afk is now known as matsubara [13:51] Thanks adeuring. I was also thinking, it may makes sense to clear the cache at the start of the update() method too, which would remove the need for them in the tests. [13:51] noodles775: yes, that would be even better === jelmer changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: noodles775 || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:01] adeuring: If you have time to do another soyuz review, there is one at https://code.edge.launchpad.net/~jelmer/launchpad/634045-lp-bugs-fixed/+merge/35096 [14:01] jelmer: sure [14:24] jelmer: r=me, 1 nitpick === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: -775 || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:25] adeuring: Thanks [14:25] adeuring: reviewing -775 ? :-) [14:25] argh... thanks for the hint ;) === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:46] adeuring: could you review https://code.edge.launchpad.net/~brian-murray/launchpad/display-dupe-in-portlet-dupe-subscribers/+merge/34501? [14:46] bdmurray: sure === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bdmurray, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jcsackett changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bdmurray, - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:54] bac: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738/+merge/35018 [14:54] bac, are you a UI reviewer as well? [14:54] jcsackett: i am not [14:55] okay; i'll ping sinzui about it. === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bdmurray, jcsackett || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:55] * jcsackett sees he's already in the channel. [14:55] sinzui: any chance i could request you for ui review on my unknown blueprints branch? all nitpickery welcome. [14:56] jcsackett, I am asking salgado do the first round of UI reviews so that he gets experience. I can follow his review with mine [14:56] ok. [14:57] he's not online now, it looks like; i'll just set him as the requested reviewer. [15:05] yowzer, jcsackett, 1400 lines? [15:05] ...dependent branch maybe not working? [15:05] jcsackett: perhaps [15:05] shouldn't be anywhere near that size... [15:05] ...it's a little bit of code, a test, and a small template... [15:05] it is listed... [15:06] jcsackett: look at the generated diff and tell me if it is all for this branch [15:06] lots of answers stuff in there [15:07] bac: yeah, this all looks like stuff you shouldn't see b/c of the prereq branch. [15:07] * jcsackett snaps fingers. [15:07] one sec, it may be b/c the prereq branch was updated but this one isn't. [15:07] bac: yes, i failed to update this one this morning before pinging you. [15:08] give me one second while i do that. [15:08] thanks [15:30] bac: updated, and i've attached a diff from my machine in case there's still enough divergene post merge (both branches had conflicts, i may not have resolved them identically). [15:30] ok [15:33] bac: it's a much more manageable ~300 lines. :-P [15:33] hurrah [15:35] bdmurray: r=me nice work! === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, jcsackett || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:48] jcsackett: i tried reproducing your branch. started with fresh branch from devel, merged in your pre-req, and then merged your branch. got a criss-cross merge with conflicts [15:49] jcsackett: were these branches previously based db-devel? [15:49] bac: no, i've created them with rocketfuel-branch of devel. [15:50] db-devel should never have entered into it. [15:50] hmm. conflicts were easy to resolve. [15:51] bac: well, that's something. [15:51] criss-cross means there's some weird cyclic merge or something, right? [15:52] jcsackett: i think it means there is a clear ancestor relationship [15:52] P begets A [15:52] A begets B [15:52] P changes and B merges from P [15:52] A merges from P [15:52] B merges from A [15:53] craziness like that [15:53] on paper you can see the relationships but the accounting in the branch gets confused. [15:53] that's my best understanding [16:00] bac; that make sense. [16:29] jcsackett: this is interesting: if hasattr(self, 'index'): [16:30] bac: yeah, it's actually less interesting and more shaky. [16:30] jcsackett: why does that imply zcml registration? [16:30] jcsackett: if you keep it, you should use safe_hasattr [16:31] jcsackett: pretent i'm using my spock voice when i say "interesting" [16:31] bac: dig. :-P [16:31] jcsackett: so, where did you come up with that? [16:33] bac: experimentation. if you remove the zcml registration of the template in order to use a template defined in the browser class, it loses the index attribute. since the class that's used for the base services page is the also the baseclass for a bunch of other blueprints stuff (portlets &c) it's a way to determine that this is in fact the correct view. [16:33] bac: the other means i tried failed for various reasons. checking if it was a subclass of the browser class fails, b/c the code is within the base class, so it always returns true. same problems for a number of other identifying attributes. [16:34] jcsackett: it may be worth running past gary_poster to see if there is a more obvious way to do the same [16:35] bac: dig. [16:35] backlog-reading... [16:35] the magic of IRC: speak of a person and watch them appear. [16:35] :-) [16:36] so... [16:36] this is using the browser zcml tag? [16:36] gary_poster: we're discussing line 154 of https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738/+merge/35018 [16:36] which does the horrible "let's dynamically make a class for the fun of it" stuff? [16:37] ow [16:38] oh good, i've caused physical pain with code. [16:38] :-) [16:39] ok, so let me see if I understand the goal [16:39] this base class is supposed to sometimes be used with a template, and sometimes not [16:39] in all cases, we use that horrible browser:page zcml, or something like that [16:40] if we use browser:page and this base class and a template... [16:40] then this base class will win [16:40] and so you have to go sniff for the original template [16:41] and you are doing that with the index thing because it seems to work :-) [16:41] Correct me if I'm wrong; I'll proceed as if I'm right. [16:41] gary_poster: yes. index works, there's no other argument for it--we're looking for a cleaner solution. [16:41] * jcsackett is not super clear on some zcml stuff yet. [16:41] basically you sound like you've got it. [16:42] Cool. Don't bother trying to be clear on browser:page is my advice ;-) [16:42] gary_poster: advice i'm happy to take. :-) [16:42] the reason for the template being set in the code (sometimes) is b/c when the base class should win, we want to be able to set the template based on how blueprints is being used. [16:43] but the base class is also used for portlets that those set templates call. so we have to sniff so that we don't have the template calling the portlet, which calls the template, which calls the portlet...&c. [16:45] My fist thought is "don't do that." I would prefer to see a base class that always expects to control the template. [16:45] I and others generally regard browser: page as a mistake. You can register views as adapters if you follow another pattern, and then you just use Python to decide what template is supposed to be used where. You can have a base class for this base class for the legacy zcml browser:page usage. [16:45] ...But that's probably a bigger conversation than should be having right now. Let me go find that horrible mixin class that browser:page uses and we can see if it is gives us anything usable. [16:45] (a bigger conversation because you are following established LP patterns, so the discussion I am raising has to do with LP, not this review) [16:46] I appear to be feeling crusty and cantankerous this morning :-P :-) [16:47] that's fine. it does feel like this could use some bigger redesigns, but it's definitely out of scope for this. :-) [16:47] * bac eavesdropping and learning [16:53] * gary_poster was called away, but is back :-P [16:57] z3c.ptcompat also makes angels cry, even though it is very useful for switching to chameleon tentatively... :-/ === deryck is now known as deryck[lunch] [16:58] so, basically, i've sinned against code-god. :-P === Ursinha is now known as Ursinha-lunch [16:59] :-) no, not you. The environment in which you find yourself === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:00] Sorry for taking so long. I'm working my way through gems like this: [17:00] if offering is None: [17:00] offering = sys._getframe(1).f_globals [17:00] if isinstance(offering, dict): [17:00] offering = package_home(offering) [17:00] bases += (simpleviewclass.simple, ) [17:00] class_ = type("SimpleViewClass from %s" % src, bases, [17:00] {'index': ViewPageTemplateFile(src, offering), [17:00] '__name__': name}) [17:01] ow. [17:01] see? see? I told you :-) [17:01] heheh. [17:11] ok, jcsackett, bac. What you've done, under the circumstances, is actually one of three reasonable paths. My first, preferred, path is "don't do it" and have separate base classes for sick horrible things like that mixin code and for normal Python things that work as you'd expect. :-) [17:11] The other remaining paths are to check for index, as you have done, which checks whether the code I copied above slams an index page template on the class, or (I think) to check if the class subclasses zope.app.pagetemplate.simpleviewclass.simple (as you see in the bases line). [17:11] As a reviewer, I personally would have encouraged you to go down path 1, but gently, because you are doing things exactly as the rest of LP does them now. I really am supposed to be complaining about these sorts of things more loudly to the reviewers meeting or something like that, but I always have other things to do that seem like good ideas at the time. [17:11] So, I would have accepted the "index" check, but requested that you add a comment that the "index" is a page template added by the browser:page template machinery and usually used by the zope.app.pagetemplate.simpleviewclass.simple class that is magically mixed in by the browser:page zcml directive. === matsubara is now known as matsubara-lunch === adeuring changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:13] gary_poster: do you think separating things out into separate classes could be tracked in its own bug? i think it's worth doing, but is a little out of scope on this branch (especially since this is part of bridging-the-gap and we really need to be moving on from that). [17:14] jcsackett, pretending that I were your reviewer, I would be happy to be convinced by that argument, but would say that the bug is that we should not be using browser:page. I then would mutter something about needing to go check my underarms for mites, because, of course, that would be a foundations bug, for which I'm responsible. :-P [17:15] * jcsackett laughs. [17:15] okay, thanks gary_poster. [17:15] np, jcsackett :-) [17:16] jcsackett: i'm fine with the above. [17:17] bac: okay. i'll add the comment explaining what's actually going on with the index, (as well as use safe_hasattr). gary_poster, do you want me to go ahead and file that bug for foundations? [17:17] jcsackett: push up a diff and i'll look at it again. [17:19] jcsackett: yeah, sure. thanks. :-) [17:19] np. [17:22] * bac lunches [17:36] bac, when you return, changes were pushed. [17:47] hey bac! I've got a delightful branch if you would care to review it. === Ursinha-lunch is now known as Ursinha === deryck[lunch] is now known as deryck === bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: lamont || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:23] bac, ping, I have a series of three branches for the qa-tagger code I would like to have reviewed. Would you be willing to look at some of them? === matsubara-lunch is now known as matsubara [18:34] jcsackett: btw, this is missing the JS, CSS and images to make it pretty, but it is still useful: https://devpad.canonical.com/~gary/ [18:34] you can see same in apidoc.launchpad.dev when you run a local instance [18:35] gary_poster: awesome. thanks! [18:35] np :-) === bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:51] mars: where are your branches? [20:29] bac: Thanks for the review earlier. Also, do you mind if I resume a Thursday review slot, alongside noodles? [20:30] allenap: mind? heck no! [20:30] bac: :) [20:30] glad to have you back, gavin! [20:31] Cheers bac, good to be back. === jelmer_ is now known as jelmer [21:18] allenap: welcome === Ursinha is now known as Ursinha-afk === matsubara is now known as matsubara-afk === bac changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews