I’ve worked on a whole lot of GitHub-hosted projects, whether personal, open-source, or on contract. Sometimes using the public GitHub, and other times GitHub Enterprise. But one thing is the same: it’s pretty easy to submit a pull request, but it’s really hard to review a pull request well.
Without further ado, the top ten pull request review mistakes and some ideas on how to do better!
It’s so tempting. The pull request is really big, and the submitter is someone you trust. They’ve been working in this part of the code for a while, and it has always worked out well. Not to mention that you have your own deadlines to hit!
+1 LGTM Ship it!
Snap out of it!
You need to spend real time to review that code. Everyone makes mistakes - seniority level is no magical ward against them. And your role, as the reviewer, is to use your creativity and expertise to reduce the chance that this pull request makes the codebase worse in any way.
That’s really the goal, isn’t it? If every pull request makes the codebase better, maybe the project really has some long-term potential!
Why review it now? After all, it really is a big pull request. And your current task is too important. You’ll eventually get around to it, right? Or, maybe you’ll just wait for others to chime in…
Search your feelings! Let the force flow through you! You might have some very real complaints behind that resistance.
Now that you’ve identified your real concerns, take action!
Are you reviewing gibberish? The default diff view on Github and GitHub Enterprise is ‘Unified.’ In this mode, to render a set of changes to a file, the software looks at added and removed lines and attempts to group blocks of changes intelligently, all inline. But you know what? In most cases, ‘Unified’ diffs are very hard to read. That intelligent block selection really isn’t.
The good news is that both GitHub and GitHub Enterprise support ‘Split’ diffs. On the left is the old file, and on the right is the new file. You’ll see empty sections on the right if code was removed, or on the left if code was added. Either way, you can clearly see what the file looked like before and after, leading to better review decisions.
Don’t settle for the gibberish. Click on ‘Split’ in the top-right of the diff.
Very little time, if any, should be spent discussing code style and formatting during a pull request review. I’ve written before about the need to use tools like ESLint to make these kinds of things completely automated. Why? Because it’s a waste of time!
A good code reviewer spends the time to try to understand the ultimate goals of the code changes, by going back to the original requirements. Is there a work item tracking this? A spec? What exactly was it asking for?
Only with that context can true review happen. What may look reasonable during a superficial structure/style review can become unacceptable when the ultimate goal is understood.
Yes, you might shy away from bringing up ‘big’ things like this because so much time has already been spent on the existing changes, but it’s worth talking about better solutions. It’s an opportunity to learn for everyone. You might even be wrong in your belief that there’s a better solution, but it takes a discussion with the original submitter to figure that out.
Diffs are really great for showing you what has changed. But that’s the thing! By definition they don’t show you what hasn’t changed. Be on the lookout for changes which should have been applied more widely, like a find/replace that maybe didn’t cover the entire codebase.
Or a change that only hit a subset of the components it should have.
Or entirely missing tests. Tests are an important part of any change, but it’s actually very easy to forget about them if they’re not in the diff at all. You won’t be prompted to think about them.
I’ll admit, this is really hard! This is the hardest kind of review. It might help to do some quick sanity check searches in either the submitter’s branch or whatever you have on your own machine. Or you could ask the submitter about what kinds of comprehensiveness checks they’ve done beyond the code changes you can see.
Once there are some test code updates in the pull request, it’s easy to get lulled into a false sense of security. If they put some tests in, they must be high-quality and comprehensive. Right?
Testing is an art. It takes a lot of context to properly balance risk mitigation against testing cost, as appropriate for the area of the code and culture of the team. Pull request reviews are a great place for the team to build that shared context.
Some questions to consider:
If a change is in the CSS and HTML, the inclination is to treat it like an algorithmic code change. You see well-formed changes and imagine what they would do in the browser. “Seems reasonable,” you say.
But it’s not so simple. What the user ultimately sees comes from the complicated interactions between your application and various rendering engines.
Get out of your head, and pull the branch down. Try it in multiple browsers and screen sizes, because this stuff is really tricky. Even if you’re an expert frontend developer, don’t trust yourself to eyeball this stuff. This is why CodePen and the like exist!
This is another area where you can be lulled to sleep by well-formed code in the diff. But it’s important to consider the large scale. With this new code now in the project, what changes? What could happen?
Some questions to get you started:
On some pull requests there is quite a bit of back and forth, maybe due to disagreements or just the need for clarification. At long last the set of you directly involved with the pull request have agreed on a way forward - you’ve built shared context!
But what happens when the next developer comes along next week, next month, or next year, and encounters this code? They won’t have easy access to this discussion.
Some ideas to build accessible context for the future:
TODOs in the code are paired with an item in your work item database, with enough detail to make it actionable by someone other than the original reporter.
Finally! The pull request has seen some lively attention, and it’s back in the submitter’s court. You’ve given your feedback, and the submitter is making some changes in response.
Now is not the time to forget about the pull request. You’ve discussed needed changes with the submitter, but that doesn’t mean those changes will be correct! Or even made at all!
Pull request amendments are some of the highest risk changes a developer will ever make, because everyone just wants to move on. Less care given in development, less care given in review.
Look especially closely at any updates to the original pull request, even though, yes, you’ve already reviewed the pull request comprehensively. If the new changes are separated into their own commits, this is easier. If the whole pull request has been newly rebased/squashed, then this can be a bit frustrating.
It’s a hard job to design and implement software. Why would it be any easier to review it? In fact, review is probably harder because you have a whole lot less time to build up the right context to provide reasonable feedback.
But we can’t give up - it’s extremely important!
Use this article as a starting checklist, or an inspiration for one. Over time, you and your team can build up a custom list of reminders for important but easily-forgotten considerations. Eventually, your pull request process will become a powerful feedback loop for improving your team’s culture and code quality.
Remember AngelFire? It was 20 years ago when I signed up to host my first little web page, establishing my initial presence on the internet. I’ve spoken before about my blogging progression, but... Read more »
I’ve been told that I’m a very productive developer. And I want you to have my techniques! Welcome to my fourth developer productivity tip: Find a test rhythm. Stop worrying about test philosophy... Read more »