| 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:02 |
| thumper | especially since most of that change is just whitespace changes in configure.zcml | 04:03 |
| thumper | mwhudson: you perhaps? | 04:04 |
| * thumper needs to head off | 04:05 | |
| thumper | mwhudson: not urgent | 04:05 |
| mwhudson | i can have a look | 04:07 |
| lifeless | I can has review ? https://code.launchpad.net/~lifeless/launchpad/features/+merge/40049 | 05:25 |
| 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:23 |
| 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:24 |
| lifeless | yes | 06:26 |
| lifeless | thats the existing behaviour | 06: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 |
| lifeless | person.py line 1202 | 06:27 |
| jtv | lifeless: ah I see—person.inTeam(person) will return True, but as a special case that is handled just before this check. | 06:28 |
| 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:29 |
| jtv | thanks | 06:30 |
| jtv | lifeless: where is the rest of the scopes tested? | 06:30 |
| lifeless | 'are' | 06:30 |
| 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:31 |
| lifeless | adding a flag allows the use of all scopes to configure its value | 06:32 |
| jtv | I'd expect them to be relatively orthogonal..? | 06:33 |
| jtv | The scopes and the features, I mean? | 06:33 |
| lifeless | right | 06:33 |
| 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:35 |
| 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:37 |
| 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:39 |
| lifeless | https://code.launchpad.net/~lifeless/launchpad/recipes/+merge/40050 | 06:40 |
| 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:42 |
| 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: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.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:44 |
| 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:47 |
| 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:48 |
| 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:49 |
| 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:50 |
| 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:51 |
| 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:52 |
| 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:53 |
| 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:54 |
| 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:55 |
| 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:56 |
| 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:57 |
| 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. | 06:58 |
| lifeless | so, test is done | 07:01 |
| lifeless | what do you mean by docs ? | 07:01 |
| lifeless | oh, I missed the class docstring update | 07:02 |
| lifeless | done | 07:03 |
| 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:04 |
| 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:05 |
| 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:06 |
| 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:07 |
| jtv | Because they obviously would, regardless of whether that shortcut is there. | 07:08 |
| 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:09 |
| lifeless | http://pastebin.com/ZQuV4Ztq | 07:10 |
| 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:13 |
| jtv | I 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 a | 07: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 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:17 |
| 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:18 | |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| lifeless | jtv: thank you | 07: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 | ||
| 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:33 |
| jtv | allenap: are you free perhaps? | 08:49 |
| allenap | jtv: 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 | ||
| jtv | allenap: \o/ :-) | 08:52 |
| allenap | StevenK: If you're still around I'll do your review first. | 08:56 |
| StevenK | allenap: Sure | 08: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 | ||
| 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:50 |
| allenap | adeuring: 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 | ||
| adeuring | allenap: thanks! | 09:51 |
| jtv | allenap: pretty boring stuff I guess :) | 10:31 |
| allenap | jtv: Nearly done :) | 10:32 |
| jtv | allenap: I'm happy for you :) | 10:32 |
| allenap | jtv: All done. | 10:43 |
| jtv | thanks allenap1 | 10: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 | ||
| allenap | Argh, more heavy unit test reviews! | 10:44 |
| 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:45 |
| 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. | 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 | ||
| 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:39 |
| 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. | 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 | ||
| 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:34 |
| rockstar | allenap, sure. | 16:35 |
| allenap | rockstar: Thanks :) | 16:35 |
| allenap | rockstar: Thanks for the review. | 17:24 |
| === benji-lunch is now known as benji | ||
| 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:40 |
| leonardr | allenap, sure | 17:41 |
| 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:46 |
| 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 | 17:47 |
| === Ursinha-lunch is now known as Ursinha | ||
| 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:18 |
| abentley | rockstar: 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 | ||
| benji | hi guys, I have a MP for review: https://code.launchpad.net/~benji/lazr.restful/bug-539070/+merge/40003 | 18:56 |
| benji | this is a second-round review it had a major flaw yesterday that I've now fixed | 18:57 |
| rockstar | benji, it seems EdwinGrubbs has already claimed the review. | 19:11 |
| 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:12 |
| benji | cool, I'll remember that for the future | 19:13 |
| EdwinGrubbs | benji: is it a huge pain to test the WadlFieldAPI directly? | 19:33 |
| 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:34 |
| 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:36 |
| EdwinGrubbs | benji: ok, r=me | 19:37 |
| benji | cool beans | 19: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_beans | 19:39 | |
| rockstar | deryck, could you do a little review for me? https://code.launchpad.net/~rockstar/launchpad/lil-recipe-bugs/+merge/40143 | 19:49 |
| deryck | rockstar, sure | 19:50 |
| deryck | looking now.... | 19:50 |
| 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:51 |
| rockstar | This is why we get reviews, even for little branches... | 19:52 |
| deryck | indeed :-) | 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 | ||
| flacoste | gary_poster: i replied to your review | 21:36 |
| gary_poster | ack flacoste will look and reply prob tomorrow morning thanks | 21:38 |
| flacoste | gary_poster: thx | 21:41 |
| gary_poster | approved flacoste, looks great. g'night | 22:03 |
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!