On Thu, Sep 30, 2010 at 12:53 PM, geremy condra <debat...@gmail.com> wrote: > On Thu, Sep 30, 2010 at 9:33 AM, Barry Warsaw <ba...@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. _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
