[Python-Dev] We should be using a tool for code reviews
Jesse Noller
jnoller at gmail.com
Thu Sep 30 19:19:12 CEST 2010
On Thu, Sep 30, 2010 at 12:53 PM, geremy condra <debatem1 at gmail.com> wrote:
> 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
We *should* encourage this within the Sprints docs/dev guide, etc.
More information about the Python-Dev
mailing list