[Python-Dev] We should be using a tool for code reviews

[geremy condra]

On Thu, Sep 30, 2010 at 9:33 AM, Barry Warsaw <barry at python.org> wrote:
> On Sep 30, 2010, at 10:47 AM, Jesse Noller wrote:
>
>>Not to mention; there's a lot to be learned from doing them on both
>>sides. At work, I learn about chunks of code I might not have
>>otherwise known about or approaches to a problem I'd never considered.
>>I sort of drank the kool-aid.
>
> Tools aside, I completely agree.
>
> Many projects that I contribute to have policies such as "nothing lands
> without reviewer approval".  For some that means one reviewer must approve it,
> for others two +1s and no -1s, or a coding approval and a ui approval, etc.
>
> When I was the review team lead on Launchpad, I had a goal that every
> developer would also eventually be a reviewer.  We started with a small number
> of experienced developers, then ran a mentor program to train new reviewers
> (who we called "mentats").  A mentat approval was not enough to land a branch,
> but the mentor could basically say "yes, i agree with the review" and it would
> land.  Eventually, by mutual consent a mentat would graduate to full
> reviewership (and hopefully be a mentor to new reviewers).
>
> This was hugely successful among many dimensions.  It got everyone on the same
> page as to coding standards, it equalized the playing field, it got everyone
> to think collaboratively as a team, folks learned about parts of the system
> they didn't have day-to-day intimate knowledge about, and it got changes
> landed much more quickly.
>
> Here are a few things we learned along the way.  Their applicability to Python
> will vary of course, and we need to find what works for us.
>
> * Keep branches *small*.  We had a limit of 800 lines of diff, with special
>  explicit permission from the person reviewing your change to exceed.  800
>  lines is about the maximum that a person can review in a reasonable amount
>  of time without losing focus.
>
> * The *goal* was 5 minutes review, but the reality was that most reviews took
>  about 15-20 minutes.  If it's going longer, you weren't doing it right.
>  This meant that there was a level of trust between the reviewer and the dev,
>  so that the basic design had been previously discussed and agreed upon (we
>  mandated pre-implementation chats), etc.  A reviewer was there to sanity
>  check the implementation, watch for obvious mistakes, ensure test coverage
>  for the new code, etc.  If you were questioning the basic design, you
>  weren't doing a code review.  It was okay to reject a change quickly if you
>  found fatal problems.
>
> * The primary purpose of a code review was to learn and teach, and in a sense,
>  the measurable increase in quality was a side-effect.  What I mean by that
>  is that the review process cannot catch all bugs.  It can reduce them, but
>  it's much more valuable to share expertise on how to do things.  E.g. "Did
>  you know that if X happens, you won't be decref'ing Y?  We like to use goto
>  statements to ensure that all objects are properly refcounted even in the
>  case of exceptional conditions."  That's a teaching moment that happens to
>  improve quality.
>
> * Reviews are conversations, and it's a two way street.  Many times a dev
>  pushed back on one of my suggestions, and we'd have weekly reviewer meetings
>  to hash out coding standards differences.  E.g. Do you check for empty
>  sequences with "if len(foo) == 0" or "if not foo"?  The team would make
>  those decisions and you'd live by them even if you didn't agree.  It's also
>  critical to document those decisions, and a wiki page style guide works very
>  well (especially when you can point to PEP 8 as your basic style guide for
>  example).
>
> * Reviews are collaborations.  You're both there to get the code landed so
>  work together, not at odds.  Try to reach consensus, and don't be afraid to
>  compromise.  All code is ultimately owned by everyone and you never know who
>  will have to read it 2 years from now, so keep things simple, clear, and
>  well commented.  These are all aesthetics that a reviewer can help with.
>
> * As a reviewer ASK QUESTIONS.  The best reviews were the ones that asked lots
>  of questions, such as "have you thought about the race conditions this might
>  introduce?" or "what happens when foo is None?"  A reviewer doesn't
>  necessarily have to guess or work out every detail.  If you don't understand
>  something, ask a question and move on.  Let the coder answer it to your
>  satisfaction.  As a reviewer, once all your questions are answered, you know
>  you can approve the change.
>
> * Keep reviews fast, easy, and fun.  I think this is especially true for
>  Python, where we're all volunteers.  Keeping it fast, easy, and fun greatly
>  improves the odds that code will be reviewed for the good of the project.
>
> * Have a dispute resolution process.  If a reviewer and coder can't agree,
>  appeal to a higher authority.  As review team leader, I did occasionally
>  have to break ties.
>
> * Record the reviewer in the commit messages.  We had highly structured commit
>  messages that included the reviewer name, e.g.
>
>  % commit -m"[r=barry] Bug 12345; fix bad frobnification in Foo.bar()"
>
>  thus recording in the revision history both the coder and the reviewer, so
>  that we could always ask someone about the change.
>
> * Don't let changes get stale.  We had a goal that changes would go from
>  ready-to-code (i.e. design and implementation strategy have already been
>  worked out) to landed in 2 days.  The longer a change goes before landing,
>  the more stale it gets, the more conflicts you can have, and the less
>  relevant the code becomes.
>
> I know this sounds like a lot of process, but it really was fairly lightweight
> in practice.  And that's the most important!  Keep things fast, easy, and fun
> and it'll get done.  Also, these ideas evolved after 3 years of
> experimentation with various approaches, so let it take some time to evolve.
> And don't be so married to process that you're afraid to ditch steps that are
> wasteful and don't contribute value to the project.
>
> Certainly some of our techniques won't be relevant for Python.  For example,
> we assigned people to do nothing but reviews for one day out of the week (we
> call it "on-call reviewers").  This worked for us because team velocity was
> much more important than individual velocity.  Even though at first blush, it
> seemed like you personally were going to be 20% less productive, team
> productivity skyrocketed because changes were landing much faster, with much
> less waste built in.  This probably won't work for Python where our
> involvement is not governed by paycheck, but by the whims of our real jobs and
> life commitments.

Extremely well put. Could this kind of process be put in place for the
code sprints Jesse's interested in? Seems like an ideal testbed.

Geremy Condra