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.