Building Communities with Code Review

As delivered at Python Brasil 2014.

Transcript

1. Building Communities with Code Review Alex Gaynor
2. 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.
3. What is code review?
5. People reading other people’s code Not an architecture review
6. Why code review?
7. 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.
8. 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.
9. Catch bugs “Given enough eyeballs, all bugs are shallow”, “Many hands make light work”. Other people will catch the things you missed.
10. 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.
11. Ground rules for effective code review
12. 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.
13. 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.
14. 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.
15. 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”.
16. How to get started
17. 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.
18. How to be a good patch author
19. 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.
20. 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.
21. 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.
22. How to be a good reviewer
23. What are you looking for?
24. What are you looking for? • Intent What is the patch supposed to do? Is it really a bug? Do you really want this feature?
25. 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?
26. 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.
27. 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?
28. 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.
29. 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.
30. 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”
31. 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.
32. 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.
33. Anti-patterns
34. 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.
35. 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.
36. Positive patterns
37. Positive patterns • Share early, share often
38. Positive patterns • Share early, share often • Document the steps
39. Positive patterns • Share early, share often • Document the steps • Lend a helping hand
40. Positive patterns • Share early, share often • Document the steps • Lend a helping hand • Checklists e.g. make sure all documentation contains complete examples
41. 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
42. Code review is an important part of engineering culture I said that we could build communities using code review. How is that?
43. (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.
44. 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.
45. 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.
46. In Review
47. In Review • Code review should be a part of every team’s workflow
48. In Review • Code review should be a part of every team’s workflow • Use tooling to make the job easier
49. 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
50. Thanks! Questions? https://alexgaynor.net — alex.gaynor@gmail.com