Pull Request Best Practices

Let’s pretend you don’t know this (for the sake of article structure). When developers complete a new feature, bug fix, or other update, they submit a pull request (in version control systems like GitHub and GitLab) to merge their changes into the project's main branch. This process allows other team members to review the changes, discuss potential improvements, and suggest edits before integrating the code into the larger project.

Pull requests provide a structured platform for code review, facilitate collaboration by allowing multiple developers to contribute to the same project, serve as a record of why specific changes were made, and help maintain a high level of code quality.

You know exactly what your code does and why you have made the changes, but the reviewer does not. Your commits should contain a commit message that makes it clear why the changes were made.

It is beneficial to follow the Conventional Commits Specification, which is as follows:

<type>[optional-scope]: <description> 

Example: 
feat(shop-api): add voucher validation

While individual commit messages are important, pull requests provide an overall view of the changes being committed. It is crucial to have a name and description for the PR that provides proper context, which is why many organizations implement a standard template.

At Crystallize, we have a standardized template for pull requests in open-source projects to ensure consistency and that all the necessary information is provided. The template includes:

• Type of the commit (e.g., breaking change, feature, bug fix)
• Reference to the Shortcut ticket, if applicable
• A description of what the PR is doing

This makes the review process easier and more efficient.

| Q | A | 
| ------------- | ------------ | 
| Branch? | main / x.y.z | 
| Bug fix? | yes/no | 
| New feature? | yes/no | 
| BC breaks? | yes/no | 
| Fixed tickets | #... | 

<!-- - Please fill in this template according to the PR you're about to submit. - Replace this comment by a description of what your PR is solving. -->

I remember being on a call with our CTO Sébastien once, and he mentioned how he reviews his PRs to weed out the usual suspects, like a stray console.log() forgotten to be deleted somewhere. Reviewing your PR saves time for the reviewer. This can also help you figure out if something in your code is too ambiguous. You can add more explanation for it via comments in the code or within the pull request itself.

You can use liners to establish a standard set of rules for how the code should look. You would also usually set up linting checks as part of your GitHub workflow. This ensures the code formatting is consistent throughout the project, regardless of who is working on it.

For example, all the Crystallize open-source repositories have defined styling rules. Before creating a PR, we run prettier to ensure the code being pushed is clean. The prettierrc.json file looks like this. Of course, your organization might have different preferences.

{ 
  "tabWidth": 4, 
  "semi": true, 
  "useTabs": false, 
  "bracketSpacing": true, 
  "bracketSameLine": false, 
  "singleQuote": true, 
  "trailingComma": "all", 
  "printWidth": 120 
}

All is well. You have a PR, and you have assigned a reviewer. It is completely fine for anyone on the team to review it, but what if you have a monorepo with several projects? Not everyone will likely be familiar with every project in the mono repo.

Take Crystallize, for example. We have several APIs—Catalogue, PIM, Core Next, Shop, etc.- and certain developers are involved with specific APIs. Having a code owner for each project is a faster and much easier way to have code reviews.

So, if I create a PR for the Shop API, I know the best person to ask for a review here. As the code owner will know their way around the project, it speeds up the review process.

To define code owners in GitHub, you create a file named CODEOWNERS and assign owners as follows:

apps/api/shop @plopix 
libs/shop-api @plopix

Ideally, you would have small pull requests, which makes it easier for the reviewer to go through the PR, but sometimes you might be working on a big feature. In cases like these, it is helpful to have draft PRs. A good use case is when the PR is a work in progress, but you push commits to it to get a second opinion, allowing people to do a review at the current stage the code is in and give you feedback.

Another thing that helps immensely to avoid pushing unreviewed code is having protected branches. Usually, our projects require at least one review and passing all the checks in place before merging is enabled. For example, we have a check where only a code owner is allowed to merge PRs. This additional check is good to have in place so you don’t merge something that needed to be properly reviewed before.

In the image below, merging is blocked until at least one reviewer approves the request.

The organization of your commits also plays an equally important role in telling the story of your changes. A well-structured pull request (PR) enables reviewers to grasp the evolution of your code easily.

But what if you need to make adjustments after committing changes? It is not an issue! As developers, we have the power to refine our commit history through rebasing. This allows us to reorganize, rewrite, or combine commits, ensuring a coherent narrative.

Another helpful technique is squashing. By condensing multiple commits into one, squashing can significantly enhance the clarity of your PR. Here are some key benefits of squashing:

• Simplified review process: Reviewers can focus on the final state of your changes without getting bogged down in intermediate steps.
• Cleaner commit history: Your main branch remains uncluttered, making understanding the project's evolution easier.
• Easier rollbacks: Reverting a single, comprehensive commit is simpler than undoing multiple minor changes if issues arise.

While rebasing and squashing are powerful tools, it is important to use them warily. Generally, you should preserve commits representing distinct logical changes or significant milestones in your development process.

For those unfamiliar with these concepts, here's a quick comparison: While merging integrates branches by creating a new commit, rebasing rewrites commit history by applying your changes on top of the target branch. This results in a linear, more readable history.