Vox

Leitura de 19 min

4 Ways to Be a Better PR Reviewer

Summary

This episode of Developer Tea focuses on improving the pull request review process, which host Jonathan Cattralli identifies as a fundamental part of software engineering collaboration. He argues that engineers typically review more code than they write, making effective reviewing a core job duty that’s fundamentally about communication rather than just technical correctness.

Cattralli presents four practical strategies for becoming a better PR reviewer. First, he recommends reviewing on a “sliding scale of importance,” where reviewers should only block pull requests for issues that will have immediate negative consequences, while treating other suggestions as advisory comments that don’t prevent merging. This approach preserves developer autonomy while still providing valuable feedback.

The second strategy involves asking open questions that invite discussion rather than yes/no answers. Questions like “how did you test this?” or “what ways do you see this failing?” encourage deeper understanding of the developer’s thought process, though Cattralli cautions against questions that might lead to unnecessarily long PR discussions that would be better handled in a quick call.

Third, reviewers should create a shared goal of resolving the PR quickly—either by shipping or closing it—rather than using the review as a platform to voice every possible opinion. Understanding the context and urgency of the PR helps avoid unnecessary friction. Finally, Cattralli emphasizes distinguishing between factual observations and personal opinions in comments, clearly labeling opinions as such while letting objective data (like performance benchmarks or team coding standards) speak for themselves.

Recommendations

Tools

  • Headspin — Sponsor of the episode, described as a mobile testing platform that unifies end-to-end automated testing, performance monitoring, and user experience analytics for any application on any device and network.
  • GitHub — Mentioned as an example of modern version control systems that facilitate the PR process with features like line-specific comments and direct suggestion acceptance.

Topic Timeline

  • 00:00:00 — Introduction to pull request collaboration — Jonathan Cattralli introduces the episode’s focus on the pull request review process as a key engineering collaboration method. He explains that pull requests are how code changes happen in teams and that engineers typically review more code than they write. The episode will focus on the reviewer’s perspective, framing PR review as fundamentally about communication rather than just technical evaluation.
  • 00:04:00 — First strategy: Review on sliding scale of importance — Cattralli presents the first strategy: review on a sliding scale of importance. He explains that many reviewers mistakenly believe they must approve only perfect code with no suggested changes. Instead, he recommends only blocking PRs for issues with immediate negative implications (like syntax errors or clear bugs) while treating other suggestions as advisory comments. This approach increases developer autonomy while still providing guidance.
  • 00:07:51 — Second strategy: Ask open questions — The second strategy involves asking open questions that can’t be answered with yes/no. Questions like “how did you test this?” or “what ways do you see this failing?” invite discussion and reveal the developer’s reasoning. Cattralli cautions against questions that might lead to excessively long PR discussions, suggesting that extended conversations might be better handled in a quick call rather than in the PR comments.
  • 00:10:39 — Third strategy: Create shared goal of resolution — Cattralli presents the third strategy: create a shared goal of resolving the PR quickly by either shipping or closing it. He notes that the most common mistake is reviewing without understanding the PR’s context, which can lead to reviewers adding excessive comments just to have their voice heard. Understanding context helps avoid unnecessary friction and keeps the process moving toward resolution.
  • 00:12:45 — Fourth strategy: Distinguish facts from opinions — The final strategy is to use factual observations and opinions explicitly without blurring the lines. Cattralli explains that many “should” statements in PR comments are actually personal opinions rather than objective requirements. He recommends clearly labeling opinions as such while letting objective data (like performance benchmarks or team coding standards) speak for themselves. This clarity prevents confusion about what feedback is mandatory versus optional.

Episode Info

  • Podcast: Developer Tea
  • Author: Jonathan Cutrell
  • Category: Technology Business Careers Society & Culture
  • Published: 2020-05-18T09:00:00Z
  • Duration: 00:17:24

References

Podcast Info

Transcript

[00:00:00] What exactly does it mean to collaborate as an engineer with other engineers?

[00:00:13] Certainly there are times when we are talking about our code, and there are times when we

[00:00:17] are pair coding, but in today’s episode I want to talk about another process that doesn’t

[00:00:23] get as much attention as it deserves, the pull request process.

[00:00:28] My name is Jonathan Cattralli and you’re listening to Developer Tea.

[00:00:30] My goal in this show is to help driven developers like you find clarity, perspective, and purpose

[00:00:35] in their careers.

[00:00:37] Pull requests are a huge part of almost any engineer’s job.

[00:00:41] If you don’t know what a pull request is, you probably will soon, but essentially a

[00:00:46] pull request is a request to change the code base in some way.

[00:00:53] When we started back at the very beginning of the history of Git, the version control

[00:00:58] system Git, a pull request was literally a request that you would send to someone, maybe

[00:01:05] in an email, to pull your code, to pull a branch that you had pushed up to the Git server

[00:01:11] and review it on their machine.

[00:01:12] Now this has evolved and in basically every modern version control system you have tools

[00:01:17] to facilitate this PR process.

[00:01:19] For example, on GitHub you can add a comment to a specific line of code.

[00:01:24] You can also add suggestions and even accept those suggestions directly into your branch.

[00:01:31] So what makes a good pull request process is an important question because this is essentially

[00:01:37] how code gets changed.

[00:01:40] In a team of any sufficient size, you’re going to have code that you write in your own branch

[00:01:48] of the code base and then you’re going to submit some kind of pull request.

[00:01:53] This is essentially showing the difference between your code and what was previously

[00:01:58] there.

[00:02:00] There’s two sides to this equation and perhaps more, but at least two.

[00:02:04] One is the submitter and the other one is the reviewer.

[00:02:09] In today’s episode, we’re going to be talking about the reviewer side and there’s a good

[00:02:13] reason that we’re putting this first.

[00:02:16] You are going to review, most likely, far more code in your career than you will write.

[00:02:21] This is just a function of the fact that you work with more people than likely just one.

[00:02:28] For all of those people who have code to review, it’s likely that you will end up reviewing

[00:02:33] more code than you actually write yourself.

[00:02:37] If you were to ask a software engineer what their primary job duty is, they would say

[00:02:43] writing code and it makes sense, right?

[00:02:47] But actually, we review more than we write, so our primary job duty, in some ways, can

[00:02:54] be seen as reviewing code.

[00:02:56] Now, of course, if you listened to the last episode, we keep on saying the word code over

[00:03:01] and over and in that episode, we talked about how we perceive code and this is even one

[00:03:08] layer deeper or further away from the code itself.

[00:03:12] This pull request process is talking about the code, so really everything you’re going

[00:03:18] to hear on today’s episode, this is kind of a cheat code, is about communication.

[00:03:24] Communicating in a situation where you are presenting or proposing something and then

[00:03:30] asking for feedback, that is essentially what a pull request is.

[00:03:35] So all of the advice that I’m going to give you today about how to be a better pull request

[00:03:40] reviewer is basically communications advice.

[00:03:45] It’s not just about pull requests.

[00:03:48] You can apply the same principles that we’re going to be talking about in any communication

[00:03:52] situation.

[00:03:55] So I’m going to give you four practical ways that you can become a better reviewer.

[00:03:59] So let’s jump straight in.

[00:04:00] Number one, review on a sliding scale of importance.

[00:04:05] Review on a sliding scale of importance.

[00:04:08] Let me explain what I mean by this.

[00:04:10] Too often a review of code gets held up by minor change requests.

[00:04:15] These are things that sometimes are even considered just about your opinion.

[00:04:19] You don’t really have good data to explain why you would want this particular change

[00:04:24] and it might be something that is kind of over optimizing or maybe it’s just a slightly

[00:04:30] different way of doing what they’re doing.

[00:04:33] So you’re suggesting this in the PR because it seems like the right place to suggest

[00:04:37] it and here’s the critical error that a lot of people make.

[00:04:41] They believe that the bar for acceptance or kind of the bar for approving a PR is I have

[00:04:49] no comments to make.

[00:04:52] In other words, in order for me to approve this PR, I need to look at it and accept that

[00:04:58] it is exactly the way that I think it should be in the end, that there’s no changes that

[00:05:04] I would suggest making and I’m ready to merge it in right now as if it were my code.

[00:05:13] If you consider making the change requests that you have advisory only, in other words,

[00:05:20] don’t block the PR, don’t require those changes to be made before you approve the PR.

[00:05:27] You leave the power in the hands of the engineer to decide what they will do about it.

[00:05:33] It’s not just your opinion as the reviewer that should be listened to.

[00:05:38] For example, my personal rule, my personal default rule is to only block if I know the

[00:05:44] submitted code will have an immediate or near term negative implication.

[00:05:49] For example, if there’s obviously a missing bracket or something like if there’s a very

[00:05:54] clear bug, syntax error or a very clear logic bug that I know about, then I’ll block the

[00:06:01] PR because I know if they merge this, it will have a very immediate negative implication

[00:06:07] or if I know that there is a clear performance issue that’s going to immediately or very

[00:06:13] close to immediately cause a problem that I might block then, but I provide feedback

[00:06:19] on anything else that seems questionable to me even after I approve the PR.

[00:06:25] In other words, I’ll approve the PR and leave those comments just as kind of advisory comments,

[00:06:31] things that I think we could consider but don’t necessarily have to keep us from merging

[00:06:37] this code as it is.

[00:06:39] This is kind of like the cliche saying that we hear about improv, the yes and.

[00:06:45] You provide validation about a direction and you provide additional comments and direction

[00:06:52] beyond that instead of trying to control everything or reduce the developer’s kind of autonomy

[00:06:58] by saying that the PR can’t go in until they follow your exact rules.

[00:07:05] You increase autonomy and you provide direction and opinion for what it is not to be clear.

[00:07:12] Some of these comments are going to have a little bit more of an engineering manager

[00:07:17] slant to them.

[00:07:18] If you are a lead developer, you might have a little bit more kind of input and perhaps

[00:07:24] even more ownership over the code.

[00:07:27] You might even be mentoring one of the other developers directly in a technical capacity

[00:07:34] and the way that these comments come together might be a little bit different.

[00:07:38] So we’re kind of leaving room for that here.

[00:07:41] Understand that that’s kind of my bias coming into this.

[00:07:44] Number two.

[00:07:45] Number two, right?

[00:07:46] So number one was review on a sliding scale of importance.

[00:07:48] Number two, ask open questions.

[00:07:51] Ask open questions, assuming the engineer considered what you are about to bring up

[00:07:55] already.

[00:07:57] What is an open question?

[00:07:59] An open question is one that can’t be answered with a yes or a no, as an example.

[00:08:05] Open questions invite discussion and they invite opinion.

[00:08:10] A simple example of this is, did you test this?

[00:08:14] This is a yes or no question and it’s likely to lead to a very simplified answer about

[00:08:21] a potentially nuanced question.

[00:08:24] You might get much better information if you ask, how did you test this?

[00:08:29] Or what ways do you see this failing?

[00:08:34] Now I want to give you a word of caution here because I can hear some developers groaning

[00:08:38] because they don’t want to write for hours in their PRs.

[00:08:42] So be careful with asking open questions that will lead to long discussions that aren’t

[00:08:46] necessarily relevant directly to this PR, right?

[00:08:50] It may make sense.

[00:08:51] For example, if your discussion in the PR is going back and forth for too long, it might

[00:08:56] make sense to schedule a phone call and talk about it rather than spending a lot of time

[00:09:01] going back and forth in the PR.

[00:09:03] But asking these kinds of open questions allows the person to explain what their strategy

[00:09:09] was and provide some background as to the choices they made.

[00:09:15] We’re going to take a quick sponsor break and then we’re going to come back with the

[00:09:17] other two suggestions I have for becoming a better reviewer.

[00:09:22] Today’s episode is sponsored by Headspin.

[00:09:25] Headspin for mobile unifies end-to-end automated testing, full stack performance monitoring

[00:09:31] and user experience analytics.

[00:09:34] Headspin is made for any application, whether it’s native or on the web, running on any

[00:09:39] device, on any network, anywhere in the world.

[00:09:43] With the patented global device infrastructure that offers real SIM enabled devices on real

[00:09:48] Wi-Fi and carrier networks with 24 seven remote access.

[00:09:53] This totally leaves behind that big drawer full of mobile devices that you’ve been trying

[00:09:58] to keep around and instead it’s going to replace it with a much more mature solution, including

[00:10:05] for example, AI powered analysis, right?

[00:10:09] And this will track your user experience metrics and your KPIs over time.

[00:10:13] You can have cold and warm starts, errors, crashes, response times.

[00:10:17] You can track audio and video quality and you can even track biometric responsiveness.

[00:10:22] This is truly a full end-to-end automated testing solution.

[00:10:27] Go and check it out.

[00:10:28] Headspin.io, that’s Headspin.io.

[00:10:32] Thanks again to Headspin for sponsoring today’s episode of developer T.

[00:10:37] All right, let’s talk about number three.

[00:10:39] Going back, number one was review on a sliding scale of importance.

[00:10:43] Number two is ask open questions and assume that the engineer considered what you’re about

[00:10:48] to bring up already.

[00:10:50] Number three, number three, create a shared goal of resolving the PR either by shipping

[00:10:57] or closing.

[00:10:59] This is the goal of any PR.

[00:11:02] The goal is to come to some resolution.

[00:11:06] It’s not to explain yourself thoroughly.

[00:11:10] It’s not to understand our differences.

[00:11:14] That’s not the point of a PR.

[00:11:16] The most common mistake I’ve seen in PR review is the engineer reviewing does so without

[00:11:21] knowing the context of that PR.

[00:11:25] In this scenario, an engineer might accidentally adopt the goal of adding every possible comment

[00:11:31] that they can to pick apart the PR so that their voice is heard, so that their opinion

[00:11:38] is well known.

[00:11:40] This can be extremely frustrating for the original submitting engineer, especially if

[00:11:45] they’re on a tight deadline or if they plan to improve the code in a follow-up pull request.

[00:11:51] Make sure you, as the reviewer, understand as much as you can about the context of the

[00:11:56] PR before tying it up with a lot of friction in that process.

[00:12:02] Once you understand the context, then using that context to resolve the PR as quickly

[00:12:07] as possible.

[00:12:09] For example, if you see the PR going way off in the wrong direction, it might make more

[00:12:14] sense to close the PR and open a new one entirely.

[00:12:20] This concept is to resolve quickly rather than leaving this thing out in the open for

[00:12:25] an extremely long amount of time.

[00:12:29] Fourth and final, so going back over the first three, review on a sliding scale of importance,

[00:12:34] ask open questions assuming the engineer considered what you are about to bring up already, create

[00:12:39] a shared goal of resolving the PR either by shipping it or closing it, and finally, use

[00:12:45] Use factual observations and opinions explicitly, but don’t blur those lines.

[00:12:52] Use factual observations and opinions explicitly, but don’t blur those lines.

[00:12:59] There’s no one perfect way to write code.

[00:13:01] That shouldn’t come as a surprise.

[00:13:03] Hopefully, if you’re listening to this podcast, you know that this is a highly opinionated,

[00:13:09] highly personal, highly contextual job that we do.

[00:13:13] It’s easy to confuse, though, the things that we believe about code and the things that

[00:13:19] we think are factual.

[00:13:22] The ways that we prefer to write our code versus some other objective standard, a requirement

[00:13:31] to write that code that way.

[00:13:33] And this shows up in PR comments when we say you should do it this way instead.

[00:13:40] When we say you should do it this way instead, a lot of the time, that should carries a lot

[00:13:46] of weight.

[00:13:47] Where is the should coming from?

[00:13:49] If you can back it up with some context and some data, then the should might make a better

[00:13:56] argument.

[00:13:57] But a lot of the time, the should is actually simply an opinion.

[00:14:04] And if you call it out as an opinion, then it’s much better for the context of a PR.

[00:14:10] If you say, I personally like to do it this way, and then explain why you like to do it

[00:14:15] that way, someone else might say, well, I appreciate that you like to do it that way,

[00:14:20] but I have a different way of doing it, and here’s my reasoning.

[00:14:25] Once again, don’t spend wheels too long in the context of a PR talking about the different

[00:14:30] ways that you like to do things.

[00:14:33] If you think it’s going to be valuable to that person to consider the way that you might

[00:14:37] do it, then that’s a useful comment to make.

[00:14:41] But if it’s just beating the same dead horse, for example, if that person already knows

[00:14:46] your opinion, and if they still want to do it their own way, and you haven’t found a

[00:14:51] common ground, then don’t spend time talking about the same thing that you’ve already talked

[00:14:56] about.

[00:14:57] On the flip side, if you’re not talking about opinion, but instead, if you’re talking about

[00:15:02] some objective measure, for example, using this method rather than this method produces

[00:15:09] 10 times better performance with this particular benchmark.

[00:15:12] Well, this is hard data.

[00:15:14] This is information that you’re providing, and you’re allowing the information to speak

[00:15:19] for itself.

[00:15:21] Another type of information that kind of speaks for itself is if you as a company or as an

[00:15:27] organization, engineering organization, or even as a team, if you have all kind of agreed

[00:15:33] on or if you’ve established, whether you agree with it or not, if you’ve all established

[00:15:39] a particular coding style and a PR goes outside of that coding style, this is objective information.

[00:15:48] Even though it may be a collectively held opinion, if everyone on a particular team

[00:15:55] is complying with that opinion, then it doesn’t really make sense for one person not to.

[00:16:01] And so instead of constantly saying that the team’s opinion is X, Y, or Z, you can probably

[00:16:08] explicitly state that this code goes outside of the kind of agreed upon guidelines.

[00:16:17] These are four ways that I think you can become a better reviewer.

[00:16:20] In the next episode of Developer T, I want to talk about how you can submit better PRs.

[00:16:27] How can you get your PRs reviewed faster?

[00:16:30] How can you avoid errors and difficult problems as a result of merging your PR prematurely?

[00:16:38] What are some of the best practices around being the submitter in that process?

[00:16:44] Thank you so much for listening to today’s episode of Developer T. Hopefully this was

[00:16:48] helpful and if it was, or if you know someone who would benefit from listening to this episode,

[00:16:52] I think a lot of developers would, then I would encourage you to share it with them

[00:16:57] directly.

[00:16:58] This is a wonderful way to help Developer T continue growing as a podcast and reaching

[00:17:03] new developers every day.

[00:17:05] Thank you so much for listening.

[00:17:06] Thank you again to Headspin for sponsoring today’s episode.

[00:17:09] Head over to Headspin.io to get started today.

[00:17:12] Today’s episode was produced by Sarah Jackson.

[00:17:14] My name is Jonathan Cottrell.

[00:17:15] And until next time, enjoy your tea.

Visão de gráfico