[04:02] <thumper> https://code.launchpad.net/~thumper/launchpad/blueprint-update-lifecycle-status-with-event/+merge/40048
[04:02] <thumper> slightly bigger than I expected
[04:02] <thumper> but < 800
[04:03] <thumper> especially since most of that change is just whitespace changes in configure.zcml
[04:04] <thumper> mwhudson: you perhaps?
[04:05]  * thumper needs to head off
[04:05] <thumper> mwhudson: not urgent
[04:07] <mwhudson> i can have a look
[05:25] <lifeless> I can has review ? https://code.launchpad.net/~lifeless/launchpad/features/+merge/40049
[06:23] <lifeless> jtv: around ?
[06:23] <jtv> lifeless: yes
[06:23] <lifeless> https://code.launchpad.net/~lifeless/launchpad/features/+merge/40049
[06:23] <lifeless> if you can spare the time
[06:24] <lifeless> I've used a too high layer, will fix that
[06:24] <jtv> lifeless: looking
[06:24] <lifeless> (As in, I think DatabaseFunctionalLayer will work, on reflection)
[06:24] <jtv> lifeless: the cover letter suggests that for a non-team person, person.inTeam(person) will fail.  Is that correct?
[06:24] <jtv> Sorry, not fail; return False.
[06:26] <lifeless> yes
[06:26] <lifeless> thats the existing behaviour
[06:27] <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] <lifeless> person.py line 1202
[06:28] <jtv> lifeless: ah I see—person.inTeam(person) will return True, but as a special case that is handled just before this check.
[06:29] <jtv> lifeless: stupid question perhaps, but what are these scopes you mention?
[06:29] <lifeless> https://dev.launchpad.net/LEP/FeatureFlags
[06:29] <lifeless> end of the page
[06:30] <jtv> thanks
[06:30] <jtv> lifeless: where is the rest of the scopes tested?
[06:30] <lifeless> 'are'
[06:31] <lifeless> same source file for the ones I've added.
[06:31] <lifeless> Not all scopes are tested rigorously
[06:31] <lifeless> adding a scope adds custom configuration facilities to all flags
[06:32] <lifeless> adding a flag allows the use of all scopes to configure its value
[06:33] <jtv> I'd expect them to be relatively orthogonal..?
[06:33] <jtv> The scopes and the features, I mean?
[06:33] <lifeless> right
[06:35] <jtv> lifeless: 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:37] <lifeless> I don't actually, it has sufficiently high noise that I just eyeball things.
[06:37] <lifeless> I've filed bugs on the errata.
[06:39] <jtv> I 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] <lifeless> sec, I'm just pushing a new branch
[06:39] <lifeless> I'll context switch and look with you
[06:40] <lifeless> https://code.launchpad.net/~lifeless/launchpad/recipes/+merge/40050
[06:42] <lifeless> jtv: your editor is wrong about the import of layers being redundant, AFAICT
[06:42] <lifeless> jtv: if you mean in test_webapp.py
[06:42] <jtv> yes
[06:42] <jtv> I don't find the word "layers" anywhere else in that file.
[06:43] <jtv> Ah, 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 +0000
[06:43] <lifeless> +++ lib/lp/services/features/tests/test_webapp.py       2010-11-04 06:40:55 +0000
[06: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:44] <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.LaunchpadFunctionalLayer
[06:44] <lifeless> +    layer = layers.DatabaseFunctionalLayer
[06:44] <lifeless> anyhow - 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] <lifeless> bah, pastefail :)
[06:47] <jtv> lifeless: 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:48] <lifeless> jtv: so, its a risk, but is it a significant one?
[06:48] <jtv> I get the feeling that it's more of an interpretation thing that you want specifically in this use-case.
[06:48] <lifeless> jtv: inTeam(team_object) will be reliable.
[06:48] <jtv> Maybe we should ask someone dyslexic.  :-)
[06:48] <lifeless> jtv: so the only place that this is a new risk is where we have a /string/ reference to a team object
[06:48] <lifeless> and are doing a membership check
[06:49] <lifeless> jtv: I think that that will only be in places like this
[06:49] <jtv> Yes, it's the sort of thing you might do in tests I guess.
[06:49] <lifeless> where we'd want to assume that the string data is potentially stale and just answer 'no'
[06:50] <jtv> If 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:51] <lifeless> jtv: thats true, OTOH no code does it today - I hope :)
[06:51] <lifeless> jtv: I'd rather iterate on this if someone wants to do that, rather than polish an interface thats not really desirable anywahy
[06:51] <jtv> If you hit this mistake today, you'd get an error.
[06:52] <lifeless> and 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:53] <jtv> (One problem with bools is it's so damned easy to hit the right value by mistake)
[06:53] <lifeless> grep -n "inTeam('"
[06:53]  * jtv was doing the same
[06:53] <lifeless> so I think I'm fixing a latent bug
[06:54] <lifeless> if someone deletes or renames soyuz-team
[06:54] <lifeless> lp will start blowing up
[06:54] <lifeless> yet returning False would be appropriate in the existing uses.
[06:54] <jtv> Which seems a lot more appropriate than people being locked out for reasons they can't figure out.
[06:55] <lifeless> inTeam is a query interface.
[06:55] <lifeless> its not an assertion interface.
[06:55] <lifeless> I think you're arguing for behaviour appropriate in an assertion/predicate interface.
[06:55] <lifeless> I'd be delighted to raise an exception in such an interface.
[06:56] <jtv> Actually I think the interface itself is somewhat misplaced.
[06:56] <lifeless> sure
[06:56] <jtv> It's so fundamental and low-level that you don't expect a lot of intelligence in there.
[06:56] <jtv> I mistrust "smart" code.
[06:56] <lifeless> I just don't see the benefit of an exception here when there are three users and none of them want an exception.
[06:56] <lifeless> YAGNI
[06:57] <jtv> Well 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] <jtv> We'd only know we had a need if we wasted a lot of time debugging.
[06:57] <lifeless> jtv: sure we would, someone would have added a test encoding the behaviour they wanted.
[06:58] <lifeless> I should, in fact, add a test for the change I made there.
[06:58] <lifeless> the implicit test from this code path isn't really sufficient
[06:58] <jtv> That was next on my list, yes.  :)
[06:58] <jtv> After that comes documentation.
[07:01] <lifeless> so, test is done
[07:01] <lifeless> what do you mean by docs ?
[07:02] <lifeless> oh, I missed the class docstring update
[07:03] <lifeless> done
[07:04] <jtv> (I feel it matters because you based the change on an incorrect reading of the existing behaviour)
[07:04] <lifeless> jtv: 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] <jtv> That is not actually true, as discussed above.
[07:05] <lifeless> err
[07:05] <lifeless> it is true
[07:05] <lifeless> there is a single special case
[07:05] <lifeless> inTeam(self)
[07:05] <jtv> The existing handling for teams that are persons is to return True if self is a member, which can happen only if self == team.
[07:06] <jtv> That is the "if self.id == team.id" check.  It returns True.
[07:06] <jtv> Only if that check fails does the method give up for the case where not team.is_team.
[07:06] <lifeless> yes, and it can happen for person.inTeam(person) too
[07:06] <lifeless> which is inconsistent
[07:07] <lifeless> however thats useful for a bunch of places
[07:07] <jtv> person.inTeam(person) returns True, regardless of whether person is actually a team or not.
[07:07] <lifeless> right
[07:07] <lifeless> but all other person.inTeam(otherperson) return False
[07:08] <jtv> Because they obviously would, regardless of whether that shortcut is there.
[07:09] <jtv> person.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] <lifeless> jtv: so, it also returns False if team is None
[07:09] <lifeless> according to the docstring
[07:09] <jtv> Yes
[07:09] <lifeless> I have updated the docstring in the interface.
[07:10] <lifeless> http://pastebin.com/ZQuV4Ztq
[07:13] <jtv> lifeless: thanks.  Could you break up the sentence in the docstring?  Throwing parentheticals at it makes it harder to follow.
[07:13] <lifeless> sure
[07:13] <jtv> BTW we have our own pastebin: paste.ubuntu.com
[07:14] <jtv> I guess that isn't as full-featured though.
[07:15] <jtv> (But in return we also don't get ads :)
[07:16]  * StevenK introduces lifeless to 'utilities/paste'
[07:16] <lifeless>         :param team: One of an object providing `IPerson`, the string name of a
[07:17] <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 looking
[07:17] <lifeless>             up the team from a string finds nothing or team was `None` then
[07:17] <lifeless>             `False` is returned.
[07:17] <lifeless> StevenK: its nonpersistent and just adds noise to the world.
[07:17] <lifeless> StevenK: also, pastebinit.
[07:17] <lifeless> jtv: I use pastebin.com for transient stuff because it has a transient option
[07:17] <lifeless> paste.ubuntu.com is *10 GB* of crud
[07:18] <jtv> lifeless: 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:19] <jtv> That's useful to know when I'm using a browser with ad blocking!
[07:19] <jtv> (Animated ads… <shudder>)
[07:19] <lifeless> jtv: uhm, paraphrasing code - sure
[07:19] <lifeless> OTOH the missing team and None cases are different spellings of the same thing, i wanted to group them.
[07:20] <lifeless> the function is about 3 times as long as it should be
[07:20] <jtv> That's fine with me—just no need to bring the _act_ of looking up the name into it AFAICS
[07:20] <lifeless> mmm
[07:20] <jtv> Yes, we should never have allowed team names in the same lookup method as actual teams IMHO
[07:20] <lifeless> actually its relevant today.
[07:20] <lifeless> because its an extra query.
[07:20] <lifeless> its part of the cost you should know about if you read the doc and consider using the function
[07:20] <jtv> That's a performance issue—wouldn't lump that in with the meaning of the parameter.
[07:21] <lifeless> its lumped in by the structure
[07:21] <jtv> Yes, 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] <lifeless> I'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:22] <lifeless> we've now spent more time reviewing than the entire patch took.
[07:22] <lifeless> so I'm going to say - thanks for the review, we've definitely made it better, but we need to stop.
[07:22] <jtv> OK
[07:23] <jtv> I'll approve it, and add the notes.
[07:23] <lifeless> if you feel up for another one, the second half of getting recipes onto lpnet is the other merge proposal I linked here.
[07:24] <lifeless> jtv: thank you
[08:33] <jtv> Anyone up for a big test-only review?  https://code.launchpad.net/~jtv/launchpad/test-pofile-permissions/+merge/40058
[08:33] <jtv> gmb, do you feel particularly sadistic today?  Here's your chance.  :)
[08:49] <jtv> allenap: are you free perhaps?
[08:52] <allenap> jtv: I am now :)
[08:52] <jtv> allenap: \o/  :-)
[08:56] <allenap> StevenK: If you're still around I'll do your review first.
[08:58] <StevenK> allenap: Sure
[09:50] <adeuring> allenap: 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:51] <allenap> adeuring: With pleasure :)
[09:51] <adeuring> allenap: thanks!
[10:31] <jtv> allenap: pretty boring stuff I guess :)
[10:32] <allenap> jtv: Nearly done :)
[10:32] <jtv> allenap: I'm happy for you :)
[10:43] <allenap> jtv: All done.
[10:43] <jtv> thanks allenap1
[10:43] <jtv> !
[10:44] <allenap> Argh, more heavy unit test reviews!
[10:45] <jtv> allenap: btw, good question you ask there.  The closing is needed to show that [party] has certain rights _despite_ circumstances being against it.
[10:46] <allenap> jtv: Cool, I suspected something like that, but I wasn't sure.
[10:46] <jtv> allenap: I think I'll just work it into the closeTranslations docstring.
[14:39] <EdwinGrubbs> jcsackett: 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:40] <jcsackett> EdwinGrubbs: yeah, actually everything blew up after that. :-P
[14:40] <jcsackett> so net good is we know for sure there are tests that catch that--i just didn't find the right tests last night.
[16:34] <allenap> rockstar: Are you up for a UI review? https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-2-devel/+merge/40117
[16:35] <rockstar> allenap, sure.
[16:35] <allenap> rockstar: Thanks :)
[17:24] <allenap> rockstar: Thanks for the review.
[17:40] <allenap> leonardr: 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:41] <leonardr> allenap, sure
[17:46] <allenap> leonardr: 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] <leonardr> ok, cool
[17:47] <leonardr> i'm guessing you just don't have lazr.restful, which is a test dependency but not a runtime dependency
[17:47] <leonardr> i may just run the tests and merge it myself, if that's ok
[18:18] <abentley> rockstar: chat?
[18:18] <rockstar> abentley, I was just about to head out to lunch.  Can I grab you when I get back?
[18:19] <abentley> rockstar: sure.
[18:56] <benji> hi guys, I have a MP for review: https://code.launchpad.net/~benji/lazr.restful/bug-539070/+merge/40003
[18:57] <benji> this is a second-round review it had a major flaw yesterday that I've now fixed
[19:11] <rockstar> benji, it seems EdwinGrubbs has already claimed the review.
[19:12] <benji> rockstar: 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] <EdwinGrubbs> benji: I can finish it up.
[19:12] <rockstar> benji, if he's already started the review, he should finish it, unless he defers to us.
[19:12] <rockstar> EdwinGrubbs, thanks.
[19:13] <benji> cool, I'll remember that for the future
[19:33] <EdwinGrubbs> benji: is it a huge pain to test the WadlFieldAPI directly?
[19:34] <EdwinGrubbs> benji: it looks like the last line of _entry_adapter_utility will cause an error, since I don't see `adapter` defined anywhere.
[19:34] <EdwinGrubbs> benji: oh, nevermind, that line can never be reached. can it be removed?
[19:36] <benji> EdwinGrubbs: yep it's a huge pain :)  I tried and decided I'd exhausted the reasonable amount of time to spend.
[19:36] <benji> you're right, that's some dead code that snuck in somehow, I'll remove that.
[19:37] <EdwinGrubbs> benji: ok, r=me
[19:37] <benji> cool beans
[19:38]  * benji wonders why the beans are cool, and why that signifies something positive.
[19:39]  * benji now knows: http://en.wiktionary.org/wiki/cool_beans
[19:49] <rockstar> deryck, could you do a little review for me? https://code.launchpad.net/~rockstar/launchpad/lil-recipe-bugs/+merge/40143
[19:50] <deryck> rockstar, sure
[19:50] <deryck> looking now....
[19:51] <deryck> rockstar, "this the archive where the package" should be "this *is* the archive where the package" ?
[19:51] <rockstar> deryck, fixeded.
[19:51] <deryck> rockstar, r=me then
[19:52] <rockstar> This is why we get reviews, even for little branches...
[19:52] <deryck> indeed :-)
[21:36] <flacoste> gary_poster: i replied to your review
[21:38] <gary_poster> ack flacoste will look and reply prob tomorrow morning thanks
[21:41] <flacoste> gary_poster: thx
[22:03] <gary_poster> approved flacoste, looks great.  g'night