= 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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-dev
More help : https://help.launchpad.net/ListHelp
