How to Effectively Open Pull Requests

Pull requests is a must-have tool at any world-class tech company that pushes code. It ensures your code is being reviewed by fellow engineers and boosts the reviewee’s confidence that their code upholds a set of standards that the engineering team agreed on. But like every cup of ️☕, quality, plays a part in a PR. Poor quality PR’s can leave the reviewee and reviewer a bad taste in their mouth, commonly caused through bashing and non-constructive feedback. It can lead to distrust and annoyance between the engineers, directly affecting the codebase and even the company’s culture. However, an effective PR can generate good organic conversations about best practices, code conventions, and team standards. The code modifications in a PR will affect the codebase in its present and future form. Ultimately, each and every PR that is approved and merged are LEGO blocks that builds the product and company. With that in mind, it is rather important that engineers nail down the essentials on how to open effective PR’s. Effective PR’s should generate conversations on mind sharing, best practices, engineering conventions, and team unity. Creating effective PR’s may seem like a trivial task, however many engineers fall into poor habits without even realizing them. Here are some tips on how to step up you PR game.

Keep the pull requests short

How do you annoy another engineer before reviewing your code? Open a large pull request with over 10,000 lines of code change. Eye strain, back aches, and migraines are just some side effects engineers experience when they have to scroll through a ginormous PR. Keep you PR’s short, simple, and to the point. Historically, large PRs are often approved and merged in quickly because the reviewer skims through the code leaving no comment in desperation for the scroll to end. If the team doesn’t care about the health of the codebase, by all means, but if your team is striving for quality and care, open small and concise PRs, as a side bonus your reviewer will thank you. There’s no magic number on how many line changes a PR should be, but break up your PR if you have a difficult time following or describing what the code is doing in one sentence. Another good rule of thumb is having a 1:1 ratio between a ticket and a PR and if the ticket seems too large to begin with, break the ticket up to subtask tickets, this will ensure smaller PR’s. Remember, easy to read PRs are suppose to generate good conversations and collaboration among the engineers. PRs should not be used to track competitions on who can change the most lines in the repo. Another side effect of having large PR’s are the constant rebasing that are necessary to keep the branch up to date due to the large amount of files it modifies. Keep them short and simple is the name of the game.

Keep comments constructive

Receiving non-constructive and incomplete feedback are fairly common, it devalues the effort and won’t benefit the parties involved. Some comments may even seem better left out as it could’ve offended or insulted the engineer who opened the PR. Leaving constructive feedback engages good conversations. Here are some do’s and don’ts when commenting on PR’s:

• Don’t tell the engineer how YOU would implement the solution. We all know there are dozens of ways to reach a solution. Just because the reviewer might do it another way doesn’t mean the engineer has to implement it a certain way. As long as the code follows a set of team conventions and architecture, give it the green light.
• Don’t focus too much on code style such as extra spaces and putting else’s on new lines. Leave these tasks for a linter and not for the human eye. Don’t waste your efforts locating missed code styles, instead focus on the actual implementation.
• Do give positive feedback. Giving positive feedback encourages your fellow engineers. Receiving constructive feedback is great, but too much of them can still make the reviewee feel ganged up on. We’ve all opened PR’s that solves a difficult problem, sympathize with your reviewee and tell them that they’ve done a good job refactoring or thank them for fixing an annoying bug that’s been around for a period of time.
• Don’t use words or phrases such as “just” or “I thought I told you to…”. These phrases can imply that the reviewee should’ve known some facts but failed to use this knowledge, when in most cases, the reviewee had no knowledge about it. It can make them feel outed and discouraged by implying some criteria. Instead, teach your fellow engineers and point them to the right direction. Everyone loves an engineer who can teach, learn, and remain friendly. Here are some examples of empathetic comments:

Instead of “Just use the User model factory” use “Check out the user model factory, there’s a function there that may help what you’re trying to accomplish”

Instead of “Didn’t I tell you not to allocate this Car object” use “What’s the reason we are allocating this Car object? What do you think about refactoring this?”

Instead of “Why did you do this? What’s going on here?” use “Let’s sync on this snippet, trying to further understand the ticket specs.”

• Do leave questions when you don’t understand a code snippet. If you have a question, chances are other engineers will also have a similar question and the code may not be clear enough to describe what it is trying to accomplish.
• Do give alternative solutions when you see a code smell. When you see a red flag in the PR, don’t just leave a comment with no direction. Keeping the reviewee guessing and leaving them with no direction is simply not acting as a team player. Contribute ideas and collaborate on solutions, this is what drives effective PR’s.

• Don’t have unrelated PR conversations. PRs, when done correctly are great forms of documentations and would be ruined if you had to read about someone’s dinner plans while reading through a PR history. Keep them directly related to the code it modifies. Leave your dentist appointments out of it.

Add pull request descriptions

Don’t forget to provide a detailed description on what the PR is accomplishing. Reading code snippets without context can be frustrating especially if the PR is on the larger side. Leave a few sentences describing what your end goal is. Specify whether the PR was a feature, bug or chore? Is the feature intended to roll out to everyone or just to a selected target? If front-end is involved with the code modification, generating screenshots or gifs may provide more visual aid. Using templates is also a great way to ensure the reviewee doesn’t forget to provide a description. Lastly, providing the link to the corresponding ticket allows the reviewer to dig deeper into the details.

Pay attention on opening effective PR’s and you should start to notice a better engineering experience. Don’t let your ego take over your PR’s and work together with your fellow engineers. Remember, the codebase is a product and reflection of the team and if you want quality, work together not against each other. Teach each other and give directions when needed. Happy coding.

Thanks for reading this blog, I am sure you may have additional pointers from your own experience. Feel free to share them by leaving comments, tweet at @ginowu07 or linkedIn https://www.linkedin.com/in/gino-wu-60baba67

Special thanks to all the hard work the Stash iOS engineering team spent on iterating our PR process and to Annie H. Lee for providing images.