Polish Your PRs 💅 + Rev Up Your Reviews 🚗

Published in

Turo Engineering

8 min readJul 2, 2018

It’s first thing Monday morning, you’ve opened up your email and noticed that @en6in33r [Handle changed to protect the innocent. -Ed] has requested you to review their PR. You open it up and discover that it has no description, contains 29 commits, changes 73 files with 3,000+ additions and 2,300+ deletions. 😮 Where do you begin?!

Defining a standard for how PR’s should be formatted and clear goals for reviewing peers’ code is helpful for ensuring that feedback is meaningful and that the process is efficient amongst the team.

We’ve worked to improve the review process on the Front End team over at Turo. Here are some of the helpful tips we’ve learned along the way.

Note: this blog revolves around using git/GitHub for a team’s version control and code review process. You’ll still be able to use and adapt these tips within a different toolset.

It starts with your commits

A long time ago, in a galaxy far far away, there once was an Engineer that committed their changes like this:

> Fix the bug> Jk that didn’t fix it, this time it’s fixed> OMG WHY IS THIS BUG STILL HERE> Final bug fix> Final final bug fix

Well, maybe that hits a little closer to home… it might have been you… yesterday. We’ve all done it.

Committing often (and pushing) is valuable. It helps guarantee that if your computer decides to retire right before a deadline, that your work won’t be lost. On the other hand, doing so can convolute the commit history and make it hard to understand what’s going on.

Clean up your commit history if it gets messy — If you do find yourself in the situation above, before submitting your code for review, rebase against master (or whichever branch is your base for releasing) and clean up your commits. This will help structure your changes in a way that makes it easier for your fellow reviewer to parse.
Explain why the change is necessary — Though the names of functions and variables should be descriptive enough to inform what the code is doing, commit messages should aim to answer both what is happening and why it needs to happen.

An example — say that you have a React button component that needs to be extended to allow it to display in different colors depending on which type of user is viewing it. Even a simple change can benefit from a thought out commit message:

Not good:
> Add button colorGood.
> Add color prop to allow Button to be green for Guests or purple for Owners

Everything cleaned up and ready to go? Time to open a Pull Request (PR)!

Fill out the Description

Going back to the (coughnightmarecough) PR that I shared at the beginning —can you tell what it’s about? No? It’s important for Engineers to provide sufficient context to the people reviewing their changes. It can be hard (or nearly impossible) to provide a successful review without being given all of the appropriate information at the start— and tracking down these pieces can add a lot of extra time.

Consider including the following in your next PR to help your fellow reviewer:

Link to the product requirements — At Turo, we use Jira to track and break down the features that we are working on. A link to the Jira ticket (or tickets) that are involved in a PR are included in the PR’s Description in order to make it convenient for the reviewer to look back at the acceptance requirements.
Give it a meaningful title — Typically, we include the ticket number and name.
Summarize the changes — in a few sentences, what is in this PR and why is the change required. (Hint: you might be able to copy this directly from the original task ticket)
State how to test the feature — Does the Reviewer need to be signed in with a specific test account? Is there a URL for a test instance that they should use to test? Describe the user experience/flow that they would need to go through in order to reach the feature or confirm the absence of the bug.
List any actions you need to take before this is deployed — Sometimes you have tasks, like waiting for translations to be completed or deploying another branch, that will hold up a PR from being merged, but they may not be a blocker for getting a review. Noting these in the PR prevents the reviewer from taking the time to leave comments when the items are already on your radar.
Provide screenshots of the before and after when making a UI change or introducing a new component. I’ve found that creating Markdown tables* helps to clearly display the changes side by side.

Markdown in GitHub PR and result:

| Before | After |
| --- | --- |
| ![alt text](link to image)|![alt text](link to image)|

Requesting Review

We are responsible for deploying our own code at Turo, and each Engineer is responsible for making sure that their work gets the eyes 👀that it needs before it can be shipped 🚢. Our Engineering team guideline is that each PR gets two ✅/👍 review approvals before it can be deployed.

Be clear about deadlines — Everyone has a lot of work on their plate, so if you aren’t clear about when you need the review by, it could drop to the bottom of someone’s to-do queue. If it fixes a major product bug or a new feature is being scheduled to go out alongside a marketing announcement, make sure that the Reviewer is aware and can plan accordingly.
Be clear about what you’re looking to have reviewed — Is this a work in progress that you want an initial round of feedback on to make sure that you’re making good architecture decisions? Is this feature in its final form and ready to ship ASAP? Is it a proof-of-concept for a library you’re considering utilizing?
Request review from someone who’s familiar with the code — If you’re working in an existing feature, consider asking someone that has worked in the space before to review the changes. Their familiarity with it will afford a quicker understanding of the change’s context and potentially lead to a shorter turn around.
Alternatively, request review from someone who is new to this part of the code base — Requesting review from someone that has less familiarity with the area is also valuable. Though it may take a bit longer for them to understand the context and get through the review, they come in with a fresh set of eyes that may catch an edge case or bug that you missed due to running through the same flow so many times during development.
Avoid rebasing after you’ve requested a review — As a reviewer, it can be hard to tell what’s new vs. what you’ve already looked at. If you absolutely need to rebase, check in with your Reviewers to make sure it’s not going to impact their progress.

Pro tip: if waiting for code review is a blocker for moving on to the next item on your to-do list and thus leaves you with some free time, offer to take a look at a PR opened by the Reviewer you just requested in order to help open up their plate. Teamwork!

Leaving and responding to feedback

First and foremost, be respectful. There is always more than one way to write a piece of code and it’s okay to have different opinions. To the Reviewer, ask for clarification if something doesn’t make sense. To the Committer, don’t be offended by suggestions.

Be conscious of how you phrase comments — Comments that contain required and suggested changes should be distinguishable from one another. When leaving comment for a required, merge-blocking change, explain why it needs to be addressed (e.g. a given line or segment does not meet a team standard).
Link to documentation to further explain the problem or support the suggestion that you are making

> Not good:
This should be a boolean.> Good.
We've stored a variable like this in the past as a boolean instead of a string [link to file]. I would suggest doing that here too.

Respond to comments as soon as you can after someone reviews your work. As a product and team continues to grow (join us!), the number of PRs does too. To ensure that everything is staying on track, it’s up to the owner to continue to play an active role in getting their work through the review process.
Take larger discussions offline — If there’s a back and forth that seems to continue to go on, don’t hesitate to chat in person — sometimes that’s more efficient. Make sure to summarize any decisions made offline in the PR to keep everyone involved up to date.
Lint your code! By utilizing ESLint and Prettier we’ve been able to produce a consistent code style amongst our team. Gone are the days when a PR ends up with a dozen “delete these extra lines”, “add a semi-colon”, “this line is too long” comments. A linter will highlight code that doesn’t adhere to certain style guidelines and Prettier will look at the set of rules the team has established and reprint the code to follow them. It’s a HUGE time saver.

That’s all folks 👋

Whether you’re opening PR’s or reviewing (most likely both), I hope that some of these tips will help make the code review process smoother for you and your team.

Bonus tip: If your team uses Slack, you can set up a slackbot to post previews of each GitHub PR that is opened. Images and gifs are rendered in the preview and make for a great way to grab your peer’s attention to persuade them to check out your PR 😉. Here are some of my favorites: