/srv/irclogs.ubuntu.com/2010/11/04/#launchpad-reviews.txt

thumperhttps://code.launchpad.net/~thumper/launchpad/blueprint-update-lifecycle-status-with-event/+merge/4004804:02
thumperslightly bigger than I expected04:02
thumperbut < 80004:02
thumperespecially since most of that change is just whitespace changes in configure.zcml04:03
thumpermwhudson: you perhaps?04:04
* thumper needs to head off04:05
thumpermwhudson: not urgent04:05
mwhudsoni can have a look04:07
lifelessI can has review ? https://code.launchpad.net/~lifeless/launchpad/features/+merge/4004905:25
lifelessjtv: around ?06:23
jtvlifeless: yes06:23
lifelesshttps://code.launchpad.net/~lifeless/launchpad/features/+merge/4004906:23
lifelessif you can spare the time06:23
lifelessI've used a too high layer, will fix that06:24
jtvlifeless: looking06:24
lifeless(As in, I think DatabaseFunctionalLayer will work, on reflection)06:24
jtvlifeless: the cover letter suggests that for a non-team person, person.inTeam(person) will fail.  Is that correct?06:24
jtvSorry, not fail; return False.06:24
lifelessyes06:26
lifelessthats the existing behaviour06:26
jtv(This reminds me once again how nice it would be to have an optimized equivalent for "person is in one or more of these teams")06:27
lifelessperson.py line 120206:27
jtvlifeless: ah I see—person.inTeam(person) will return True, but as a special case that is handled just before this check.06:28
jtvlifeless: stupid question perhaps, but what are these scopes you mention?06:29
lifelesshttps://dev.launchpad.net/LEP/FeatureFlags06:29
lifelessend of the page06:29
jtvthanks06:30
jtvlifeless: where is the rest of the scopes tested?06:30
lifeless'are'06:30
lifelesssame source file for the ones I've added.06:31
lifelessNot all scopes are tested rigorously06:31
lifelessadding a scope adds custom configuration facilities to all flags06:31
lifelessadding a flag allows the use of all scopes to configure its value06:32
jtvI'd expect them to be relatively orthogonal..?06:33
jtvThe scopes and the features, I mean?06:33
lifelessright06:33
jtvlifeless: My editor shows the "from canonical.testing import layers" line in the test as redundant.  I know you run "make lint" regularly; didn't it complain?06:35
lifelessI don't actually, it has sufficiently high noise that I just eyeball things.06:37
lifelessI've filed bugs on the errata.06:37
jtvI see.  While I'm going over superficialities, I notice that the two assertion lines just above TestDBScopes seem to test the exact same thing.06:39
lifelesssec, I'm just pushing a new branch06:39
lifelessI'll context switch and look with you06:39
lifelesshttps://code.launchpad.net/~lifeless/launchpad/recipes/+merge/4005006:40
lifelessjtv: your editor is wrong about the import of layers being redundant, AFAICT06:42
lifelessjtv: if you mean in test_webapp.py06:42
jtvyes06:42
jtvI don't find the word "layers" anywhere else in that file.06:42
jtvAh, it was redundant but isn't any more.  Good show.06:43
lifeless--- lib/lp/services/features/tests/test_webapp.py       2010-11-04 05:20:30 +000006:43
lifeless+++ lib/lp/services/features/tests/test_webapp.py       2010-11-04 06:40:55 +000006:43
lifeless@@ -42,12 +42,12 @@06:43
lifeless         scopes = webapp.ScopesFromRequest(request)06:43
lifeless         self.assertTrue(scopes.lookup('pageid:'))06:43
lifeless         self.assertFalse(scopes.lookup('pageid:foo'))06:43
lifeless-        self.assertFalse(scopes.lookup('pageid:foo'))06:44
lifeless+        self.assertFalse(scopes.lookup('pageid:foo:bar'))06:44
lifeless class TestDBScopes(TestCaseWithFactory):06:44
lifeless-    layer = layers.LaunchpadFunctionalLayer06:44
lifeless+    layer = layers.DatabaseFunctionalLayer06:44
lifelessanyhow - thats what I have as a pending commit for this branch to tweak it.06:44
lifeless     def test_team_scope_outside_team(self):06:44
lifeless         request = LaunchpadTestRequest()06:44
lifelessbah, pastefail :)06:44
jtvlifeless: the reladiff looks fine… one more question though: currently inTeam("bogus-team-name") is an error afaics.  Is it really right to drop that in general?  Won't it risk hiding bugs?06:47
jtv(It's not a very _clear_ error mind you)06:47
lifelessjtv: so, its a risk, but is it a significant one?06:48
jtvI get the feeling that it's more of an interpretation thing that you want specifically in this use-case.06:48
lifelessjtv: inTeam(team_object) will be reliable.06:48
jtvMaybe we should ask someone dyslexic.  :-)06:48
lifelessjtv: so the only place that this is a new risk is where we have a /string/ reference to a team object06:48
lifelessand are doing a membership check06:48
lifelessjtv: I think that that will only be in places like this06:49
jtvYes, it's the sort of thing you might do in tests I guess.06:49
lifelesswhere we'd want to assume that the string data is potentially stale and just answer 'no'06:49
jtvIf the string data is stale in a test, an error will be much more helpful than unexpected results—or even worse, expected results for the wrong reason.06:50
lifelessjtv: thats true, OTOH no code does it today - I hope :)06:51
lifelessjtv: I'd rather iterate on this if someone wants to do that, rather than polish an interface thats not really desirable anywahy06:51
jtvIf you hit this mistake today, you'd get an error.06:51
lifelessand tomorrow, you'd get False, say 'wtf', and look.06:52
jtv_If_ the False was not coincidentally what I expected.  Question then would be, how many other places would I look first?06:52
jtv(One problem with bools is it's so damned easy to hit the right value by mistake)06:53
lifelessgrep -n "inTeam('"06:53
* jtv was doing the same06:53
lifelessso I think I'm fixing a latent bug06:53
lifelessif someone deletes or renames soyuz-team06:54
lifelesslp will start blowing up06:54
lifelessyet returning False would be appropriate in the existing uses.06:54
jtvWhich seems a lot more appropriate than people being locked out for reasons they can't figure out.06:54
lifelessinTeam is a query interface.06:55
lifelessits not an assertion interface.06:55
lifelessI think you're arguing for behaviour appropriate in an assertion/predicate interface.06:55
lifelessI'd be delighted to raise an exception in such an interface.06:55
jtvActually I think the interface itself is somewhat misplaced.06:56
lifelesssure06:56
jtvIt's so fundamental and low-level that you don't expect a lot of intelligence in there.06:56
jtvI mistrust "smart" code.06:56
lifelessI just don't see the benefit of an exception here when there are three users and none of them want an exception.06:56
lifelessYAGNI06:56
jtvWell the reason I'm careful with this is that the situation is such that if we ever had a need, we wouldn't see it from looking at the current codebase.06:57
jtvWe'd only know we had a need if we wasted a lot of time debugging.06:57
lifelessjtv: sure we would, someone would have added a test encoding the behaviour they wanted.06:57
lifelessI should, in fact, add a test for the change I made there.06:58
lifelessthe implicit test from this code path isn't really sufficient06:58
jtvThat was next on my list, yes.  :)06:58
jtvAfter that comes documentation.06:58
lifelessso, test is done07:01
lifelesswhat do you mean by docs ?07:01
lifelessoh, I missed the class docstring update07:02
lifelessdone07:03
jtv(I feel it matters because you based the change on an incorrect reading of the existing behaviour)07:04
lifelessjtv: what do you mean?07:04
jtv"We could raise, but the existing handling for e.g. teams that are Persons is to return False, so I followed that style, for now"07:04
jtvThat is not actually true, as discussed above.07:04
lifelesserr07:05
lifelessit is true07:05
lifelessthere is a single special case07:05
lifelessinTeam(self)07:05
jtvThe existing handling for teams that are persons is to return True if self is a member, which can happen only if self == team.07:05
jtvThat is the "if self.id == team.id" check.  It returns True.07:06
jtvOnly if that check fails does the method give up for the case where not team.is_team.07:06
lifelessyes, and it can happen for person.inTeam(person) too07:06
lifelesswhich is inconsistent07:06
lifelesshowever thats useful for a bunch of places07:07
jtvperson.inTeam(person) returns True, regardless of whether person is actually a team or not.07:07
lifelessright07:07
lifelessbut all other person.inTeam(otherperson) return False07:07
jtvBecause they obviously would, regardless of whether that shortcut is there.07:08
jtvperson.inTeam(otherperson) returns True if person == otherperson or otherperson is a team that person participates in—I don't see anything in there that I'd expect to raise an exception.07:09
lifelessjtv: so, it also returns False if team is None07:09
lifelessaccording to the docstring07:09
jtvYes07:09
lifelessI have updated the docstring in the interface.07:09
lifelesshttp://pastebin.com/ZQuV4Ztq07:10
jtvlifeless: thanks.  Could you break up the sentence in the docstring?  Throwing parentheticals at it makes it harder to follow.07:13
lifelesssure07:13
jtvBTW we have our own pastebin: paste.ubuntu.com07:13
jtvI guess that isn't as full-featured though.07:14
jtv(But in return we also don't get ads :)07:15
* StevenK introduces lifeless to 'utilities/paste'07:16
lifeless        :param team: One of an object providing `IPerson`, the string name of a07:16
lifeless            team or `None`. If a string was supplied the team is looked up.07:17
lifeless        :return: A bool with the result of the membership lookup. When looking07:17
lifeless            up the team from a string finds nothing or team was `None` then07:17
lifeless            `False` is returned.07:17
lifelessStevenK: its nonpersistent and just adds noise to the world.07:17
lifelessStevenK: also, pastebinit.07:17
lifelessjtv: I use pastebin.com for transient stuff because it has a transient option07:17
lifelesspaste.ubuntu.com is *10 GB* of crud07:17
jtvlifeless: better, but it's sort of paraphrasing the code.  Why not just say something like "if no team of that name exists"?07:18
* jtv looks for the transient option…07:18
jtvThat's useful to know when I'm using a browser with ad blocking!07:19
jtv(Animated ads… <shudder>)07:19
lifelessjtv: uhm, paraphrasing code - sure07:19
lifelessOTOH the missing team and None cases are different spellings of the same thing, i wanted to group them.07:19
lifelessthe function is about 3 times as long as it should be07:20
jtvThat's fine with me—just no need to bring the _act_ of looking up the name into it AFAICS07:20
lifelessmmm07:20
jtvYes, we should never have allowed team names in the same lookup method as actual teams IMHO07:20
lifelessactually its relevant today.07:20
lifelessbecause its an extra query.07:20
lifelessits part of the cost you should know about if you read the doc and consider using the function07:20
jtvThat's a performance issue—wouldn't lump that in with the meaning of the parameter.07:20
lifelessits lumped in by the structure07:21
jtvYes, very unfortunate.  Frankly I'd love to fix this method, but if the performance specs are implicitly interwoven with the docstring that's just extra work.07:21
lifelessI'm not clear about what rephrasing you're suggesting, when I tried to apply it to my text it didn't want to fit in.07:21
lifelesswe've now spent more time reviewing than the entire patch took.07:22
lifelessso I'm going to say - thanks for the review, we've definitely made it better, but we need to stop.07:22
jtvOK07:22
jtvI'll approve it, and add the notes.07:23
lifelessif you feel up for another one, the second half of getting recipes onto lpnet is the other merge proposal I linked here.07:23
lifelessjtv: thank you07:24
=== StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvAnyone up for a big test-only review?  https://code.launchpad.net/~jtv/launchpad/test-pofile-permissions/+merge/4005808:33
jtvgmb, do you feel particularly sadistic today?  Here's your chance.  :)08:33
jtvallenap: are you free perhaps?08:49
allenapjtv: I am now :)08:52
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvallenap: \o/  :-)08:52
allenapStevenK: If you're still around I'll do your review first.08:56
StevenKallenap: Sure08:58
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: StevenK || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
adeuringallenap: could you please have a look at this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-4/+merge/39991 ?09:50
allenapadeuring: With pleasure :)09:51
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
adeuringallenap: thanks!09:51
jtvallenap: pretty boring stuff I guess :)10:31
allenapjtv: Nearly done :)10:32
jtvallenap: I'm happy for you :)10:32
allenapjtv: All done.10:43
jtvthanks allenap110:43
jtv!10:43
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
allenapArgh, more heavy unit test reviews!10:44
jtvallenap: btw, good question you ask there.  The closing is needed to show that [party] has certain rights _despite_ circumstances being against it.10:45
allenapjtv: Cool, I suspected something like that, but I wasn't sure.10:46
jtvallenap: I think I'll just work it into the closeTranslations docstring.10:46
=== mrevell is now known as mrevell-lunch
=== mrevell-lunch is now known as mrevell
=== fjlacoste is now known as flacoste
=== matsubara is now known as matsubara-lunch
EdwinGrubbsjcsackett: I found an error in your incremental diff. I added a comment to the mp, but I wanted to make sure you knew about this before you landed it, in case the tests didn't detect it.14:39
jcsackettEdwinGrubbs: yeah, actually everything blew up after that. :-P14:40
jcsackettso net good is we know for sure there are tests that catch that--i just didn't find the right tests last night.14:40
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== matsubara-lunch is now known as matsubara
=== deryck_ is now known as deryck
=== deryck_ is now known as deryck
=== rockstar changed the topic of #launchpad-reviews to: On call: allenap, rockstar || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha is now known as Ursinha-lunch
=== Guest63625 is now known as jelmer
=== benji is now known as benji-lunch
allenaprockstar: Are you up for a UI review? https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-2-devel/+merge/4011716:34
rockstarallenap, sure.16:35
allenaprockstar: Thanks :)16:35
allenaprockstar: Thanks for the review.17:24
=== benji-lunch is now known as benji
allenapleonardr: Can you help me land something to lazr.restfulclient? I've just reviewed https://code.launchpad.net/~gz/lazr.restfulclient/python_2.4_compatibility/+merge/25717 and I'm now trying to run tests but they're failing, http://paste.ubuntu.com/525803/17:40
leonardrallenap, sure17:41
allenapleonardr: I lost track of time, and I have to go. BTW, if you do the merge yourself, I've checked that the contributor has signed the agreement.17:46
leonardrok, cool17:46
leonardri'm guessing you just don't have lazr.restful, which is a test dependency but not a runtime dependency17:47
leonardri may just run the tests and merge it myself, if that's ok17:47
=== Ursinha-lunch is now known as Ursinha
abentleyrockstar: chat?18:18
rockstarabentley, I was just about to head out to lunch.  Can I grab you when I get back?18:18
abentleyrockstar: sure.18:19
=== salgado is now known as salgado-dr
=== benji changed the topic of #launchpad-reviews to: On call: allenap, rockstar || Reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjihi guys, I have a MP for review: https://code.launchpad.net/~benji/lazr.restful/bug-539070/+merge/4000318:56
benjithis is a second-round review it had a major flaw yesterday that I've now fixed18:57
rockstarbenji, it seems EdwinGrubbs has already claimed the review.19:11
benjirockstar: he did the first round yesterday, but since he's not reviewing today I figured I should give it to you guys.  Does it actually work the other way around?19:12
EdwinGrubbsbenji: I can finish it up.19:12
rockstarbenji, if he's already started the review, he should finish it, unless he defers to us.19:12
rockstarEdwinGrubbs, thanks.19:12
benjicool, I'll remember that for the future19:13
EdwinGrubbsbenji: is it a huge pain to test the WadlFieldAPI directly?19:33
EdwinGrubbsbenji: it looks like the last line of _entry_adapter_utility will cause an error, since I don't see `adapter` defined anywhere.19:34
EdwinGrubbsbenji: oh, nevermind, that line can never be reached. can it be removed?19:34
benjiEdwinGrubbs: yep it's a huge pain :)  I tried and decided I'd exhausted the reasonable amount of time to spend.19:36
benjiyou're right, that's some dead code that snuck in somehow, I'll remove that.19:36
EdwinGrubbsbenji: ok, r=me19:37
benjicool beans19:37
* benji wonders why the beans are cool, and why that signifies something positive.19:38
* benji now knows: http://en.wiktionary.org/wiki/cool_beans19:39
rockstarderyck, could you do a little review for me? https://code.launchpad.net/~rockstar/launchpad/lil-recipe-bugs/+merge/4014319:49
deryckrockstar, sure19:50
derycklooking now....19:50
deryckrockstar, "this the archive where the package" should be "this *is* the archive where the package" ?19:51
rockstarderyck, fixeded.19:51
deryckrockstar, r=me then19:51
rockstarThis is why we get reviews, even for little branches...19:52
deryckindeed :-)19:52
=== matsubara is now known as matsubara-afk
=== allenap changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
flacostegary_poster: i replied to your review21:36
gary_posterack flacoste will look and reply prob tomorrow morning thanks21:38
flacostegary_poster: thx21:41
gary_posterapproved flacoste, looks great.  g'night22:03
=== Ursinha is now known as Ursinha-afk

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