= ReviewerMeeting20100106 = == summary == * Maris to bring up cross-team reviews in the lazr-js task force meeting and report back. * Gavin to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc * Gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. * Curtis to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. == logs == === ameu === {{{ [15:00] #startmeeting [15:00] Meeting started at 09:00. The chair is bac. [15:00] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:00] welcome to the AMEU Reviewers meeting. [15:01] who's here today? [15:01] me [15:01] me [15:01] me [15:01] moo [15:01] me [15:02] EdwinGrubbs, deryck, gmb, BjornT: ping [15:02] me [15:02] me [15:02] Erk [15:02] me [15:02] gary_poster: ping [15:02] bac: thank you, me [15:03] team leads ping your peeps [15:03] done. May not get too many from my team. [15:03] me [15:04] i know we have a lot of people missing today due to sprints, so let's move on. [15:04] bac: jtv is sprinting [15:04] [topic] agenda [15:04] New Topic: agenda [15:04] yay, mootbot is not case sensitive! [15:04] heh [15:04] * Roll call [15:04] * Action items [15:04] * Limit doctests to 200 lines [allenap] [15:04] * Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary] [15:04] * Cleaning up ImportFascist warnings [bac,sinzui] [15:04] * Peanut gallery (anything not on the agenda) [15:04] me [15:04] me [15:05] allenap, (a nice trick to get pinged through agenda ;) [15:05] [topic] outstanding actions [15:05] New Topic: outstanding actions [15:05] [topic] * intellectronica to draft guidelines for drive-by cleanups [15:05] New Topic: * intellectronica to draft guidelines for drive-by cleanups [15:06] sorry, haven't done that. please keep it on the list and i'll get it for our next meeting [15:06] intellectronica: excellent, thanks! [15:06] [topic] invite other teams to do lazr-js code reviews? (See if this was sent to the ML.) [mars/bac] [15:06] New Topic: invite other teams to do lazr-js code reviews? (See if this was sent to the ML.) [mars/bac] [15:06] i searched through the ML's and did not see it had been discussed [15:06] mars did you have some thoughts on the topic? [15:07] bac, I'm going to add it to the agenda for the lazr-js taskforce call tomorrow [15:07] mars: ok, so you can report back here next week. [15:07] bac, you can keep it on the list here if you would like to see a wrapup item next week [15:07] [action] mars to report on outcome of lazr-js review discussion [15:07] ACTION received: mars to report on outcome of lazr-js review discussion [15:07] yeah :) [15:07] great, moving on to new items [15:08] [topic] Limit doctests to 200 lines [allenap] [15:08] New Topic: Limit doctests to 200 lines [allenap] [15:08] gavin, take it away [15:09] Right, [15:09] Because of the accumulation of state in doctests [15:09] I think it would be good to limit new doctests to 200 lines or less. [15:10] gmb wrote a new doctest last week of just under 200 lines and it was still perfectly comprehensible. [15:10] So that's where the number comes from. [15:10] * gmb notes that it got changed into a unittest anyway during the review process... [15:10] I don't think that'd be right for large classes and corresponding doctests, 200 sounds as artificially low limit [15:11] Although manuel (?) adds a reset-globs directive whihc might make this moot. [15:11] and there is an alternative to the state problem [15:11] perhaps large classes are the problem, but we do have them [15:11] exactly, reset-globs in manuel [15:11] agree with danilos. Also, agree with allenap's observation of manuel [15:11] allenap: I have thought about breaking many registry doctests into smaller themes [15:11] manuel also adds the option of running specific section of a doctest [15:11] allenap: but my motivation it to control the layer the test is run on. [15:12] i think limiting the length is not the best way to handle this. instead i suggest dividing tests thematically [15:12] I agree, I don't see the problem as much with larger narratives as it is with naughty test code state. [15:12] sinzui, I think the problem with existing doctests is that we don't have a good structure of tests in general (oh, I'd like to clean most of translations ones as well) [15:12] that is, stop and start a new file when you're really testing something new (with new state and so on) [15:12] intellectronica: i agree [15:13] The main problem I have is trying to understand the object and database state by the end of the test. That can't really be fixed by reset-globs or manuel as I understand it. [15:13] intellectronica, +1, basic doctests (those which are usually large) should be basic class documentation; specific tests should be unit test [15:13] intellectronica: The limit is an incentive to split :) [15:14] intellectronica: But I agree. [15:14] allenap, I see the point, but artificial limit would have to be broken in so many valid cases [15:14] allenap: yes, i understand that, but somehow it feels a bit too artificial to me. are you concerned that without a hard limit developers/reviewers won't bother? [15:14] allenap: it is a good point but i'd rather see us have smaller, more themed tests as a guide rather than adopting a limit [15:14] We do have a "800 line limit for reviews" that is a guideline but often breached [15:15] also, i'll play sinzui and ask: who's going to split all the existing test? ;) [15:15] how about instead spreading a rule "enforce corner-case testing through unit tests" among reviewers instead? those tend to make doctests harder to read [15:15] instead of a hard limit, can we call it a "refactor point"? As in, over X lines, the reviewer should look for a possible refactoring [15:15] Okay, sounds good. Is there a way to measure or have more concrete guidelines, or is it at developer/reviewer discretion? [15:15] it's ok if they do not find such a point [15:15] kind of like review lint [15:16] allenap: there is a measure. variables shouldn't be recycled [15:16] mars, I still believe that those complex doctests should really be turned into unittests, and that's exactly where you can't reuse variables :) [15:16] intellectronica: I don't think we should split the existing tests... until it is no longer possible to not split them. [15:16] intellectronica: I like that :) [15:16] intellectronica: There's still a problem of db state. [15:16] danilos, well, if they are over 800 lines, then you would recommend the refactoring, wouldn't you? :) [15:17] allenap: if we adopt this as a policy we will eventually have to split the tests, if only so that they don't serve as a negative example for new contributors [15:17] danilos: +1 [15:17] danilos, or 400, or whatever [15:17] intellectronica: One line that creates a file or email message requires that the test be run on a different layer. In many cases the test is actually moving to a new theme and I think it is easier to read the that aspect of the test separately. [15:17] intellectronica: Good point. [15:17] allenap: i guess it's the same for db state. don't rely on db state unless it's either set in the sample data or in the prologue to your test [15:18] mars, well, reviewers should always look for points of refactoring, so I just feel such "rule" wouldn't really make any difference, but sure :) [15:18] intellectronica: I agree, and I don't know of a way to make it less vague than that. I suppose that's what I was looking for. [15:18] I find the stories that doctests tell valuable. The stories are often long--and in fact, I often wish they were *better* connected, not less, from a narrative standpoint. That viewpoint would make me want to see manuel a preferred option, or perhaps Sphinx involved for tying together story chunks, but that is maybe herder. [15:18] sinzui: so what you're saying is that we should split tests if we're required to run the test in a lower layer because of a change in the middle of the test? [15:18] mars, even when the diff you are looking at is 10 lines, as a reviewer, you should wonder if the code is sitting in the right place [15:19] and means that it doesn't help for reading text files, only processed files [15:19] danilos, well, I don't think of it as a rule, just a recommendation for reviewers. Kind of a warning, or code advice. [15:19] s/herder/harder/ [15:19] danilos, right [15:19] anyway, it seems this is a hot topic [15:20] allenap, bac, shall we move it to the mailing list or try to come up with a conclusion here? [15:20] I think a lot of people talking about dividing things up are coming from the perspective of (1) expertise and (2) testing. [15:20] intellectronica: yes. In the exampled I am thinking about, registiry object deletion, email notification, these themes are different from the core uses of the object. [15:20] danilos: Mailing list I think. There are many threads of conversation and this isn't going to end here anyway ;) [15:21] they are valuable, but these are narratives. [15:21] sinzui: yes, i think that's a good guideline [15:21] allenap, right, I think the general consensus is that we don't want 200 as the limit, so it's best to discuss other solutions to the problem you observe on list [15:21] danilos: yes, let's move the discussion to the mailing list. allenap will you start the discussion there? [15:21] bac: Sure. [15:21] gary_poster, good point [15:21] :-) thank you mars [15:21] [action] allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc [15:21] ACTION received: allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc [15:22] [topic] Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary] [15:22] New Topic: Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary] [15:22] OK I almost have the conversation pastable but not quite. [15:22] I was talking with EdwinGrubbs about a security check. He was checking whether a user had a specific permission on an object. He was using a utility in the LP tree that I forget. [15:22] I said that my top preference would be try:...except UnauthorizedError:... and my next preference would be canAccess and canWrite from zope.security.checker. [15:22] canView is nicer because you do not specify the permission, which is fragile. [15:22] The try/except approach uses the basic security machinery directly. It is of course the basic mechanism that Zope provides. [15:22] Edwin pointed out that this was not a pattern in the LP tree. He was also afraid it would be slower. [15:22] I'm pretty sure it will not be slower than calling a Python function, but can verify with a timeit test. [15:23] So, I suppose I have these thoughts: [15:23] 1) I'd like to make sure that we generally agree that we don't want to specify permissions unless we have to [15:23] (that is, my main suggestion from the story above) [15:24] 2) I'd like people to consider using the try/except approach, and for it to be a reasonable/approved one. If we want me to construct a timeit test to verify I can. [15:24] That's it. [15:25] s/conversation pastable/introduction pastable/ ;-) [15:25] * danilos reads [15:25] gary_poster: i also think that using try/except is clearer [15:25] cool [15:26] i think a lot of us have heard the "wisdom" that try/except is slower. i think a timeit test to disprove that would be useful. [15:26] I know we are using checkPermission for these things [15:26] EdwinGrubbs: are you around to say remind me what the utility was that you used at first? [15:26] Maybe it is checkPermission [15:26] yes, it was check_permission() [15:27] from canonical.launchpad.webapp.authorization import check_permission [15:27] ah ok, looking [15:27] bac, +1 [15:27] it's a wrapper around checkPermission [15:27] bac: why is performance an issue here? usually you don't call these in loops [15:27] flacoste: oh ok [15:27] example: check_permission('launchpad.Edit', pofile) [15:27] and i think actually the try: except: is what is used under the hood by check_permission anyway [15:27] you do call them in loops if you are displaying a list of objects [15:27] so generally, I don'y think we should use check_permission (or checkPermission) again, if at all possible. That's my #1, [15:28] canAccess and canWrite are less fragile [15:28] I'm happy to do the timeit check. I still think that try/except will be faster than a Python function, but I think that's interesting information, so I can do that and report back to the list. [15:28] gary_poster: Is this in view or in model code? [15:29] view code [15:29] intellectronica: it may be in a loop. if some timing tests can show it is a myth then i think that's useful. [15:29] good [15:30] bac: knowing is definitely useful, but i think that even if we find that it is slower, we should still consider using try/except in cases where it doesn't affect performance _drastically_ [15:30] hmm, right [15:30] canAcess / canWrite uses try: except [15:30] whereas checkPermission uses the checker information [15:30] intellectronica: well, a lot of these cases are in vocabularies, IIRC. many of those do have performance issues [15:31] bac: yes, where we do it iteratively we definitely should optimize for performance [15:32] the reason I don't like try...except is that (especially with API), we will get a lot of cases where we'll have multiple conditions we are checking for [15:32] so, nested try..excepts sound ugly to me [15:32] If try:except is faster, as I suspect, then we do not have to have the theoretical discussion. suggested resolution for now: I'll do the timeit test and report back to the list. If there is a clear answer in favor of try/except for performance as well, then that's easy. ...except for danilo's case... [15:33] danilo, can you send me an example to look at? [15:33] gary_poster, I am not even sure it's a valid example, but there are cases where we do an OR of multiple permission checks [15:34] danilos: right, I'd like to see if it is valid; if it is not, we should know what a preferred spelling us [15:34] but that's probably just bad design where objects are not properly security-wrapped [15:34] [action] gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. [15:34] is [15:34] ACTION received: gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. [15:34] cool thanks [15:34] thanks for bringing it up gary [15:35] [topic] Cleaning up ImportFascist warnings [bac,sinzui] [15:35] New Topic: Cleaning up ImportFascist warnings [bac,sinzui] [15:35] jml's fix to the import fascist revealed lots of places where we have problems [15:36] we need to drive those warnings back to zero or we'll continue to expand the problem [15:36] gary_poster, sure, why don't you ping me later and we can take a look (basically, lp.translations.browser.distroseries.DistroSeriesLanguagePackView has an example) [15:36] sinzui has a branch that accomplishes a lot of it but has reported new code is introducing more problems [15:36] sinzui where did you find the bulk of the offenders to be? [15:36] danilos: awesome thank you [15:37] Most were explicit import from _schema_circular_imports which were not needed [15:37] They were easy to remove [15:37] sinzui: does your branch solve the problem for LP or just registry? [15:37] LP, but there are two new ones in the last 12 hours [15:38] ah, cool. so once this branch lands we can go back to zero tolerance as reviewers, no? [15:38] BranchJobDerived BranchJobType are not in BranchJob.__all__, but are imported by translationtemplatesbuildjob and translationtemplatesbuildjob [15:38] should we make them errors? [15:39] I thought that was an option [15:39] I could add these two classes to __all__, but I think the code team and translations team need to coordinate [15:39] sinzui, if it has happened during last 12h, it's something that's happening during a sprint in Wellington [15:40] correct [15:40] sinzui, and I don't really overlap with them, so I'd appreciate if you mention it to jtv when he shows up and I am sure he'll be happy to fix the issue or find someone who will :) [15:40] I will [15:40] sinzui, thanks [15:41] so we're in agreement to get this cleaned up over the next week and then keep it under control? [15:41] sinzui, and thanks for spending the effort to fix it everywhere else as well, I hope we didn't have many issues in translations since we shouldn't be able to import stuff if we do (because we are not using global imports either) [15:41] bac, +1 [15:42] [action] sinzui to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. [15:42] ACTION received: sinzui to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. [15:42] that's it for agenda items [15:42] sinzui, bac: just to confirm, this will also be part of lint checks? [15:42] [topic] peanut gallery [15:42] New Topic: peanut gallery [15:42] landing is challenging since we are in phantom testfix [15:42] (i.e. easy to spot) [15:43] sinzui, js buildbot? [15:43] danilos: i'm not sure. sinzuie? [15:43] er, sinzui? [15:43] danilos: importfacist has never been a part of lint since the testrunner has alwyas reported them [15:43] thanks [15:43] danilos: I thought I saw rockstar land a reversion to get us out of testfix [15:44] sinzui: it's not phantom [15:44] db_lp has three failures [15:44] moving on, does anyone have an issue that is not on the agenda? [15:44] related to code hosting [15:44] i have [15:44] flacoste: go [15:44] but it's not related to reviewers :-) [15:44] hmm [15:44] but would like the opportunity that we have a lot of attention :-) [15:44] we need a volunteer to fix the db_lp failures [15:44] flacoste: ok, reminding you we're at 45 minutes [15:45] that's all [15:46] bac: you can cloase the meeting [15:46] flacoste: let's try to resolve that outside the meeting [15:46] #endmeeting [15:46] Meeting finished at 09:46. }}} === asiapac === No meeting due to Wellington Sprint.