HOW TO: Reviewing a Pull Request

I feel like reviewing a pull request (PR) is a bit like when you're told to write an essay at school/university. It's such a fundamental part of being a developer of any form when working in a team, and yet it's rare than anyone actually sits down and explains to you how to review a PR effectively. Much like at school, you're told to "write an essay on.." but have you been told how to write an essay?!

So, I can't quite sit down with you and talk you through how to review a PR but instead I'll share my learnings with you - a mix of reading, experience and asking my seniors! Whilst PRs will all be different, there are a few key things to look out for...

Check the PR comments

Hopefully, the person who has raised the PR will have given it a helpful name and written something in the description so you know what you're looking out for. There might be a specific area of focus or it might be general. But, usually it can be a helpful pointer to what you should be looking for.

Code Quality and Standards

No matter what your level is, if you're reviewing a PR you are there to review the standards of your code base. Yes, you might phrase or ask things differently as a junior reviewing a senior's work, but yes you should definitely be checking on those things. That's one of the points of a PR!

It's good to check for both internal standards and industry wide/language standards. A lot of these will hopefully be picked up if you have a CI pipeline which initiates such checks, but if not you're that check mechanism (and even if there is, pipelines aren't infallible and don't always check for everything!)

When looking at standards, you can check things like the presence and quality of doc strings, typos, extra white spaces and variable naming conventions (if you're working in Python this might be whether you're adhering to PEP8 standards, or even just sense checking variable names are helpful.) You might also be looking for clarity/conciseness here - has a whole new function been written that replicates what a built in function does? Is that necessary or should it be cleaned up?

Code Logic

Typically, you should only be reviewing code that's part of a code base you've been working on so that you understand what's happening when you look at the PR. This will be helpful when you're checking the code logic, though it isn't an absolute must. In theory, you should be able to follow well written code whether you know what it's meant to be doing or not, but if you're unfamiliar with the code it makes things a bit more tricky.

With the logic, you want to be checking that the code is doing what it is expected to be doing. Is the data being manipulated in a logical way? Do the steps of the function make sense? Are the flow control constraints going to act as expected/have they been applied correctly?

These are all things that can't easily be checked by testing, because code can still function even if it's not doing the right thing.

Does the Code Work

So, you shouldn't *have* to be checking this in a PR ideally, but it comes down to how much you trust that your team members are doing this check. Even the tiniest change to your code might break something so before any PR is raised the code should be run and checked.

Sadly though, this isn't always the case and so you might want to run this check for yourself as an added precaution. Worst case, you've wasted a few minutes running something that works fine but by doing this you could save hours of time whilst other people have issues or even problems in production depending on which branch the PR is going to be merged to.

These tips hopefully will help you to feel more confident when reviewing PRs, but can also be used to help you raise high quality PRs if you know what is being looked for from the reviewers perspective. It feels like there's a lot to do, but with time it gets easier. Even practicing raising PRs on projects you work on yourself and reviewing them is great experience to help prepare you for working in a team.