Holger's Blog

This is a guide 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 provides it for Mercurial repositories. I use Git with GitHub and GitLab.

A pull request (from here on I’ll call it PR), sometimes also called Merge Request (in GitLab) is a way to integrate the changes you made to the repository (the code base) with the master branch (that’s the code that goes live to production). I will not go into using Git too much right here, there are other articles about that. If you need a refresher on Git and branching, please come back after reading these articles.

So let’s assume you wrote your changes on your own branch, which I’ll call feature/MIH-318-my-feature-name. You already can see three things here:

• The branch name begins with the kind of branch this is. In our case we work on a new feature for our application, so we call it “feature”.
• The second thing you might notice is, the MIH-318 part of the name. This comes from our project management software. This is the short title of the ticket that the branch belongs to. More on that later.
• The last part is the name of the feature. This could also come from your project management software and be the long name of the ticket, or it could be a descriptive name of the feature you are building. Something like new-user-login-form or integrate-stripe-payment-provider.

Now it’s time to get these changes into testing (Staging environment or whatever you’d like to call it). Therefore you pushed your branch with the latest changes to your repository (origin) and open a new pull request to merge the changes into the master branch.

Why should you use PRs?

Using PRs gives you an incredible valueable feedback mechanism. This shares the responsibility of the work and makes sure you don’t break the application when things go live. This is because a PR is a great opportunity to catch bugs early on during the cycle. Catching bugs and errors earlier in the development cycle reduces the costs to fix them. After all, they didn’t interfere with other (new) code yet and are (hopefully) compartmentalized. PRs are also a good form of documentation, because of the description and discussion happening in a PR.

How do you do PRs?

Before opening a PR

I advocate for using before-commit hooks in Git. These hooks enable you to run custom scripts before commiting changes. There a quite a number of quality checks, if you ask me (and you kind-of do, since you are reading this! 😉), and almost all of them should be run before comitting your changes. I’ll mention the checks later on, as well.

Baseline measurements of a PR

One of the most important things to remember when doing PRs: Don’t let them get too big. A size I found to work well was around 400 lines changed. While this is no hard limit, going over it tends to incorporate too many changes at once. This makes the review harder for the reviewers.

Another thing not to lose track of, is the number of days a PR is open. You should review PRs as soon, as they are assigned to you, or when you are being mentioned in their description (or in comments). It is your duty not to let your team wait for too long on your feedback/review. If there are no major errors in the changes, a 400 lines feature should be reviewed and merged within 1–2 business days.

Who do you assign a PR to? Who should review?

This depends very much on your team structure and how you are organized. If it is possible, it would be great to get two people to review your changes. A healthy mix could be one person from your immediate team, and another one from a different team. This can have the benefit of a different perspective. The one from the outside-team probably doesn’t know too much about your feature and the specifics of your code. This forces you to be extra detailled when writing your description and documentation and they might not make some assumptions that you and your team take for granted. This situation needs to be handled carefully, you don’t want to get derailed by fundamental questions. But it could also prove to be beneficial to your feature, ymmv. It also increases knowledge across team boundaries, which I consider to be good practice.

If cross-team doesn’t work for you, try to get a more experienced (or same level as you) and a less experienced person from your team. While both reviewers will help you improve your result, the less experienced might get a chance to learn from reviewing your code.

In any case, @mention your reviewers in the description and (if possible in your software) assign the PR to the one you want to be responsible for merging.

What should you write in the description of a PR?

The first thing that you should write is a good title. This is the first thing that reviewers see, and it usually also goes into the emails they get about the PR. So make sure they understand what it is about. This is also a good place to let the reviewers know if you only want a review, without the goal of merging the PR. Perhaps you have an early draft of your feature, and want guidance on the general direction. You can do that by prefixing your title with something like

WIP: Integrate Stripe as payment provider

This WIP (work in progress) makes your intentions clear. If you are using GitHub, common practice is to use labels for that. So you could have a “Do not merge” label, or again a “WIP” label. I tend to always write it into the title as well, so people can’t miss it.

The next point to take care of is the description. This is where you should spent most of your time, when creating a PR. Consider writing about the following points:

• What is changed?
• Why was the change necessary?
• How does the project / part of the code / microservice look after making your changes?
• Are there any side-effects of your change?
• Do you want to add any commentary about your changes? Perhaps link to Stack Overflow answers that helped you, or blog articles and books you referenced during development?
• Is there anything unclear with your changes? Perhaps you have a road-block that you need help with? Or you are uncertain how your changes interfere with another component? Ask about that. This is the place to do that.

If it makes sense, you should add screenshots of the before/after situation. If your changes contain animations or important visual effects, provide a gif or a short screencast of your changes.

Finally, make clear what you want the reviewers to look for, test or what they should comment on. If they follow this guide, they will look for more than that, but it is always appreciated to get a little guidance from the author of a PR.

Don’t forget to link/reference the issue/ticket in your management software that this feature/PR belongs to.

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.

How do you review a PR?

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.

Best practices on doing pull requests

This section 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.

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.

🎉

Yesterday I’ve streamed for another 2 hours. While I started with a “Just Chatting” segment, I quickly turned to programming again. Not least because I was only chatting with myself 😂.

Eventually I started with implementing an algorithm for “rolling weeks” in my chores app. Have a look to find out what that is about 😉

www.twitch.tv/videos/20…