Pull request template for the win!
First steps
More than a year ago, our Android team at RSQ Technologies introduced a pull request description template into the code review process. It moved us one step further into more transparent and fluent application development. At that time our app was already two years old, with new features in development and continuous maintenance. Through this development process, we stepped into multiple problems with lack of knowledge about certain parts of the codebase. The code was getting harder and harder to understand with every new view, feature, or module.
We decided to use the pull request description template to help us understand the changes introduced into the app, find bugs, and point out code smells.
In this article, I want to show you our template which we have used for a year. Next, I will discuss things that worked, things that did not, and changes that we introduced. If you just are just interested in the final form of our template — just go to the Current code review template section.
What we started with
Our pull request template has grown organically. Some of the points were based on the articles which we read at the time, some of them were added later on our weekly team Retrospective meeting.
# Description, Motivation, and Context ## What is the destination branch of this PR? #
# How Has This Been Tested? ## Screenshots (if appropriate): #
# Types of changes #
[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)# Checklist: #
[ ] I merged current develop to this branch.
[ ] My change requires a change to the documentation (confluence test paths, code style etc.)
[ ] I have updated the documentation accordingly.
[ ] My change requires a change to the UI tests.
[ ] I have updated UI test accordingly.
[ ] I unit tested new methods.
[ ] I checked linter on local.
[ ] I checked unit tests on local.# Is additional manual testing needed? ## Files with major changes / what is worth paying more attention to? ## Which variances have been affected #
[ ] variant 1
[ ] variant 2
[ ] variant 3
[ ] variant 3
What worked, and what did not
- Description, Motivation and Context
Worked? Definitely yes.
One of the most important points on the list. When reviewers start to read the pull request, they have the most important information about the changes.
- What are the changes?
- Why have you chosen this solution?
- What was the goal of that task?
- What was the basic idea behind the code?
All of the information written under the first point gives the reviewer a clear view of the background of this change. It is much easier to point out pain points in the code when you understand what was the motivation behind this change.
2. What is the destination branch of this PR?
Worked? Yes.
This one might sound silly at the first glance. You might say that the destination of the pull request is probably displayed somewhere on the top of the page (the place is depending on what you use Github, BitBucket, etc.). This question was added to the template so the PR creator would pay attention and double-check the destination.
We had multiple problems after merging the branch. Then realizing that it was merged to the default develop, not to the release branch or feature branch. Adding this question to the template solved the problem.
3. How has this been tested?
Worked? Yes.
In our Android team, in this point, we mostly focus on the emulators and physical devices used for testing the code. Sometimes it was important to make sure that the feature was tested on both phone and tablet or just phone with small pixel density.
When the pull request introduces larger changes into the app, that part also consists of testing paths that have been checked, random data that have been typed, and so on.
4. Screenshots (if appropriate)
Worked? Definitely yes.
That idea came to us with Perfecting Process for Presenting PRs by Ataul Munim. This article was presented on one of our weekly knowledge sharing meetings and right after it ended we instantly decided that we must use it in our code review process!
In the Android environment, it is not that fast to checkout and display somebody’s specific view where the change was introduced. When the project is quite big it might take tens of minutes to checkout a new branch, compile the code, and click the app right to the place where modification was made. It is also hard to visualize the change just from reading XML files on Android or HTML/CSS on the web.
When pull request consists of the screenshots you can tell right off the bat if the visual part is looking correctly or there is something wrong with it.
5. Types of changes
Worked? Not really.
[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)The first nonsense on the list. Three checkboxes which are supposed to give reviewer more context about the task but at the end they were non-informational. In most of the cases, it was a duplication of information from the first point Description, Motivation and Context.
6. Checklist
Worked? In most cases - yes.
[ ] I merged current develop to this branch.
[ ] My change requires a change to the documentation (confluence test paths, code style etc.)
[ ] I have updated the documentation accordingly.
[ ] My change requires a change to the UI tests.
[ ] I have updated UI test accordingly.
[ ] I unit tested new methods.
[ ] I checked linter on local.
[ ] I checked unit tests on local.All of those points were added just to remind the developer if she/he prepared code before it was ready to be merged.
Checklists are recommended in most of pull request templates.
7. Is additional manual testing needed?
Worked? Yes.
The developer may ask others in their team to check out the feature and test it by themselves before merging. It is mostly used in feature branches or bugs for edge cases.
8. Files with major changes / what is worth paying more attention to?
Worked? Hmm… Sometimes?
This point was added somewhere in the middle of the process when new pull requests came with massive changes but mostly with file migration and renames. In those cases, we wanted to point the reviewer to focus on the most important files where most of the logic was happening.
9. Which variants have been affected
Worked? Yes.
This point might be quite specific to Android development. Variant is a type of build — it defines what app is built from the codebase. In our project, we build multiple apps (more than four) from one codebase. It is quite important to know which apps are going to be affected by this modification.
It might work for you as well if your project is similar.
Next steps
After working with this template for one year we decided to do some changes. We started a chat on our work messaging app to discuss all the stuff we liked and disliked about the template — what we normally skip, what was most important, and which questions were not precise enough. It was time to refresh the idea and talk more about new checkpoints, questions. Also removing some purposeless points.
One of the articles which also pushed us to take the action and introduce some changes was A Code Review Checklist to Focus on the Important Parts by Michaela Greiler. All of the new points added to the code review were based on those proposed in her article. Thanks!
1. Link to the resources (mockups, change description)
When there is more thought standing behind the change, it is good to have some links to resources that give you a wider perspective on the problem.
2. Does this change require updates outside of the code, like updating the documentation, configuration? If so, was this done?
Previously that question was hidden in the checklist. We converted it into the question, so the PR creator might give it a deeper thought.
3. Is the code testable? If not - why?
We stumbled upon some issues with testability in our codebase. So we wanted every programmer to rethink his code in case of testability and start a little discussion if there is a possibility to give the tests some more time.
4. Does similar functionality already exist in the codebase? If so, why isn’t this functionality reused?
Similarly to the previous point — we wanted a place to discuss if feature like it was already written somewhere in the codebase and maybe it can be reused.
Current code review template
# Description, Motivation and Context# Link to the resources (mockups, change description)# What is the destination branch of this PR?# How Has This Been Tested?# Screenshots:# Does this change require updates outside of the code, like updating the documentation, configuration? If so, was this done?# Do reviewers should manually test your changes?# Is the code testable? If not - why?# Does similar functionality already exist in the codebase? If so, why isn’t this functionality reused? # Checklist:
[ ] I unit tested new methods.
[ ] I checked linter on local.
[ ] I checked unit tests on local.# Which variants have been affected
[ ] variant 1
[ ] variant 2
[ ] variant 3
[ ] variant 4# Files with major changes / what is worth paying more attention to?
(Delete this point if it is not needed)
Should you use pull request template?
Yes! If there is any advice that I wanted to share with you in this article — give more attention to the code reviews. When reviewers know the context, understand the data flow, and the final result is visible — it is easier to spot the problems within the code.
Make your own pull request description template, add Bitbucket PR checks, automate processes with Continous Integration, try Danger. Make your way to better code reviews.
Well-prepared code reviews make well-reviewed code. Which at the end, makes better apps.
