When I review a pull/merge request, I aim to figure out these main things:

• How familiar am I with the existing code?
• What is the scope of the changes?
• How bad would it be if there were an issue with the changes?
• How easy are the changes to tweak later?
• How thoroughly did I check for issues?

How familiar am I with the existing code? Obviously, I am most effective in reviewing a change when it’s in an area of code I’m familiar with. I like to tell the author my level of familiarity and tell them how deeply I understand what the change does. It provides some context for my comments. Ideally, if I’m not familiar with it, someone else who is would be available to do a more thorough review if needed—but sometimes changes just need signing off, or are quite simple anyway.

What is the scope of the changes? How much of the system does this code affect? Is it just some internal functions, or does it affect an external API or protocol? This is an important thing to know about a change, and it’s usually fairly easy to work out, even if you’re not familiar with the details. It helps you, as a reviewer, keep a bird’s eye view of the system’s changes, which can help with future bug hunting (“ah, didn’t that PR from last week change this part..?”). I don’t always tell the author my conclusions to this, but it can be useful to check with them, e.g. “if I understand right, this changes the interface in this way…”

How bad would it be if there were an issue with the changes? How critical is this part of the system? If there was a bug, would we find out quickly? Could we then fix it quickly? Would it affect our users? The more impactful or harder-to-detect a potential bug is, the more effort should be spent on review.

How easy are the changes to tweak later? This follows from the scope question. The wider the scope of the change, the harder it is to go back and change our mind later; what would, at review stage, be a small tweak to a new database schema or API becomes a big migration project once it’s actually out in the world. Wider-scope changes should have more in-depth review. Conversely, if the change only affects a small, internal module, it doesn’t matter that much about its API, and it’s not worth having big disagreements over it.

How thoroughly did I check for issues? Well, this is where I do any in-depth double-checking / issue hunting. Whether I should or even can do this depends on the answers to all the prior questions. Recently I have started to always tell the author how thorough I’ve been, so they know how informed I am in my review. Personally, I find it a bit disconcerting to get silent approvals on PRs and not know how much of my change people really thought about.

Answering these questions doesn’t mean all reviews are 60 minutes of deep analysis. It can just as easily be “had a quick 5min scan, not familiar with this area but looks sane. approved.”

As a final note, I find it’s really useful to make it clear which of your review comments are actual requests and which are just curiosities or suggestions. Conventional Comments is some good inspiration for this.