Troubleshooting your code reviews
Google engineers contribute thousands of open source pull requests every month! With that experience, we've learned that a stuck review isn't the end of the world. Here are a few techniques for moving a change along when the frustration starts to wear on you.
You've been working on a pull request for weeks - it implements a feature you're excited about, and sure, you've hit a few roadbumps along the review path, but you've been dealing with them and making changes to your code, and surely all this hard work will pay off. But now, you're not sure what to do: your pull request hasn't moved forward in some time, you dread working on another revision just to be pushed back on, and you're not sure your reviewers are even on your side anymore. What happened? Your feature is so cool - how are you going to land it?
Reviews can stall for a number of reasons:
- There may be multiple solutions to the problem you're trying to solve, but none of them are ideal - or, all of them are ideal, but it’s hard to tell which one is most ideal. Or, your reviewers don't agree on which solution is right.
- You may not be getting any response from reviewers whatsoever. Either your previously active reviewers have disappeared, or you have had trouble attracting any in the first place.
- The goalposts have moved; you thought you were just going to implement a feature, but then your reviewer pointed out that your tests are poor style, and in fact, the tests in that whole suite are poor style, and you need to refactor them all before moving forward. Or, you implemented it with one approach, and your reviewer asked for a different one; after a few rounds with that different approach, another reviewer suggests you try… the approach you started with in the first place!
- The reviewer is asking you to make a change that you think is a bad idea, and you aren't convinced by their arguments. Or maybe it's the second reviewer who isn't convinced, and you aren't sure which side to take in their disagreement - and which approach to implement in your PR.
- You think the reviewer's suggestion is a good one, but when you sit down to write the code, it's toilsome, or ugly, or downright impossible to make work.
- The reviewer is pointing out what's wrong, but not explaining what they want instead; the reviews you are receiving aren't actionable, clear, or consistent.
These things happen all the time, especially in large projects where review consensus is a bit muddy. But you can't change the project overnight - and anyway, you just want your code to land! What can you do to get past these obstacles?
Step Back and Summarize the Discussion
As the author of your change, it's likely that you're thinking about this change all the time. You spent hours working on it; you spent hours looking through reviewer comments; you were personally impacted by the lack of this change in the codebase, because you ran into a bug or needed some functionality that didn't exist. It's easy to assume that your reviewers have the same context that you do - but they probably don't!
You can get a lot of mileage out of reminding your reviewers what the goal of your change is in the first place. It can be really helpful to take a step back and summarize the change and discussion so far for your reviewers. For the most impact, make sure you're covering these key points:
What is the goal of your pull request?
Avoid being too specific, as you can limit your options: the goal isn't to add a button that does X, the goal is to make users who need X feel less pain. If your goal is to solve a problem for your day job, make sure you explain that, too - don't just say "my company needs X," say "my company's employees use this project in this way with these scaling or resource constraints, and we need X in order to do this thing."
There will also be some goals inherent to the project. Most of the time, there is an unstated goal to keep the codebase maintainable and clean. You may need to ensure your new feature is accessible to users with disabilities. Maybe your new feature needs to perform well on low-end devices, or compile on a platform that you don't use.
Not all of these constraints will apply to every change - but some of them will be highly relevant to your work. Make sure you restate the constraints that need to be considered for this pull request.
What constraints have your reviewers stated?
During the life of the pull request until now, you've probably received feedback from at least one reviewer. What does it seem like the reviewer cares about? Beyond individual pieces of feedback, what are their core concerns? Is your reviewer asking whether your feature is at the appropriate level because they're in the middle of a long campaign to organize the project's architecture? Are they nitpicking your error messages because they care about translatability?
Try to summarize the root of the concerns your reviewers are discussing. By summarizing them in one place, it becomes pretty clear when there's a conflict; you can lay out constraints which mutually exclude each other here, and ask for help determining whether any may be worth discarding - or whether there's a way to meet both constraints that you hadn't considered yet.
What alternative approaches have you considered?
Before you wrote the code in the first place, you may have considered a couple different ways of approaching the problem. You probably discarded the paths not taken for a reason, but your reviewers may have never seen that reasoning. You may also have received feedback suggesting you reimplement your change with a different approach; you may or may not think that approach is better.
Summarize all the possible approaches that you considered, and some of their pros and cons. Even better, since you've already summarized the goals and constraints that have been uncovered so far during your review, you can weigh each approach against these criteria. Perhaps it turns out the approach you discarded early on is actually the most performant one, and performance is a higher priority than you had realized at first. The community may even chime in with an approach you hadn't considered at all, but which is much more architecturally acceptable to the project.
List assumptions and remaining open questions
Through the process of writing this recap, you may come to some conclusions. State those explicitly - don't rely on your reviewers to come to the same conclusions you did. Now you have all the reasoning out in the open to defend your point.
You may also be left with some open questions - like that two reviewers are in disagreement without a clear solution, or that you aren't sure how to meet a given constraint. List those questions, too, and ask for your co-contributors' help answering them.
Here's an example of a time the author did this on the Git project - it's not in a pull request, but the same techniques are used. Sometimes these summaries are so comprehensive that it's worth checking them into the project to remind future contributors how we arrived at certain decisions.
Pushing Back
Contributors - especially ones who are new to the project, but even long-time members - often feel like the reviewer has all the power and authority. Surely the wise veteran reviewing my code is infallible; whatever she says must be true and the best approach. But it's not so! Your reviewers are human, and they want to work together with you to come to a good solution.
Pushing back isn't always the answer. For example, if you disagree with the project's established style, or with a direction that's already been discussed and agreed upon (like avoiding adding new dependencies unless completely necessary), trying to push back against the will of the entire project is likely fruitless - especially if you're trying to get an exception for your PR. Or if the root of your disagreement is "it works on my machine, so who cares," you probably need to think a little more broadly before arguing with your reviewer who says the code isn't portable to another platform or maintainable in the long term. Reviewers and project maintainers have to live with your code for a long time; you may be only sending one patch before moving on to another project. So in matters of maintainability, it's often best to defer to the reviewer.
However, there are times when it could be the right move:
- Your reviewer's top goal is avoiding code churn, but your top goal is a tidy end state.
- When you try out the approach your reviewer recommended, but it is difficult for a reason they may not have anticipated.
- When the reviewer's feedback truly doesn't have any improvements over your existing approach, and it's purely a matter of taste. (Use caution here - push back by asking for more justification, in case you missed something!)
Sometimes, after hearing pushback, your reviewer may reply pointing out something that you hadn't considered which converts you over to their way of thinking. This isn't a loss - you understand their goals better, and the quality of your PR goes up as a result. But sometimes your reviewer comes over to your side instead, and agrees that their feedback isn't actually necessary.
Lean on Reviewers for Help
Code reviews can feel a little bit like homework. You send your draft in; you get feedback on it; you make a bunch of changes on your own; rinse and repeat. But they don't have to be this way - it's much better to think of reviews as a team effort, where you and the reviewers are all pitching in to produce a high-quality code change. You're not being graded!
If implementing changes based on the feedback you received is difficult - like it seems that you'll need to do a large-scale refactor to make the function interface change you were asked for - or you're running into challenges that are hard to figure out on your own - like that one test failure doesn't make any sense - you can ask for more help! Just keep in mind the usual rules of asking for help online:
- Use a lot of detail to describe where you're having trouble - cite the exact test that's failing or error case you're seeing.
- Explain what you've tried so far, and what you think the issue is.
- Include your in-progress code so that your reviewers can see how far you've gotten.
- Be polite; you're asking for help, not demanding the author change their feedback or implement it for you.
Remember that if you need to, you can also ask your reviewer to switch to a different medium. It might be easier to debug a failing test over half an hour of instant messaging instead of over a week of GitHub comments; it might be easier to reason through a difficult API change with a video conference and a whiteboard. Your reviewer may be busy, but they're still a human, and they're invested in the success of your patch, too. (And if you don't think they're invested - did you try summarizing the issue, as described above? Make sure they understand where you're coming from!) Don't be afraid to ask for a bit more bandwidth if you think it'd be valuable.
Get a Second Opinion
When you're really, truly stuck - nobody agrees and your pushback isn't working and you met with your reviewer and they're stumped too - remember that most of the time, you and your reviewer aren't the only people working on the project. It's often possible to ask for a second opinion.
It may sound transactional, but it's okay to offer review-for-review: "Hi Alice, I know you're quite busy, but I see that you have a PR out now; I can make time to review it, but it'd help me out a lot if you can review mine, too. Bob and I are stuck because…." This can be a win for both you and Alice!
If you're stuck and can't come to a decision, you can also escalate. Escalation sounds scary, but it is a healthy part of a well-run project. You can talk to the project or subsystem maintainer, share a link to the conversation, and mention that you need help coming to a decision. Or, if your disagreement is with the maintainer, you can raise it with another trusted and experienced contributor the same way.
Sometimes you may also receive comments that feel particularly harsh or insulting, especially when they're coming from a contributor you've never spoken with face-to-face. In cases like this, it can be helpful to even just get someone else to read that comment and tell you if they find it rude, too. You can ask just about anybody - your friend, your teammate at your day job - but it's best if you ask someone who has some prior context with the project or with the person who made the comment. Or, if you'd rather dig, you can examine this same reviewer's comments on other contributions - you may find that there's a language barrier, or that they're incredibly active and are pressed for time to reply to your review, or that they're just an equal-opportunity jerk (or not-so-equal-opportunity), and your review isn't the exception.
If you can’t find another person to help, you can read through past closed and merged PRs to see the comments – do any of them apply to your PR, and perhaps the maintainer just hasn’t had time to give you the same feedback? Remember, this might be your first PR in the project but the maintainer might think “oh I’ve given this feedback a thousand times”.
When to Give Up
All these techniques help put more agency back in your hands as the author of a pull request, but they're not a sure thing. You might find that no matter how many of these techniques you use, you're still stuck - or that none of these techniques seem applicable. How do you know when to quit? And when you do want to, how can you do it without burning any bridges?
What did I want out of this PR, anyway?
There are many reasons to contribute to open source, and not all of us have the same motivations. Make sure you know why you sent this change. Were you personally or professionally affected by the bug or lack of feature? Did you want to learn what it's like to contribute to open source software? Do you love the community and just feel a need to give back to it?
No matter what your goal is, there's usually a way to achieve it without landing your pull request. If you need the change, it's always an option to fork - more on that in a moment. If you're wanting to try out contribution for the first time, but this project just doesn't seem to be working for it, you can learn from the experience and try out a different project instead - most projects love new contributors! If you're trying to help the community, but they don't want this change, then continuing to push it through isn't actually all that helpful.
Like when you re-summarized the review, or when you pushed back on your reviewer's opinions, think deeply about your own goals - and think whether this PR is the best way to meet them. Above all, if you're finding that you dread coming back to the project, and nothing is making that feeling go away, there's absolutely nothing wrong with moving on instead.
Taking a break
It's not necessary for you to definitively decide to stop working on your PR. You can take a break from it! Although you'll need to rebase your work when you come back to it, your idea won't go stale from a few weeks or months in timeout, and if any strong emotions (yours or the reviewers') were playing a role, they'll have calmed down by then. You may even come back to an email from your long-lost disappeared reviewer, saying they've been out on parental leave for the last two months and they're happy to get back to your change now. And once the idea has been seeded in the project, it's not unusual for community members to say weeks or months later, talking about a different problem, "Yeah, this would be a lot easier if we picked up something like <your abandoned pr>, actually. Maybe we should revive that effort."
These breaks can be helpful for everyone - but it's best practice to make sure the project knows you're stepping away. Leave a comment saying that you'll be quiet for a while; this way, a reviewer who may not know you feel stuck isn't left hanging waiting for you to resume your work.
Forking
For smaller projects, you often have the option of forking - a huge benefit of using open source software! You can decide whether or not to share your forked change with the world (in accordance with the license); "forking" can even mean that you just compile the project from source and use it with your own modifications on your local machine.
And if you were working on this PR for your day job, you can maintain a soft fork of the project with those changes applied. Google even has a tool for doing this efficiently with monorepos called Copybara.
Bonus: if you use your change yourself (or give it to your users) for a period of time, and come back to the PR after a bit of a break, you now have information about your code's proven stability in the wild; this can strengthen the case for your change when you resume work on it.
Giving up responsibly
If you do decide you're done with the effort, it's helpful to the project for you to say so explicitly. That way, if someone wants to take over your code change and finish pushing it through, they don't need to worry about stepping on your toes in the process. If you're comfortable doing so, it can be helpful to say why:
"After 15 iterations, I've decided to instead invest my spare time in something that is moving more quickly. If anybody wants to pick up this series and run with it, you have my blessing."
"It seems like Alice and I have an intractable disagreement. Since we're not able to resolve it, I'm going to let this change drop for now."
"My obligations at home are ramping up for the summer and I won't have time to continue working on this until at least September. If anybody wants to take over, feel free; I'll be available to provide review, but please don't block submission on my behalf, as I'll be quite busy."
Avoid the temptation to flounce, though. The above samples explain your circumstances and reasoning in a way that's easy to accept; it's less productive to say something like, "It's obvious that this project has no respect for its developers. Further work on this effort is a waste of my time!" Think about how you would react to reading such an email. You'd probably dismiss it as someone who's easily frustrated, maybe feel a little guilty, but ultimately, it's unlikely that you'd make any changes.
You Are the Main Driver of Your PR
In the end, it's important to remember that, ultimately, the author of a pull request is usually the one most invested in the success of that pull request. When you run into roadblocks, remember these techniques to overcome them, and know that you have power to help your code keep moving forward.
By Emily Shaffer – Staff Software Engineer
Emily Shaffer is a Staff Software Engineer at Google. They are the tech lead of Google's development efforts on the first-party Git client, and help advise Google on participation in other open source projects in the version control space, including jj, JGit, and Copybara. They formerly worked as a subsystem maintainer within the OpenBMC project. You can recognize Emily on most social media under the handle nasamuffin.
