Best practices on doing pull requests

This article will deal with a few best practices that should increase the quality of your PRs as well as make your life easier, when using PRs.

Deploy a test build

I wrote about it before, but you should have your build server (or continuous integration server) deploy your branch-to-be-reviewed to a seperate, clean staging environment. This is where testers can have a look at your changes. This environment is a 100% copy for the production environment. The only thing missing is the real data from production (since that's sensitive data). During the deployment the CI server will populate the environment's database with seed data that is close to the data on production (and mirrors the size of production's database).

You could name this a staging or remote test environment.

Document the lifecycle in your project management tool

It doesn't matter whether you use JIRA, Trello, Asana or something else. As long as the tool integrates with your build environment and repository in a way, that you can easily jump between the ticket, the PR and any documentation on the wiki. The ticket needs to document the complete lifecycle from the creation of the new branch with every commit on it, to the PR and the CI server's build results as well as the merge and deployment to the remote-test environment and, lastly, to production. You want to have a clear audit-trail easily accessible. This is not done to be able to point fingers, but to be able to understand why things are the way they are. Even 12 months later. That's what your project management tool is responsible for.

Merging when everything is green and 👍

Merging PRs can be a story in itself. The most important thing is: Only merge when every reviewer gave you the thumbs-up and when the CI server indicates that there were no errors at all.

The merging should be done by the assigned reviewer. If there are multiple people responsible, you can talk about who should merge. It shouldn't be the author of the PR. Allowing the author to merge her own changes leads to a bad practice where people merge to quickly, without a proper review. Other than that, it doesn't matter whether the person merging is more or less senior than the author.

Where do you merge into, or: What's the base branch?

The simplest strategy is to merge a feature branch (and most other branches) into the master branch. This is usually the branch that gets deployed.
It could be, that you follow a different dev methodology, like [git-flow]. If that is the case, you will probably merge into develop. So your base branch would be develop.

No matter what your base branch, note that:

Smaller features make merging easier

The less changes you want to merge, the easier it will be to review them. You should count on the fact that development of other features will continue while you are developing your feature branch. Those might have started earlier, or there were other branches that fixed bugs. In any case, your base branch will have moved on and evolved. It had a certain state when you branched off of it, but that state won't be the same it is in right now. The longer you wait with your feature, the more work you'll have to do — after you already thought you were done — when you want to merge your changes. This can be frustrating. You wouldn't be the first one to introduce bugs at this point. Since the code has evolved, you are not yet that familiar with the new pieces. But you'll have to make your code work with them. That's where bugs are hiding.

The smaller the feature, the earlier you can merge and the less painful a process it will be.

Commit early and often with good commit messages

I wrote about it before: Let your commits be small and many. Your commits should follow the SRP (Single Responsibilty Principle). Your commit should change one thing. It might and could have side-effects. But it's purpose it to change one thing. In your commit message's subject (50 char length at max) you should succinctly describe what changes this commit introduces. Whenever you reach to the word "and", you know your commit is too big.

A clean history of 2000 commits that are each tightly scoped and with clearly written commit messages is easier to understand and more helpful than 200 commits that each contain countless changes with minimal commit messages. Bad examples for commit messages, that are too often, are "fix", "kill bug", "add stripe" etc.

The commit message serves as your audit trail to your future self. Perhaps you have to do software development for a few years, but there will be a time where you scroll through your commit messages, searching for this one commit that helps you understand why a bug occurs, or why a library was rolled back to an older version. I've seen it with enough teams, that I really suggest you start improving your commit messages.

Do not squash your commits

This might be a contested opinion, but I am against sqashing commits. Please see the point above, but your commits paint a picture and tell a story. Squashing them loses this. Sure, the changes are there, but you are not able to identify how a change came to be.

There is one situation where squashing improves things: When you commit your code, without taking care to write good commit message or how to reduce your commit sizes. Then you squash your commits in the end and write a good, detailled commit message, that adheres to the before-mentioned best practices. This does increase the quality. The history, the how is still lost, but at least you can understand that one commit. That's still better than many commits without proper messages.

There are people that do not like merge commits. I understand the reasoning, it seems to be an unnecessary commit. This is true, you can prevent this when rebasing your changes onto the base branch. This can be difficult if your repository changes very quickly.

If you do need to squash before merging (I don't know or understand why, please tell me!) please only do it right before merging. Don't do this when opening the PR. Your reviewers should be able to have a look at the history of the development of your branch.

Use templates to make your lives easier

Now that I explained why you need a well-written description and detailled commit messages, you might wonder how you can facilitate their creation.
I would propose you create templates.
We'll start with commit message templates.

Git commit message template

Inside your project folder, you'll find the .git folder. Open the .git/config file in your favorite text editor. Add the following lines:

[commit]
    # can be local to your project or global
    template = path/to/your/.gitmessage

It makes no sense, to create this for every project, so you can add this inside your global git configuration. On unix systems this can be found in

~/.gitconfig

You would add the same thing in there. I would propose you create a repository for your company or department where you store these templates. This makes it easier to update them for the whole team. Then you just clone the repository and link it correctly: template = path/to/cloned/repository/.gitmessage.

Here's a link to my Git templates.

This is a basic template that you could tweak:

# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Add co-authors if you worked on this code with others:
#
# Co-authored-by: Full Name <email@example.com>
# Co-authored-by: Full Name <email@example.com>

You would have this pasted into every commit message you create as a comment (the # signs denote comments in git messages). Then you never forget what to write about.

These templates work locally for every developer.

Pull request templates

If you use GitHub or GitLab you can create a template that will be used when somebody creates a PR in your repository. It works mostly the same as the git commit message template: Your template gets pasted into the description of the PR and you can use it as an aid to fill in the textbox. As far as I know, there is currently no way to require certain information to be filled out.

A way around that would be to have your CI server parse the PR description and complain on the PR if certain things are missing. This would mean that your developers need to be very disciplined in filling out the description, or else risk complaints by the CI server. If you would be interested in something like that, I will show how to do that in another article.

If you are using GitHub, you can find their [description on how to use a pull request template on their help pages].
GitLab offer a similar description, showing you [how to create merge request templates for their service].

Both services let you use markdown to write your descriptions. There are some useful additions to Markdown available for you. You can put them to use in your template, so your developers gradually learn to use them properly. One of the biggest features is to create checkboxes in your description (or comments). This is done like this:

- [ ] Make logo more bigger

While your checkboxes won’t be mandatory to be checked for your pull request to be accepted, they do provide a nice visual cue about any open tasks.

Merge the pull request

I said it earlier already, the person merging the PR shouldn’t be the one who opened it. This helps to enforce the principle of four-eyes (or less, depending on your developers).

Finally the PR is merged and you can work on the next feature.

🎉

How do you review a Pull Request?

The first thing you should do is read the title and the description thoroughly. Your co-worker took her time and made sure you can get all the information you need out of the description. So read it carefully. Take a look at the screenshots and videos, so you get a sense of what it looks and feels like.

The next thing you should do? Check out the branch locally and run the tests. If the PR isn't WIP anymore, the tests should all be green. On the CI server as well as on your machine. It is important to run the tests on your computer as well, because you could have a different setup with your database that could surface a hidden issue. Or you are using a different browser than your co-worker so you might see certain problems that are browser-specific. Looking at the feature on your computer provides the benefit of becoming more intimate with the changes and being able to look at them in a bigger context. The review interface on GitHub and GitLab often only show the changes and small amounts or neighouring lines. On your computer, you can see the whole glory. This is good for context.

When you find things that you want to comment on, please do this kindly. Be generous, everybody makes mistakes — even you. Write your comments as questions. This could look something like:

> I noticed you instantiate the Foo class inside the constructor. What were your reasons for doing that?

Even if you already know, that dependency injection would have been a better choice for this spot, help the other developer figure it out by asking questions. Do not tell them what to do or to change. Show them the direction, point them to places they could learn about how they could do it better, or explain why you think something might make more sense if they do it differently. The more context you provide and the more care you take to explain your questions and suggestions, the easier it will be to improve the code.

Remember: [You are not your code].

Things to look for when reviewing a PR

There things that your linter won't catch. These are long or unclear variable or function names. There might be code that is commented and can be deleted. Perhaps it was there from an earlier version of the code, as a draft. You could even find violations of the style guide, that the linter did not catch.

Perhaps your team writes documentation, like in classic docs or in a wiki. Was it updated correctly? Can you understand what was written? Is the ticket's status updated and are all references to other issues or related changes present? Perhaps you can point the author to other changes in the code, or features or resources that are related to the thing you are reviewing (mailing-list threads, help-desk/service tickets, blog posts, articles or books)?

Take a look at the commits that are part of the PR. Do they conform to your commit-message guidelines? Do they make the feature's history clear? What do you learn from scanning them? Are there any changes necessary?

Let me state again that the most important thing is to be polite, helpful and respectful of your co-workers. PRs should be a place to improve (the code and yourself) and to learn.

What role does CI/CD play in your PR?

My guideline for development workflows leans heavily on having a CI/CD server that orchestrates the whole workflow. CI/CD means continuous integration and continuous deployment. There is a lot to CI, and why and how you should do it. Look for multiple articles on that from me here. For the time being, you can get a wonderful introduction to the subject from another expert: [Martin Fowler on his site].

Your CI server should do certain steps when a PR is opened. These include:

  • Check the code for linting errors and warnings.
  • Run all automated tests, including long-running integration tests.
  • Run static (and behavioral) code quality analysis.
  • Deploy the feature branch to a test environment automatically.
  • Inform the reviewers about the results of the above steps, through a comment on the PR, and post the url of the deployed test environment.
  • Block or unblock the the PR from being able to be merged, depending on the results of the above steps.

If you make your CI server report all linting errors, you free your developers to focus on the changes in the logic. Imagine the case where reviewers wants to check your PR, and the first thing they have to do is create lots and lots of comments about linting violations. Nobody will be able to focus on the really important changes and the discussions will grow very big. So let your robots (CI server) handle that for you.

What is a pull request?

This is one of several articles on what I consider the best way to do a pull request. There are many ways to do this. I consider this to be the best way because of my experiences doing it like that with different teams. The results tell me that I am right. But first: What is a pull request?

What is a pull request, or: How should I integrate my changes with the code

To make any of this work, you need to be using a version control hosting provider like GitHub, GitLab or Bitbucket. All of them provide this functionality. Bitbucket even offers it for Mercurial repositories. I use Git with GitHub and GitLab.

Continue reading What is a pull request?