launchpad-dev team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

= 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] <bac> #startmeeting
[15:00] <MootBot> Meeting started at 09:00. The chair is bac.
[15:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[15:00] <bac> welcome to the AMEU Reviewers meeting.
[15:01] <bac> who's here today?
[15:01] <flacoste> me
[15:01] <sinzui> me
[15:01] <henninge> me
[15:01] <mars> moo
[15:01] <adeuring> me
[15:02] <bac> EdwinGrubbs, deryck, gmb, BjornT: ping
[15:02] <EdwinGrubbs> me
[15:02] <gmb> me
[15:02] <gmb> Erk
[15:02] <deryck> me
[15:02] <bac> gary_poster: ping
[15:02] <gary_poster> bac: thank you, me
[15:03] <bac> team leads ping your peeps
[15:03] <gary_poster> done.  May not get too many from my team.
[15:03] <danilos> me
[15:04] <bac> i know we have a lot of people missing today due to sprints, so let's move on.
[15:04] <henninge> bac: jtv is sprinting
[15:04] <bac> [topic] agenda
[15:04] <MootBot> New Topic:  agenda
[15:04] <bac> yay, mootbot is not case sensitive!
[15:04] <gary_poster> heh
[15:04] <bac>  * Roll call
[15:04] <bac>  * Action items
[15:04] <bac>  * Limit doctests to 200 lines [allenap]
[15:04] <bac>  * Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary]
[15:04] <bac>  * Cleaning up ImportFascist warnings [bac,sinzui]
[15:04] <bac>  * Peanut gallery (anything not on the agenda)
[15:04] <allenap> me
[15:04] <intellectronica> me
[15:05] <danilos> allenap, (a nice trick to get pinged through agenda ;)
[15:05] <bac> [topic] outstanding actions
[15:05] <MootBot> New Topic:  outstanding actions
[15:05] <bac> [topic] * intellectronica to draft guidelines for drive-by cleanups
[15:05] <MootBot> New Topic:  * intellectronica to draft guidelines for drive-by cleanups
[15:06] <intellectronica> sorry, haven't done that. please keep it on the list and i'll get it for our next meeting
[15:06] <bac> intellectronica: excellent, thanks!
[15:06] <bac> [topic] invite other teams to do lazr-js code reviews? (See if this was sent to the ML.) [mars/bac]
[15:06] <MootBot> New Topic:  invite other teams to do lazr-js code reviews? (See if this was sent to the ML.) [mars/bac]
[15:06] <bac> i searched through the ML's and did not see it had been discussed
[15:06] <bac> mars did you have some thoughts on the topic?
[15:07] <mars> bac, I'm going to add it to the agenda for the lazr-js taskforce call tomorrow
[15:07] <bac> mars: ok, so you can report back here next week.
[15:07] <mars> bac, you can keep it on the list here if you would like to see a wrapup item next week
[15:07] <bac> [action] mars to report on outcome of lazr-js review discussion
[15:07] <MootBot> ACTION received:  mars to report on outcome of lazr-js review discussion
[15:07] <mars> yeah :)
[15:07] <bac> great, moving on to new items
[15:08] <bac> [topic] Limit doctests to 200 lines [allenap]
[15:08] <MootBot> New Topic:  Limit doctests to 200 lines [allenap]
[15:08] <bac> gavin, take it away
[15:09] <allenap> Right,
[15:09] <allenap> Because of the accumulation of state in doctests
[15:09] <allenap> I think it would be good to limit new doctests to 200 lines or less.
[15:10] <allenap> gmb wrote a new doctest last week of just under 200 lines and it was still perfectly comprehensible.
[15:10] <allenap> 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] <danilos> I don't think that'd be right for large classes and corresponding doctests, 200 sounds as artificially low limit
[15:11] <allenap> Although manuel (?) adds a reset-globs directive whihc might make this moot.
[15:11] <flacoste> and there is an alternative to the state problem
[15:11] <danilos> perhaps large classes are the problem, but we do have them
[15:11] <flacoste> exactly, reset-globs in manuel
[15:11] <gary_poster> agree with danilos.  Also, agree with allenap's observation of manuel
[15:11] <sinzui> allenap: I have thought about breaking many registry doctests into smaller themes
[15:11] <flacoste> manuel also adds the option of running specific section of a doctest
[15:11] <sinzui> allenap: but my motivation it to control the layer the test is run on.
[15:12] <intellectronica> i think limiting the length is not the best way to handle this. instead i suggest dividing tests thematically
[15:12] <mars> I agree, I don't see the problem as much with larger narratives as it is with naughty test code state.
[15:12] <danilos> 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] <intellectronica> that is, stop and start a new file when you're really testing something new (with new state and so on)
[15:12] <bac> intellectronica: i agree
[15:13] <allenap> 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] <danilos> intellectronica, +1, basic doctests (those which are usually large) should be basic class documentation; specific tests should be unit test
[15:13] <allenap> intellectronica: The limit is an incentive to split :)
[15:14] <allenap> intellectronica: But I agree.
[15:14] <danilos> allenap, I see the point, but artificial limit would have to be broken in so many valid cases
[15:14] <intellectronica> 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] <bac> 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] <gary_poster> We do have a "800 line limit for reviews" that is a guideline but often breached
[15:15] <intellectronica> also, i'll play sinzui and ask: who's going to split all the existing test? ;)
[15:15] <danilos> 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] <mars> 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] <allenap> Okay, sounds good. Is there a way to measure or have more concrete guidelines, or is it at developer/reviewer discretion?
[15:15] <mars> it's ok if they do not find such a point
[15:15] <mars> kind of like review lint
[15:16] <intellectronica> allenap: there is a measure. variables shouldn't be recycled
[15:16] <danilos> 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] <allenap> intellectronica: I don't think we should split the existing tests... until it is no longer possible to not split them.
[15:16] <allenap> intellectronica: I like that :)
[15:16] <allenap> intellectronica: There's still a problem of db state.
[15:16] <mars> danilos, well, if they are over 800 lines, then you would recommend the refactoring, wouldn't you? :)
[15:17] <intellectronica> 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] <allenap> danilos: +1
[15:17] <mars> danilos, or 400, or whatever
[15:17] <sinzui> 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] <allenap> intellectronica: Good point.
[15:17] <intellectronica> 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] <danilos> 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] <allenap> 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] <gary_poster> 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] <intellectronica> 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] <danilos> 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] <gary_poster> and means that it doesn't help for reading text files, only processed files
[15:19] <mars> 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] <gary_poster> s/herder/harder/
[15:19] <mars> danilos, right
[15:19] <danilos> anyway, it seems this is a hot topic
[15:20] <danilos> allenap, bac, shall we move it to the mailing list or try to come up with a conclusion here?
[15:20] <gary_poster> I think a lot of people talking about dividing things up are coming from the perspective of (1) expertise and (2) testing.
[15:20] <sinzui> 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] <allenap> danilos: Mailing list I think. There are many threads of conversation and this isn't going to end here anyway ;)
[15:21] <gary_poster> they are valuable, but these are narratives.
[15:21] <intellectronica> sinzui: yes, i think that's a good guideline
[15:21] <danilos> 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] <bac> danilos: yes, let's move the discussion to the mailing list.  allenap will you start the discussion there?
[15:21] <allenap> bac: Sure.
[15:21] <mars> gary_poster, good point
[15:21] <gary_poster> :-) thank you mars
[15:21] <bac> [action] allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc
[15:21] <MootBot> ACTION received:  allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc
[15:22] <bac> [topic] Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary]
[15:22] <MootBot> New Topic:  Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary]
[15:22] <gary_poster> OK I almost have the conversation pastable but not quite.
[15:22] <gary_poster> 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] <gary_poster> 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] <gary_poster> canView is nicer because you do not specify the permission, which is fragile.
[15:22] <gary_poster> The try/except approach uses the basic security machinery directly.  It is of course the basic mechanism that Zope provides.
[15:22] <gary_poster> Edwin pointed out that this was not a pattern in the LP tree.  He was also afraid it would be slower.
[15:22] <gary_poster> I'm pretty sure it will not be slower than calling a Python function, but can verify with a timeit test.
[15:23] <gary_poster> So, I suppose I have these thoughts:
[15:23] <gary_poster> 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] <gary_poster> (that is, my main suggestion from the story above)
[15:24] <gary_poster> 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] <gary_poster> That's it.
[15:25] <gary_poster> s/conversation pastable/introduction pastable/ ;-)
[15:25]  * danilos reads
[15:25] <intellectronica> gary_poster: i also think that using try/except is clearer
[15:25] <gary_poster> cool
[15:26] <bac> 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] <danilos> I know we are using checkPermission for these things
[15:26] <gary_poster> EdwinGrubbs: are you around to say remind me what the utility was that you used at first?
[15:26] <gary_poster> Maybe it is checkPermission
[15:26] <EdwinGrubbs> yes, it was check_permission()
[15:27] <danilos> from canonical.launchpad.webapp.authorization import check_permission
[15:27] <gary_poster> ah ok, looking
[15:27] <danilos> bac, +1
[15:27] <flacoste> it's a wrapper around checkPermission
[15:27] <intellectronica> bac: why is performance an issue here? usually you don't call these in loops
[15:27] <gary_poster> flacoste: oh ok
[15:27] <danilos> example: check_permission('launchpad.Edit', pofile)
[15:27] <flacoste> and i think actually the try: except: is what is used under the hood by check_permission anyway
[15:27] <EdwinGrubbs> you do call them in loops if you are displaying a list of objects
[15:27] <gary_poster> so generally, I don'y think we should use check_permission (or checkPermission) again, if at all possible.  That's my #1,
[15:28] <gary_poster> canAccess and canWrite are less fragile
[15:28] <gary_poster> 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] <henninge> gary_poster: Is this in view or in model code?
[15:29] <EdwinGrubbs> view code
[15:29] <bac> 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] <henninge> good
[15:30] <intellectronica> 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] <flacoste> hmm, right
[15:30] <flacoste> canAcess / canWrite uses try: except
[15:30] <flacoste> whereas checkPermission uses the checker information
[15:30] <bac> intellectronica: well, a lot of these cases are in vocabularies, IIRC.  many of those do have performance issues
[15:31] <intellectronica> bac: yes, where we do it iteratively we definitely should optimize for performance
[15:32] <danilos> 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] <danilos> so, nested try..excepts sound ugly to me
[15:32] <gary_poster> 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] <gary_poster> danilo, can you send me an example to look at?
[15:33] <danilos> 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] <gary_poster> 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] <danilos> but that's probably just bad design where objects are not properly security-wrapped
[15:34] <bac> [action] gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week.
[15:34] <gary_poster> is
[15:34] <MootBot> 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] <gary_poster> cool thanks
[15:34] <bac> thanks for bringing it up gary
[15:35] <bac> [topic] Cleaning up ImportFascist warnings [bac,sinzui]
[15:35] <MootBot> New Topic:  Cleaning up ImportFascist warnings [bac,sinzui]
[15:35] <bac> jml's fix to the import fascist revealed lots of places where we have problems
[15:36] <bac> we need to drive those warnings back to zero or we'll continue to expand the problem
[15:36] <danilos> 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] <bac> sinzui has a branch that accomplishes a lot of it but has reported new code is  introducing more problems
[15:36] <bac> sinzui where did you find the bulk of the offenders to be?
[15:36] <gary_poster> danilos: awesome thank you
[15:37] <sinzui> Most were explicit import from _schema_circular_imports which were not needed
[15:37] <sinzui> They were easy to remove
[15:37] <bac> sinzui: does your branch solve the problem for LP or just registry?
[15:37] <sinzui> LP, but there are two new ones in the last 12 hours
[15:38] <bac> ah, cool.  so once this branch lands we can go back to zero tolerance as reviewers, no?
[15:38] <sinzui> BranchJobDerived BranchJobType are not in BranchJob.__all__, but are imported by translationtemplatesbuildjob and translationtemplatesbuildjob
[15:38] <gary_poster> should we make them errors?
[15:39] <gary_poster> I thought that was an option
[15:39] <sinzui> I could add these two classes to __all__, but I think the code team and translations team need to coordinate
[15:39] <danilos> sinzui, if it has happened during last 12h, it's something that's happening during a sprint in Wellington
[15:40] <sinzui> correct
[15:40] <danilos> 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] <sinzui> I will
[15:40] <danilos> sinzui, thanks
[15:41] <bac> so we're in agreement to get this cleaned up over the next week and then keep it under control?
[15:41] <danilos> 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] <danilos> bac, +1
[15:42] <bac> [action] sinzui to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues.
[15:42] <MootBot> ACTION received:  sinzui to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues.
[15:42] <bac> that's it for agenda items
[15:42] <danilos> sinzui, bac: just to confirm, this will also be part of lint checks?
[15:42] <bac> [topic] peanut gallery
[15:42] <MootBot> New Topic:  peanut gallery
[15:42] <sinzui> landing is challenging since we are in phantom testfix
[15:42] <danilos> (i.e. easy to spot)
[15:43] <danilos> sinzui, js buildbot?
[15:43] <bac> danilos: i'm not sure.  sinzuie?
[15:43] <bac> er, sinzui?
[15:43] <sinzui> danilos: importfacist has never been a part of lint since the testrunner has alwyas reported them
[15:43] <bac> thanks
[15:43] <sinzui> danilos: I thought I saw rockstar land a reversion to get us out of testfix
[15:44] <flacoste> sinzui: it's not phantom
[15:44] <flacoste> db_lp has three failures
[15:44] <bac> moving on, does anyone have an issue that is not on the agenda?
[15:44] <flacoste> related to code hosting
[15:44] <flacoste> i have
[15:44] <bac> flacoste: go
[15:44] <flacoste> but it's not related to reviewers :-)
[15:44] <bac> hmm
[15:44] <flacoste> but would like the opportunity that we have a lot of attention :-)
[15:44] <flacoste> we need a volunteer to fix the db_lp failures
[15:44] <bac> flacoste: ok, reminding you we're at 45 minutes
[15:45] <flacoste> that's all
[15:46] <flacoste> bac: you can cloase the meeting
[15:46] <bac> flacoste: let's try to resolve that outside the meeting
[15:46] <bac> #endmeeting
[15:46] <MootBot> Meeting finished at 09:46.
}}}

=== asiapac ===

No meeting due to Wellington Sprint.