/srv/irclogs.ubuntu.com/2010/09/10/#launchpad-reviews.txt

=== 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
rockstarwallyworld_, I did a ui review for the link-bugs-in-merge-proposal, but I didn't do a code review.00:56
wallyworld_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 it00:57
wallyworld_rockstar: the change was just a simple pt file edit00:57
rockstarwallyworld_, okay.00:58
wallyworld_rockstar: so you don't think we need a light grey horizontal separator between the linked bugs and comments sections?00:58
rockstarwallyworld_, not really.00:58
wallyworld_rockstar: np. thanks for reviewong00:59
lifelessmwhudson: are you on the phone?01:34
mwhudsonlifeless: no longer01:35
lifelessmwhudson: are you @ lunch ?01:35
mwhudsonlifeless: not yet01:35
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/3505301:35
lifeless2 character patch01:35
lifelesssave hundreds of unnecessary DB calls.01:35
mwhudsonno diff yet01:35
lifeless+++ lib/canonical/launchpad/database/librarian.py       2010-09-10 00:32:25 +000001:35
lifeless@@ -236,7 +236,7 @@01:36
lifeless 01:36
lifeless     @property01:36
lifeless     def deleted(self):01:36
lifeless-        return self.content is None01:36
lifeless+        return self.contentID is None01:36
mwhudsonoh right01:37
lifelessmwhudson: yeah.01:38
mwhudsonlifeless: reviewed01:38
lifelessa 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:38
mwhudsonalso, yay for orms i guess01:39
lifelessye01:40
lifelessyes01:40
lifelessand yay for metaprogramming01:40
=== gary_poster_ is now known as gary_poster
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/3506007:45
lifelessjtv: hi08:43
jtvlifeless: hi08:43
* jtv looks at channel name08:43
lifelessjtv: interested in reviewing a small refactoring ?08:43
jtvah, you wish for a review?08:43
jtvsure08:43
lifeless+2 total lines, no tests (because it just refactors)08:43
lifelessjtv: https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/3506008:44
jtvlifeless: I see 211 lines, 4 files08:44
lifelessyes; measured that way :)08:45
lifelessdiffstat is what I was referring too :)08:45
lifeless doc/timeout.txt       |    2 -08:45
lifeless webapp/adapter.py     |   85 +++++++++++++++++++++++++++++++-------------------08:45
lifeless webapp/configure.zcml |    2 -08:45
lifeless webapp/servers.py     |   21 ------------08:45
lifeless 4 files changed, 56 insertions(+), 54 deletions(-)08:45
jtvlifeless: "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
lifelessits not useless08:46
lifelessit returns None08:46
lifelessthats the existing defined behaviour that wasn't previous documented08:46
lifelessI can rephrase08:47
lifelessperhaps08:47
lifeless'If timeouts are disabled, None is returned.'08:47
jtvI think the confusion stems from the new name.08:48
lifelessthe root cause is that we check for timeouts outside of requests08:48
lifelessthis is bogus08:48
lifelessbut we do it because 'request' is only well defined in principle, not well defined in reality.08:48
jtvIt's bewildering.08:48
lifelesssame root issue I was referring to the other week, that makes things complex08:49
jtvISTM this function combines two different things: getting the time budget, and checking for timeout.08:49
lifelessI'm sneaking up on fixing it by consolidating all the awkward code08:49
lifelessjtv: yes, but its less code this way (I tried the alternative)08:50
jtvDoes this mean we're not going to get any time information where timeouts are disabled?08:51
lifelessno, this doesn't affect duration and start checks08:51
jtvI hope you shoot the lot of these functions and replace them all with one. :)08:52
lifelessI want Request.annotations.start_time and Request.annotations.timeout_time08:52
jtvBut given the dual role (compute remaining time and check for timeout), maybe this should be called check_* instead of get_*?08:52
lifelessand then we can avoid recalculating stuff all the time08:52
lifelessjtv: I think either is fine08:53
jtvI 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
lifelessno_exceptions=True08:54
jtvBut that's documented as returning a negative number.08:55
lifelessoh08:55
lifelessstale docs - I found more things to cleanup as I went08:55
lifelesslet me tweak08:55
lifelessjust delete that paragraph in your mind08:55
lifelessI've just deleted it08:55
jtvexcuse me while I go buy a keg of beer08:56
lifelesslol sure08:56
jtvnow let's hope I hit the braincells with that particular paragraph in them pretty soon, or this could get ugly08:56
jtvlifeless: maybe note in the set_launchpad_default_timeout docstring that this is triggered from the start event or whatever?08:58
jtvWow, this diff has some weird effects… is _check_expired left empty but for the docstring?08:59
jtvah, I see that you merely moved set_launchpad_default_timeout09:00
jtvhey danilos09:00
daniloshey09:01
jtvhi henninge!09:02
jtvlifeless: approved09:02
lifelessjtv: thanks, will add that note09:06
=== 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
lifelessmwhudson: 8 second time reduction from that LFA patch I did10:26
bigjoolsadeuring: hello!  I have a small branch for your delight.11:42
bigjoolshttps://code.edge.launchpad.net/~julian-edwards/launchpad/disable-arch-bug-633139/+merge/3507211:42
adeuringbigjools: I'll look11:42
bigjoolsthanks11:42
=== 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
noodles775Hi adeuring, when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/627957-differences-schema-update/+merge/3507311:55
noodles775I'm requesting the db-reviews too.11:55
adeuringnoodles775: sure11:55
noodles775Ta.11:57
adeuringbigjools: r=me11:58
=== 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
bigjoolsadeuring: thanks!12:01
adeuringnoodles775: _updateType() does not return anything. So, "return type_updated or status_updated" in update() looks a bit odd ;)12:23
noodles775adeuring: thanks, yeah, it should be returning whether it updated the model.12:49
adeuringnoodles775: that's worth a test, I think13:03
noodles775Indeed... I'd thought I had tested that, but it must have just been testing the return value of the other _updated helper method.13:04
adeuringnoodles775: 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:10
noodles775adeuring: 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
noodles775so no, it's not something that would happen in the duration of a request.13:16
adeuringnoodles775: could you add comments with this explanation to the cache.clear() calls?13:17
noodles775adeuring: to each one? (I think there are 8... how about at the top of the test case?)13:18
adeuringnoodles775: yes, that should be enough ;)13:18
adeuringnoodles775: ok, with these changes, r=me13:46
bacgood morning adeuring.   just checking in.  i'll be along shortly to help.13:47
adeuringbac: morning bac13:47
=== matsubara-afk is now known as matsubara
noodles775Thanks 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
adeuringnoodles775: yes, that would be even better13:51
=== 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
jelmeradeuring: 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/3509614:01
adeuringjelmer: sure14:01
adeuringjelmer: r=me, 1 nitpick14:24
=== 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
jelmeradeuring: Thanks14:25
jelmeradeuring: reviewing -775 ? :-)14:25
adeuringargh... thanks for the hint ;)14:25
=== 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
bdmurrayadeuring: could you review https://code.edge.launchpad.net/~brian-murray/launchpad/display-dupe-in-portlet-dupe-subscribers/+merge/34501?14:46
adeuringbdmurray: sure14:46
=== 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
jcsackettbac: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738/+merge/3501814:54
jcsackettbac, are you a UI reviewer as well?14:54
bacjcsackett: i am not14:54
jcsackettokay; i'll ping sinzui about it.14:55
=== 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
* jcsackett sees he's already in the channel.14:55
jcsackettsinzui: any chance i could request you for ui review on my unknown blueprints branch? all nitpickery welcome.14:55
sinzuijcsackett, I am asking salgado do the first round of UI reviews so that he gets experience. I can follow his review with mine14:56
jcsackettok.14:56
jcsacketthe's not online now, it looks like; i'll just set him as the requested reviewer.14:57
bacyowzer, jcsackett, 1400 lines?15:05
jcsackett...dependent branch maybe not working?15:05
bacjcsackett: perhaps15:05
jcsackettshouldn't be anywhere near that size...15:05
jcsackett...it's a little bit of code, a test, and a small template...15:05
bacit is listed...15:05
bacjcsackett: look at the generated diff and tell me if it is all for this branch15:06
baclots of answers stuff in there15:06
jcsackettbac: yeah, this all looks like stuff you shouldn't see b/c of the prereq branch.15:07
* jcsackett snaps fingers.15:07
jcsackettone sec, it may be b/c the prereq branch was updated but this one isn't.15:07
jcsackettbac: yes, i failed to update this one this morning before pinging you.15:07
jcsackettgive me one second while i do that.15:08
bacthanks15:08
jcsackettbac: 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
bacok15:30
jcsackettbac: it's a much more manageable ~300 lines. :-P15:33
bachurrah15:33
adeuringbdmurray: r=me nice work!15:35
=== 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
bacjcsackett: 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 conflicts15:48
bacjcsackett: were these branches previously based db-devel?15:49
jcsackettbac: no, i've created them with rocketfuel-branch of devel.15:49
jcsackettdb-devel should never have entered into it.15:50
bachmm.  conflicts were easy to resolve.15:50
jcsackettbac: well, that's something.15:51
jcsackettcriss-cross means there's some weird cyclic merge or something, right?15:51
bacjcsackett: i think it means there is a clear ancestor relationship15:52
bacP begets A15:52
bacA begets B15:52
bacP changes and B merges from P15:52
bacA merges from P15:52
bacB merges from A15:52
baccraziness like that15:53
bacon paper you can see the relationships but the accounting in the branch gets confused.15:53
bacthat's my best understanding15:53
jcsackettbac; that make sense.16:00
bacjcsackett: this is interesting:  if hasattr(self, 'index'):16:29
jcsackettbac: yeah, it's actually less interesting and more shaky.16:30
bacjcsackett: why does that imply zcml registration?16:30
bacjcsackett: if you keep it, you should use safe_hasattr16:30
bacjcsackett: pretent i'm using my spock voice when i say "interesting"16:31
jcsackettbac: dig. :-P16:31
bacjcsackett: so, where did you come up with that?16:31
jcsackettbac: 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
jcsackettbac: 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:33
bacjcsackett: it may be worth running past gary_poster to see if there is a more obvious way to do the same16:34
jcsackettbac: dig.16:35
gary_posterbacklog-reading...16:35
jcsackettthe magic of IRC: speak of a person and watch them appear.16:35
gary_poster:-)16:35
gary_posterso...16:36
gary_posterthis is using the browser zcml tag?16:36
bacgary_poster: we're discussing line 154 of https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738/+merge/3501816:36
gary_posterwhich does the horrible "let's dynamically make a class for the fun of it" stuff?16:36
gary_posterow16:37
jcsackettoh good, i've caused physical pain with code.16:38
gary_poster:-)16:38
gary_posterok, so let me see if I understand the goal16:39
gary_posterthis base class is supposed to sometimes be used with a template, and sometimes not16:39
gary_posterin all cases, we use that horrible browser:page zcml, or something like that16:39
gary_posterif we use browser:page and this base class and a template...16:40
gary_posterthen this base class will win16:40
gary_posterand so you have to go sniff for the original template16:40
gary_posterand you are doing that with the index thing because it seems to work :-)16:41
gary_posterCorrect me if I'm wrong; I'll proceed as if I'm right.16:41
jcsackettgary_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
jcsackettbasically you sound like you've got it.16:41
gary_posterCool.  Don't bother trying to be clear on browser:page is my advice ;-)16:42
jcsackettgary_poster: advice i'm happy to take. :-)16:42
jcsackettthe 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:42
jcsackettbut 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:43
gary_posterMy fist thought is "don't do that."  I would prefer to see a base class that always expects to control the template.16:45
gary_posterI 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
gary_poster...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
gary_poster(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:45
gary_posterI appear to be feeling crusty and cantankerous this morning :-P :-)16:46
jcsackettthat'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 learning16:47
* gary_poster was called away, but is back :-P16:53
gary_posterz3c.ptcompat also makes angels cry, even though it is very useful for switching to chameleon tentatively... :-/16:57
=== deryck is now known as deryck[lunch]
jcsackettso, basically, i've sinned against code-god. :-P16:58
=== Ursinha is now known as Ursinha-lunch
gary_poster:-) no, not you. The environment in which you find yourself16:59
=== 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
gary_posterSorry for taking so long.  I'm working my way through gems like this:17:00
gary_poster    if offering is None:17:00
gary_poster        offering = sys._getframe(1).f_globals17:00
gary_poster    if isinstance(offering, dict):17:00
gary_poster        offering = package_home(offering)17:00
gary_poster    bases += (simpleviewclass.simple, )17:00
gary_poster    class_ = type("SimpleViewClass from %s" % src, bases,17:00
gary_poster                  {'index': ViewPageTemplateFile(src, offering),17:00
gary_poster                   '__name__': name})17:00
jcsackettow.17:01
gary_postersee?  see?  I told you :-)17:01
jcsackettheheh.17:01
gary_posterok, 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
gary_posterThe 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
gary_posterAs 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
gary_posterSo, 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.17:11
=== 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
jcsackettgary_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:13
gary_posterjcsackett, 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. :-P17:14
* jcsackett laughs.17:15
jcsackettokay, thanks gary_poster.17:15
gary_posternp, jcsackett :-)17:15
bacjcsackett: i'm fine with the above.17:16
jcsackettbac: 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
bacjcsackett: push up a diff and i'll look at it again.17:17
gary_posterjcsackett: yeah, sure.  thanks. :-)17:19
jcsackettnp.17:19
* bac lunches17:22
jcsackettbac, when you return, changes were pushed.17:36
bigjoolshey bac!  I've got a delightful branch if you would care to review it.17:47
=== 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
marsbac, 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?18:23
=== matsubara-lunch is now known as matsubara
gary_posterjcsackett: 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
gary_posteryou can see same in apidoc.launchpad.dev when you run a local instance18:34
jcsackettgary_poster: awesome. thanks!18:35
gary_posternp :-)18:35
=== 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
bacmars: where are your branches?19:51
allenapbac: Thanks for the review earlier. Also, do you mind if I resume a Thursday review slot, alongside noodles?20:29
bacallenap: mind?  heck no!20:30
allenapbac: :)20:30
bacglad to have you back, gavin!20:30
allenapCheers bac, good to be back.20:31
=== jelmer_ is now known as jelmer
lifelessallenap: welcome21:18
=== 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

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!