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