Merge Requests your Reviewer will Absolutely Love

One of the biggest concerns that reviewers raise is lack of contextual details on Merge Requests. This takes them a lot of time to first establish context and review even the smallest of changes.

‍While we do give out a list of standards to our new engineers, however we didn't had a proper Standard Operating Procedure Template for committing code and creating merge requests. This post will serve as a standard for our engineers to follow before creating a merge request for code review.

Note: We’ll be using the terms Merge Requests (GitLab) and Pull Requests (GitHub) interchangeably as they are exactly the same thing.

After seeing the above MR, any reviewer would definitely feel overwhelmed at where to start, and will be unsure of what the author is expecting from the reviewer. Although some context can be taken from the name (AH_college_signup) however it is not clear why these changes were made, any assumptions the engineer made during implementation, and  if any refactoring was required (in separate modules) to accomplish those changes.

Before we dig-in deeper, the best advice for an author is to refer to Confucius' quote.

"Do unto others as you wish others do unto you.” 

Before submitting your code (or pretty much anything for review), try to see things from the other side. A reviewer might receive 5-6 merge requests in a single day, while doing their own work at the same time. In case of a reviewer managing different projects/products, their day might include meetings with various stakeholders while fixing hard problems or even production issues.

OR

‍Maybe they’re not having their best day or are stuck on a problem. Hence, EMPATHY should be part of your Merge/Pull Requests.

Merge requests are much more than a display of code changes. They’re a history for your repository and demands as much time and attention as the code they contain. A merge request is an opportunity to convey What, Why, and How a set of changes were made.

When another engineer (or even you yourself in the future) stumbles upon your code in months or years, they should be able to trace back to the merge request to find more information about a given change. Additionally, a merge request acts as a guide to your code for the reviewer.

It gives the author (you) a chance to explain the context of the problem and the background of their solution to the reviewer, and allows them to call out what they’re looking for in a review.

Here are a few ways you can create pull/merge requests that your reviewer(s) are going to love:

1. Use a Standard Template

Follow a standard template, ask your lead to provide one so your team can ensure that you’re all on the same page when it comes to what should be documented in a merge/request.

It promotes consistency and encourages engineers to share their current understanding with teammates and future contributors.

Gitlab allows its users to setup description templates inside their repository, read the steps to make a merge-request template below. You can also setup various other kinds of templates inside GitLab. You can do the same in GitHub as well.

In a project, you can choose a default description template for new issues and merge requests. As a result, every time a new merge request or issue is created, it’s pre-filled with the text you entered in the template.

Below is an example template that we’ve made for this article, but every team should first discuss and collaborate to come up with a template that’s right for their use case and team dynamics.

You can also read Semantic Commit Messages on GitHub to get more information as attached below:

2. Document the What, Why & How

This is where you should describe the What, Why and How of your merge request.

• What problem is being solved?
• What benefit is this change providing?
• Why is this change being made or feature being added?
  

OR you can answer three simple questions:

• What is in the Merge Request?
• How did you do it?
• Why did you do it this way and Why do you want it reviewed?

It’s important to note that the Why is significantly more important than the What here. 

An engineer may be able to reason/give rationale on what was done by reading the code, but they may never know why a change was made if the reason isn’t documented.

For Example, the following merge request description/comment doesn’t tell a reviewer much more than the code's intended purpose:

In contrast, the description/comment below explains to the reviewer and future engineers why the change was made and why it was done that specific way.

3. Explain the changes

This is where you should describe the HOW of your merge request.

• How was the solution or feature implemented?
• What new classes, files, or modules were created or modified to implement the solution or feature?

By explaining your implementation approach and the changes you’ve made, you can save your reviewer the time they would spend reverse engineering your code.

4. Give Additional Context

Sometimes, the changes in a merge request don’t convey the amount of investigation and effort that went into debugging an issue. Occasionally the solution to a hard problem is a change of a few lines, but giving additional context and background to explain to your reviewer and to future engineers how and why the one line change fixes the problem will go a long way in saving everyone's time.

Remember that anyone in your organization (or outside of it, if your repository is public) could be reading your merge request. Don't assume that the engineer reading your merge request will be as familiar with the problem as you are.

5. Include testing & verification steps

Before the bug or feature goes for further testing to the product or testing team. It is the engineer's responsible for ensuring quality in all of their features. it is very important that we document how to manually test user-facing features that require complex setup, or how to manually test changes that aren't user-facing at all. This will allow product managers, testers and even designers to go through the same steps as you did in your testing. This would help the feature/fix/improvement to go from development to launch faster.

Save your reviewer time by clearly documenting testing and verification steps in the merge request.Checkout how the request description example under the section "Explain the changes" can be improved further by using the Merge Request Template under section "Use a Standard Merge Request Template":

The above example not only provides a better context but also links file paths to each module concerned with the merge request, making easier for the reviewer to go directly to a specific file/folder. It also provides testing notes with page URLs, making it easier for the reviewer and testers to check those changes and verify them on their end. Additionally, if the author has forgotten to test a URL/page, those can be identified quickly.

6. Show your work

If you're committing a frontend and customer-facing change? If so, adding a screenshot or a GIF allows the reviewer to see the new functionality and gives them higher confidence that your change works as expected. MacOS supports gif capturing natively, you can also use Gifox and GIPHY Capture as well.

You can also attach Loom videos for this purpose as well, using Loom for Code Reviews is one of their key use cases and it increases context and clarity for the reviewer. You can checkout their website for more information (Loom for Agile Engineering).

Additionally, adding screen captures can allow Product Managers and Designers to review and approve the changes as well.

7. Give it a final read

After you've opened your merged request and filled out the description, it's time to review your work. Pretend you're the reviewer and walk through the changes in each file. You can provide in-line comments to grab your reviewer's attention. You also might catch a mistake or two that should be fixed before you assign a reviewer.

You can use in-line comments to proactively address something that looks out of the ordinary. If you think your reviewer will be uncertain of why a certain change was included, point out the reason. Inline comments can also be used to ask your reviewer about their opinion on a specific function or method called inside your code or your approach.

You can ask questions like

I used a for loop here, do you think forEach would be a better option?

8. Keep Merge requests small and frequent

Finally, keep merge requests at a manageable size by splitting out unrelated changes.

If you need to refactor a class in order to add a new feature, then you should develop, review, and land the refactor before working on the feature. First, this makes each MR smaller, decreasing the odds your reviewer might overlook a serious issue. Second, this will make it easier for your reviewer to confirm that your refactor is only changing structure and that behavior is unchanged.

It can be tempting to land an entire feature at once, but we would encourage you to challenge yourself to break down the work into several small merge requests. Can you add the backend services for a feature in one MR and build the frontend in another?

Keep in mind that larger MRs will likely need additional review time and attention due to their size. However, if the size is overwhelming to the reviewer, a large PR is likely to get less time and attention, which in turn would hinder your ability to learn, get feedback and might come back to haunt you after it comes back from the testing team.

Summary

Develop a process with your team to help you improve communication and documentation in your merge requests. You will find that a descriptive merge request isn't enough to ensure quality throughout your codebase. This is both the responsibility of the author and the reviewer.

In our next post, we'll move forward and write extensively about Code Reviews and its different types.