Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Building Communities with Code Review

Alex Gaynor
November 07, 2014

Building Communities with Code Review

As delivered at Python Brasil 2014.

Alex Gaynor

November 07, 2014

More Decks by Alex Gaynor

Other Decks in Programming


  1. About me • Software Engineer at Rackspace • Frequent Python

    open source contributor • Django, PyPy, OpenStack, CPython, cryptography.io • Director of the Python Software Foundation I have the unique privilege of being here with *2* sponsors. On behalf of the Python Software Foundation I’m thrilled to be able to be here to speak with you. I want to thank all of the organizers, and all of *you*, the attendees, for being a part of building a strong, diverse, and international Python community.
  2. Raise the bus factor Force another person to be familiar

    enough with the code to pass judgement on it. The more people who understand the code, the more people who’ll be able to maintain it in the future.
  3. Ensure readability The properties of readable code and writable code

    are different. Getting someone who has never had to write the code to read the code makes it more likely the next person who has to read it will be able to.
  4. Catch bugs “Given enough eyeballs, all bugs are shallow”, “Many

    hands make light work”. Other people will catch the things you missed.
  5. Encourage a healthy engineering culture People need feedback to get

    better at their jobs. Code review gives you a structure in which to give people feedback. When feedback is irregular, rather than a core part of the job, feedback can be used for criticism, instead of learning and growth. Everything here works on two conditions. People are acting in good faith, people are committed to not being jerks, and people understand that they are reviewing code and not people. This applies to reviewers and reviews. There are a lot of people who believe that code review requires being a jerk. This is wrong, and this is mean, and I’d much prefer this people just walked away.
  6. Don’t make people do a machine’s work People are good

    at people things, machines are good at machine things. Code review is not running the tests, or checking for style guide violations, or computing test coverage. Get a CI server to do those. Humans will screw it up because it’s busy work, and machines are better at some types of feedback.
  7. Everybody gets code reviewed Code review isn’t something senior engineers

    do to junior engineers. It’s something everybody does. The person on their first day gets to review the first engineer. No one is above review, no change is above review.
  8. Do code review before code gets merged Some people do

    post-commit code review, meaning code gets landed on master and then it gets reviewed. This promotes a feeling of inevitability, where reviewers don’t want to give what’s perceived as “nit picky” comments because the change is landed already.
  9. Every change. Every time. No patch is too small or

    too simple for code review. I have a 100% rate of patches which are “too small for feedback” breaking the build. This also forces you to have a system where code review is easy, and means you never have an argument about what really is “too small”.
  10. Get a tool • Phabricator • Github • Gerrit •

    Whatever Get yourself a tool that tracks the history of a patch and lets you leave inline comments. It doesn’t matter what, as long as giving feedback and acting on it is easy.
  11. Describe what the patch does Make it easy for a

    reviewer to know what a patch is supposed to do. They have to be able to verify the code you wrote does what you meant it to do.
  12. Keep it small A study has show that beyond 200-400

    lines of diff, the efficacy of code review decreases, so keep your patches small. A long series of small patches is much better than a few giant patches.
  13. Code review is collaborative You need to work with your

    reviewer to get the best patch. If you disagree with some feedback, talk to them.
  14. What are you looking for? • Intent What is the

    patch supposed to do? Is it really a bug? Do you really want this feature?
  15. What are you looking for? • Intent • Design Is

    the change in the right place? Did they change some CSS when the HTML was wrong? Did they add javascript to patch the CSS?
  16. What are you looking for? • Intent • Architecture •

    Implementation Does the patch do what it says? Does it do it completely? Does it add new bugs? Does it have documentation and tests? This is the bulk of code review.
  17. What are you looking for? • Intent • Architecture •

    Implementation • Grammar This is the small stuff. Does the variable have a good name? Should something be a keyword argument instead of a positional argument?
  18. What are you looking for? • Intent • Architecture •

    Implementation • Grammar You want to work from intent down to grammar, in order. If you start at grammar, giving feedback on variable name, it’s easy to lose sight of what you’re doing and miss the fact that the patch is totally in the wrong place.
  19. Types of review elements As you’re reading through the description

    of the patch, and the patch itself, you should be leaving comments, here are the types of things you want to be putting into your comments.
  20. Types of review elements • TODO These are things that

    MUST be fixed before a patch can be land, such as a bug or a regression. One of my favorite things to ask for is that more comments be added, it’s very common that I’ll say, “The patch looks fine, but can you add a comment explaining why the code looks this way”
  21. Types of review elements • TODO • Questions These are

    points of clarification. For example “Would this be faster if it was rewritten like so?” or “Can we reuse the logic in the standard library for this?”. Nothing necessarily needs to be done here, it’s just a question for follow up.
  22. Types of review elements • TODO • Questions • Suggestions

    for follow ups These are things that the patch author might want to do, but doesn’t have to, and might be done later. For example when adding a new feature you might suggest that a bug be filed to make it be used somewhere else in the code base.
  23. Very manual processes Makes doing code review a pain, 5

    minutes of thoughtful work become 20 minutes of fighting with details of the CI system or issue tracker. Makes people less likely to want to do code review, which is terrible for an OSS project.
  24. Irregular When code review is irregular, what that really means

    is “the new people have to deal with code review, but the rest of us don’t”. This separates the core developers from the new contributors, and that’s crummy, for one it makes it hard for core devs to care about the experience of new people, they dont’ have that experience! It’s also a bad social dynamic.
  25. Positive patterns • Share early, share often • Document the

    steps • Lend a helping hand • Checklists e.g. make sure all documentation contains complete examples
  26. Positive patterns • Share early, share often • Document the

    steps • Lend a helping hand • Checklists • Document your process! e.g. make sure all documentation contains complete examples
  27. Code review is an important part of engineering culture I

    said that we could build communities using code review. How is that?
  28. (It also makes it easier to ship good code) The

    first way is that code review helps us write better code. That’s usually why we do it in the first place. Writing better code makes people more interested in working on our projects, makes them want to stay longer, and makes them enjoy themselves even more. People *like* working with good code.
  29. Code review helps people get better When people receive code

    reviews, it’s pretty clear that it can help them learn how to do their job better. Writing code reviews can do the same things, it helps strengthen our ability to look at brand new code, helps us learn to look for bugs more effectively, and helps us develop better design skills. A key part of building a community is helping new people find their way, and code review is an easy way to do it. It’s often much faster to just say “there were a few issues with this patch, I’ve cleaned them up and committed” them, but if you work with the patch author for them to improve, you’re more likely to get someone really interested in staying around. People *want* to be part of a community that wants to help them improve.
  30. Code review makes the core-developers less special One problem open

    source projects often face is that the core developers are “special”, companies also often have this where very senior developers are special. They can commit stuff on their own, they don’t get feedback from other people. This can be very discouraging to new people. Mandatory code review puts everyone on the same playing field.
  31. In Review • Code review should be a part of

    every team’s workflow • Use tooling to make the job easier
  32. In Review • Code review should be a part of

    every team’s workflow • Use tooling to make the job easier • Use code review to promote collaboration and learning on your team