Our Approach to Pull Requests
Pull Requests can be a way to learn, to communicate with your colleagues, to maintain a level of code quality. And it can be a source of stress, conflicts or a waste or time for all involved.
In this guide we’ve tried to summarise our approach to creating and reviewing pull requests. Obviously, not every pull request needs all of the steps below, we simply present some ideas and processes that we find helpful.
Note: we are using GitHub as our development platform and Clojure as our main programming language, so our approach may not work for everyone.
As a Developer…
We believe that if you create a pull request and you want this pull request to be reviewed as fast and as painless as possible your best approach is to… create better PRs. What does this mean? Simple! After you’ve written down a detailed description (with pictures and graphs when necessary) and summarised the purpose of the PR into a brief but precise title, after you’ve added reviewers and correctly selected the base branch… you do not press the “Create” button. Yet. Unless it’s a “Create Draft” button. Then you can press it. But you must remember that there’s much more that you can do in order to prepare your PR for a review.
When you’re preparing your PR for a review imagine that you are the reviewer who gets the request to review that PR. Do you have other PRs to review? Do you have sticky notes all over your work desk with “TODO” written on them in bold red marker pen? Would you rather chat with a friend, or have a coffee with a colleague, or finally call your parents? Or maybe you’re so tired or sad that all you want to do is to keep hitting the “Random” button on the XKCD site? Try to remember that your new PR is probably not at the top of the priorities list of other people. But there are ways to move it up there…
Size
“The size of one’s house might bear a relationship to the size of one’s opinion of oneself, but it had nothing to do with one’s real worth.”
Alexander McCall Smith “The Woman Who Walked in Sunshine”
Big PRs with tens or even hundreds of changed files and an uncountable number of commits do not make you seem smarter, but they do make your PR stay unreviewed week after week or require lots of iterations until all issues are resolved. And it’s not because the selected reviewers are envious of your work ethic or coding skills. They just don’t have the time to review big PRs. They are developers themselves, they have their own PRs to create, features to merge, QAs and product managers to make happy, lives to live.
Now, if your PR would’ve been small and straightforward — then it would have a better chance to get reviewed. Probably even first thing in the morning. Its size would tell the reviewers that this PR is friendly, easy to grasp and understand. It’s not trying to smuggle a copy of “Critique of Pure Reason” in its commit messages.
So don’t mix system-wide functions renaming with core logic changes and sheets of CSS.
Don’t think big. Think small. Think tiny.
But you might say that not all features that a developer gets assigned to can be described as a tiny PR. That is true. And that is why we recommend…
Chunking
“Any intelligent fool can make things bigger, more complex, and more violent. It takes a touch of genius — and a lot of courage to move in the opposite direction.”
E.F. Schumacher
Breaking a big problem down into reviewable units can be hard and is not always straightforward. Here’s an approach that we typically use:
- You read through the task and the requirements.
- You talk to everyone involved: the designer, the product manager, the butcher from the store on the corner.
- You think and draw circles and boxes on a paper or screen and you connect them with arrows.
Choose an approach, any approach that will help you go through a complex task and break it down into chunks. There is no universal way of doing this: some people might work top-down and break the problem down into consecutively smaller pieces. Others need to get a feel for the problem first by making changes here and there and building the solution in a bottom-up fashion. You might want to try different approaches and see which one feels natural to you. What’s important is that you increase your understanding of the problem and get a feeling for how the problem can be chunked. - You break the big idea into smaller ideas that can be implemented separately and chained together as a sequence of steps that can take you from endless procrastination to a sense of fulfilment (and finished task).
- If the implementation you’ve came up with feels like a giant block of marble in your head: no seems, no cracks, no ways to approach it — ask for help. Don’t waste time by being miserable. It could be that the task was not clear enough or maybe not even needed. It could be that you’re missing some part of information that someone else possesses. It’s ok to go and ask more questions, really. That’s why we have teams.
When you have the chunks in your head and on a piece of paper (or on a screen), you can already see what PRs you would need to create, their size and in which sequence to merge them.
But wait, do not open that code editor yet, because it’s time for…
Design Review
“The aim of argument, or of discussion, should not be victory, but progress”
Joseph Joubert
When you have the feature mapped out, you know what you’re going to do and how (and, hopefully, why), it’s time to pick up the phone and call your colleagues. Or you can just trap them in the coffee room, although a meeting invite with agenda is a more classical approach. This meeting does not have to be an official meeting with slides, it should not even be a whole hour — you just want to run your idea by your teammates to make sure what you have planned makes sense or perhaps maybe there’s even a better way that you can come up with together.
The design review session will provide great benefits:
- your potential reviewers and all other members of the team will know what you’re working on and what to expect in the codebase soon;
- some of the people or maybe even all of them (if you work in Merantix Healthcare) will have some valuable feedback with ideas, hints, suggestions or just words of support;
- when the time of the code review comes for these PRs reviewers will feel more comfortable knowing they already understand what you’re trying to accomplish.
Now, when the team gave you their blessing you can start coding. But when you’re done, go back to GitHub, open your first PR (if your task requires many) and make sure the PR is either a Draft or has a “WIP” in the beginning of the title. Why? Cause we’re not done yet!
Labels
“What’s in a name? that which we call a rose
By any other name would smell as sweet.”
William Shakespeare “Romeo and Juliet”
We use GitHub. We like GitHub. It’s not the best thing in the world (there’s Nintendo, beer, Rich Hickey’s talks!) but it allows you to easily create and add labels to your PRs. It will even choose a random but good looking colour for the new label. So use it! Use it for:
- describing the size of PR
- specifying whether it’s blocking you or not
- hinting on the contents: UX, technical, renaming, CSS hell, deployment, easter-egg, etc.
- highlighting the level of significance.
Labels are colourful, short and really pop out in a list of black and white titles of PRs on a reviewer’s screen. It makes sense for a team to come up with consistent labels and make sure everyone understands their meaning. Otherwise you may start seeing labels like: “1337”, “omg” or “bacon”.
But your PR is still not ready for review. Sorry…
Self-reviews
“If I speak of myself in different ways, that is because I look at myself in different ways.”
Michel de Montaigne “The Complete Essays”
Self-reviews are so easy to do and understand and provide such a big return for the effort that they should be illegal!
Open your PR. Now pretend it’s not your PR. Pretend you are reviewing someone else’s PR and… review it. Be as thorough as possible and make sure to do 2–3 passes. Do not rush. This procedure will save you from the shame and embarrassment of someone who has just camel-cased a Clojure var and was about to show this to the whole world.
But do not only use this exercise to clean-up and tighten up the code. Feel free to leave comments with explanations for the others. When you look at your PR from a GitHub page you can suddenly notice that some code changes although are easy to understand from the code editor look daunting in a web interface. You can also enhance your code with additional references right there where reviewers will be.
Choose Reviewers
“What is uttered from the heart alone, Will win the hearts of others to your own.”
Johann Wolfgang von Goethe
The decision on when to merge the PR in is really straightforward: all the reviewers assigned to a PR must approve it. But that means If your team has more than 2 people in it you probably want to be careful and not add all of the developers to every PR you have. I mean, you can, but it depends on some business related things, like, you know, deadlines, investor meetings, product releases, income.
Make sure you have the best qualified reviewers for the job when you’re adding names to the reviewers list for your PR. For example, if your PR is CSS heavy and you have a team member who dreams in CSS 3.0 — add that person! If your PR has an important conceptual change (but is still tiny) you might want to add more people or even all of them.
Just make sure you have at least 1–2 other developers (your ego doesn’t count) reviewing and approving (or not) your PR. This also covers the bus factor a bit, since if something good happens to you and you don’t need to go to work anymore because you’re suddenly the king of Sweden there would still be people who know about your PR, so your work is not wasted and the project is not jeopardised by your highness.
Create Issues for Tech Debt
“The future depends on what you do today.”
Mahatma Gandhi
Sometimes during a review of your tiny PR someone, maybe even yourself, will discover a potential problem or an opportunity to rename a whole class of functions or a better project structure. This is great! But if you go ahead and address this in the same PR it will increase the size, change the scope and require a re-review and maybe even some extra reviewers and a cycle or two of manual testing. It doesn’t have to be that way.
Check if the suggested change relates to your PR or to the codebase in general. Also check if it can be done separately and what’s the risk of not making the change right away (remember, we’ve mentioned business reasons? yeah, those). If the changes can be done separately, create a GitHub issue, reference the PR, the comment, the person, provide descriptions with as much detail as possible (while you still know what you’re talking about) and even agree with the team on a timeline of when this issue should be addressed. This should unblock you from merging your current PRs — as long as it doesn’t break anything of course.
In Case of Emergency — Just Ask for a (Faster) Review
“In terror the Boy ran toward the village shouting “Wolf! Wolf!” But though the Villagers heard the cry, they did not run to help him as they had before. “He cannot fool us again,” they said.”
Aesop “The Shepherd Boy and The Wolf”
We at Merantix Healthcare believe that a PR should not be blocking: meaning, if you’re waiting for code review you can just grab another task, create another tiny PR and do all those wonderful things we’ve talked about in this article. But sometimes you just cannot wait. Either your next PR requires this PR to be merged, or maintaining this PR is getting more and more difficult because you’re fixing merge conflicts with everyone everyday, or you’re just personally stressed and you feel like this PR is the last straw and if you could only merge it you would feel so much better… Feel free to contact your reviewers directly. Make sure to apologise and explain why you would like them to review your PR as fast as possible. Trust me, they’re good people they will understand. (If they are not good people, maybe you need a new team. We are hiring!)
But please remember you should only push your teammates for faster reviews if it’s truly an emergency, which means it shouldn’t happen often. Otherwise you either not creating PRs according to our guidelines (or any other meaningful guidelines in the Internet) or you’re just abusing their trust, which runs out quickly and regenerates slowly.
As a Reviewer…
We think that reviewers have already plenty of stuff to do. Don’t forget that they are developers as well and thus they are going through the same PR-creating motions as everyone else in the dev team.
However, there are some ideas that each reviewer might find useful …
A Review a Day
Make sure you dedicate some time every day to do code reviews. It doesn’t have to be any more specific than a simple “1 hour a day” rule. Or 30 minutes in the morning and 30 minutes in the evening. The duration, time of day and number of times is totally up to you. Just try not to miss a day. Make this a habit.
Glance Through when Not Reviewing
If you’re like us, you probably get email notifications from GitHub on all PRs that were created in the repo(s) you’re working on. Some of them specifically requesting your review, some of them are just PR creation notifications.
If you’re not requested as a reviewer please make sure you find some time to at least glance through the code changes. You don’t have to leave comments or even deeply understand every bit of the PR (although if it’s tiny it shouldn’t be difficult). You just need to get yourself familiar with what’s happening in the codebase, what your colleagues are doing, what are the comments that other reviewers are posting. This is highly educational so don’t rob yourself by skipping this.
Add Yourself as a Reviewer
When you’re glancing over a PR that you’re not assigned to as a reviewer and you spot something that can potentially be a bug or you get an idea for an improvement but you don’t have time to investigate and discuss your concern right away, just add yourself as a reviewer to that PR. The author of the PR will see that you’ve added yourself, which means that your approval is required before merging. Then you can take your time to think about your feedback and write it down in a polite formatted comment, thus adding value to the codebase.
Low Hanging Fruit
Some PRs are so quick to review you can do it from your phone. If devs are following our approach you should see PRs labeled “tiny” or “easy” or “piece-of-cake” in your “Review requests” list. Open one of those while you’re waiting for your coffee. Or review a tiny PR while stuck in public transport — imagine all the rubberneckers seeing code on your phone’s screen instead of a yet another newsfeed, they’ll think you’re sooo cool!
This can also provide a self-reinforcement mechanism: developers see tiny PRs being reviewed quickly thus being motivated to create more tiny easy-to-digest PRs.
Conclusion
Those were not rules, but rather guidelines and habits that have proven themselves useful to us. Remember, receiving and giving feedback requires effort, much effort, but if done properly provides the sense of pride in oneself and one’s peers. Peace!
