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

Barry Warsaw barry at python.org
Thu Sep 30 18:33:52 CEST 2010

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

* 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://mail.python.org/pipermail/python-dev/attachments/20100930/4896f4dd/attachment.pgp>

More information about the Python-Dev mailing list