Pull Request Etiquette

Published at 31 Dec 2019 by soywiz ( Sources: [1] )

Programming is not just about creating code that machines understand; programming is all about communication with people.

Even if you are working on a codebase alone, even if you are totally confident nobody else will ever see or touch that code, you are still communicating.

Maybe you are not going to read that code right now, or maybe you know that code so well, that you think everything is ok. But after a few weeks, months or years you might want to understand or update that code. And know what? In that case you were the person that you were communicating to: your future self.

With the rise of OpenSource CVS, Subversion and GIT, we started to be able to work along other people while creating code, without having to share code via diskettes. With the rise of GIT, Pull Requests started to become the new norm for sharing code with people you don’t know, or even in your own team. This gives us the opportunity to make the team grow as a whole, but makes communication even more important than ever.

Be patient

This is super important, especially on Open Source projects. Think that the person reviewing has her own life. Even if her job is to work on that project, might have already defined priorities, deadlines, work in progress and tasks.

You can ask for a follow-up after some time (maybe a week or two), but don’t push too much that person and be sympathetic with her.

Use descriptive commit and PR titles and descriptions

Describe what the PR does in the title, and use the description to provide further information on the details or the rationale behind that change or addition suggestion.

Make small Pull Requests

Long Pull Requests could be stalled forever when too long. So try to guess a way to make things happen in a less disruptive way that could be improved iteratively.

I myself made several mistakes in this direction 🤦‍♀️:

But yeah, you usually learn from your own mistakes.

Do rebasing to clean-up your code

Learn how to do rebasing on GIT by mixing several commits together (squash), splitting them, updating their description and so own on your own branch. This will help you create commits that are easy to read and understand by the reviewer and will increase the likely of that PR to be merged. Also think that in some circumstances some commits will be reverted later on, if they are properly splitted, it will be possible to revert just the right commit while keeping the rest.

Do not mix new features, cleanups or bugfixes

This is directly related to rebasing your code and keeping commit titles right. Different tasks should be done in different commits with different titles, keeping the GIT history right.

Make separate commits adding failing tests

When fixing a bug, I have find pretty useful to create two commits: one commit adding a failing test case, and other fixing it (pushing separately for each commit). On systems with continuous testing, it will trigger a check and the failing commit will be in red. That will help the reviewer to be confident that indeed that test failed with the previous code. The next commit will go green and that again will give confidence that the issue was fixed without breaking something else already properly tested.

Don’t take comments personally

When reviewing, people usually don’t try to be mad at you. They just try to think on ways of improving your code. Like in pair programming, that doesn’t mean you are a bad or worse programmer than the reviewer.

Be respectful with other people and keep your own ego under control

Both reviewers and people making Pull Requests should keep their own egos under control. Don’t take comments personally, try to explain your points with respect, and try to understand the other people, while improving as a coder and as a human being. Every single person has things that could be improved, and lessons that could learn. Let’s grow as a team.