On Aug 26, 2013, at 3:37 PM, Richard Wall <[email protected]> wrote:
> 1. Encourage new code reviewers -- various people have said they'd do > code reviews but are unsure of the process or feel unqualified to > comment on other people's code. So: I find that there are three major impediments to encouraging new reviewers. Most significantly, new reviewers do not actually know which code they can review. New reviewers should be reviewing only patches by people who already have commit; otherwise, new contributors end up getting their code reviewed by another new contributor who might not know the ins and outs of the process. In the best case they will get an incomplete review; in the worst case, they will be told "looks okay to me, merge" and then get kicked out of the review queue with no way to land it. I think that's a fine rule, but it has one gigantic problem: we don't have a list of committers anywhere. Making this list - and, critically, associating VCS handle (svn.twistedmatrix.com login) with Trac handle, for those cases where it differs - would really help new contributors figure out whether there's Best of all would be a separate report, "Review Tickets Submitted By Committers", which would be things that external contributors could review. Of course, lots of these will be very confusing-looking to someone new to the Twisted codebase, but it's easy enough for people to skip things they don't understand :). New reviewers don't know that it's OK to make a mistake. So first let me say: it's okay to make a mistake. Not every review is perfect. If you missed something significant, there are several filters to catch problematic reviews. First, if you're really new, and you're reviewing something for a committer, then it's their responsibility to make sure your review looks thorough enough. They can always put it back into review if they think you *might* have missed something. This is not a failure! This means you got some valuable practice and feedback. Second, plenty of people watch trunk commits. If a dodgy commit gets through, someone will generally notice and revert it. If you feel like your review needs a second look, feel free to pipe up in #twisted-dev on IRC and say "would someone please have another look at this just to make sure". Third, this is why we have the pre-release cycle: it's a final chance for everybody with Twisted applications to make sure that nothing broke anything. We don't have any mentorship in place; even with a comprehensive checklist, it can be intimidating to try to do a code review. Really, we should set up a mentorship program where mentors do code reviews with people watching. Unfortunately, our existing reviewers are generally overtaxed as it is, so for now it falls to (brave) newbies to ask for mentoring help. I feel like if someone could hack up a Trac report, or an extension to the high scores page, that addressed the first point, it would be a lot easier to deal with the second and third ones. So, if anyone reading this would like to do that, I would be forever in your debt. -glyph
_______________________________________________ Twisted-Python mailing list [email protected] http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
