14: Ep. 14 | Mastering pull requests

Erika (00:00) Welcome to the Overcommitted Podcast where we discuss our code commits, our personal commitments, and some stuff in between. I’m your host, Erica, joined by…

Jonathan Tamsut (00:09) Hi, I’m John.

Bethany (00:10) Hey, I’m Bethany.

Brittany Ellich (00:11) and I’m Brittany.

Erika (00:13) We are software engineers who initially met as a new hire group at GitHub and found a common interest in continuous learning and building interesting projects. We can even meet to share our learning experiences and discuss our lives as developers. So whether you’re pushing code or taking on new challenges, we are happy you are listening. Today’s episode focuses on pull requests, covering reviewer mindset, crafting effective pull requests, and managing the sometimes

overwhelming review workflow. But before we do that, we want to do a quick recap of Brittany’s trip to Berlin last week. Last episode, we talked about conferences and we are so excited to hear about how your experience was, Brittany.

Brittany Ellich (00:56) Awesome. Thank you. Yeah, I think it will have been two weeks ago by the time this episode airs But yes, we I went to Berlin for gophercon EU. It was a great conference met a ton of people This was the first like large ish conference I was talking to people at and I was presenting at and there was I think around 300 attendees plus people online and It was great. It was honestly a really great experience. It’s very exhilarating slash terrifying

to talk to that many people at once, but I really enjoyed it and I met a ton of great people. Highly recommend speaking at conferences even if it is scary to do.

Erika (01:36) weird.

Jonathan Tamsut (01:36) If the

listener wants to go watch her talk, how would they do that?

Brittany Ellich (01:40) I don’t think that the recording link is out yet, but if this is the future, I will definitely be linking it from my website. So BrittanyEllick.com. It’ll be there. And I’ll try to share it in the show notes if it’s available by the time this episode is out, but probably not. I think it’ll be like a month or so.

Erika (01:58) Well, happy to have you back and excited to see all of the amazing things that you’re doing speaking at conferences. Also, today’s theme very much picks up on some of your recent blog content and we’re going to talk about some of the same topics. you’ve made some great blog posts about pull request reviews starting with your mindset and

Talking about how impact on pull request of you is really the most important thing to focus on. It’s sort of easy to let your ego get in the way or even if you don’t intend it to, some of your stylistic preferences.

to sort of hold up a pull request. But you mentioned.

always asking yourself when you’re reviewing, what impact is this having? Am I unblocking my colleague? Am I making the code better by providing this suggestion or this review? And you mentioned going through four phases, which I really like, of a review. You think about the big picture, thinking about whether the code is actually even solving the problem. You think through the logic of the

application, is there any loopholes or misses in the logic? And then you think about maintainability. So how is the naming? How is the testing? Will I understand this in six months to a year to two weeks from now? And then also remembering that any pull request is.

sort of a digital diary where if you have any questions, the pull request is the perfect place to ask them because it’s always a place that you could refer back to. And that’s sort of a way to capture those decisions and yeah, any clarifications that need to happen for the changes.

So I kind of wanted to open it up to the rest of the group and Brittany if you want to add anything to that of our review processes and what we tend to do as far as start to finish reviewing pull requests and things that we think about.

Jonathan Tamsut (04:04) So, you know, one thing pattern I’ve like observed in my career is like, you know, I think developers often have feedback, but can’t really explain or give reasons why. So like maybe something aesthetically isn’t good or, you know.

your code’s not organized, but you can’t really, like, know, oftentimes, you know, I’ve received feedback that’s like, I don’t really like this, especially earlier in my career. And I think that one of my rules is like, unless I can really say like, hey, this is my reason why I’m going to explain to you why I try not to.

give feedback. mean, there’s instances where maybe things are a little ugly or just like, I don’t know, it’s kind of just like an accepted best practice. So I think that’s a big thing. And I think oftentimes, know, and I think, that’s like a good learning experience and oftentimes engineers just have a tough time really putting it towards why.

something should be different. the best code reviewers I’ve seen are great at this, are like, yeah, actually, like there’s a standard library method that’s faster and blah blah, you whatever. And those are like the code reviews that I’ve appreciated most.

Bethany (05:10) Yeah, I completely agree. And I love this concept of impact is what you’re looking towards for your benefit of reviewing a PR. And I think to the point like John’s making about having points that you can explain or maybe consistent things that you comment or things that your team just agrees on is better. I think it’s

the best thing to try to offload as much of those to CI as possible. So if you have something consistently that you want to make sure is done, such as, for instance, my team does comments above all public exported functions, that’s part of our linting rules. So we don’t have to go in and say, hey, can you add a comment to this? Can you add a comment to this? But rather, CI grabs it. And so I think anything you can

delegate to CI is awesome. And so your true impact is determining if the code changes are correct and if they’re maintainable. So I really like that thought process of what is your impact in this review process?

Jonathan Tamsut (06:12) I just want to say I really, really, really like what Bethany just said. think automating that stuff with CI is like something probably more of us should think about.

Brittany Ellich (06:21) Yeah, I think that if there’s anything that is important enough stylistically to comment on, then it should be a linting rule for your team and not something that you necessarily have to think about ideally. I think that there are also valid cases, particularly in my case, I had to learn both Ruby and Go working at GitHub. I hadn’t used either of them and there’s definitely ways to do things better in Ruby since it’s, you know, there’s just a million ways to do everything. And I really appreciate when people reach out and say,

like actually doing it this way is, you know, faster or more efficient or whatever. Because I know that I’m just never going to be able to take the time to learn it that deeply and I appreciate those. But if it’s just a stylistic thing, I don’t think it’s worth mentioning unless there’s an actual tangible value from it.

Erika (07:03) I’m curious, Brittany, do you have a PR review template or something that you use personally to keep track of your thoughts through these stages?

Brittany Ellich (07:13) Not really. I add comments as I go. I’ll do, you know, like I said, investigate the PR description and make sure that, you know, I have like a big picture understanding of what’s happening. And then just as I go, I’ll just add comments. And so my comments will be sort of all over the place as reviewing a PR, but maybe I should make one. That’s a great idea.

Erika (07:34) Yeah,

I’m impressed you can keep that all in your head at the same time. I definitely use a template to keep track of my reviews because otherwise I think it’s really easy to gloss over things. I think hearing about your like sort of four-stage approach is helpful in like thinking of the layers.

But I, yeah, I typically go through the same questions, but sort of like, yeah, through a template that I take notes on. And the other thing I look at is like,

sort of along the lines of like the PR is documentation where like the PR description sometimes isn’t even accurate to what is going on and like we typically expect to see observability or feature flags or anything described in the PR.

description, but honestly a lot of times I don’t see that and a lot of times the description gets really outdated with additional feedback. yeah, it’s a it’s sort of an underutilized documentation aspect, I think.

Jonathan Tamsut (08:51) Eric, I’m curious what’s in your template.

Erika (08:53) Yeah, I mean, I’m happy to share it out in the show notes too. It’s a lot of these same questions, like, you know, what is this code doing? Sort of like, is the logic sound? Are there any potential performance or like security impacts? And then like, how do I test this or monitor it? So those are kinds of the kind of questions that I ask.

Brittany Ellich (09:13) Yeah, we should definitely share that. I would love to see it.

Erika (09:16) Yeah, for sure.

Bethany (09:17) I, before we move on, I also think that oftentimes reviewing PRs aren’t necessarily just for stamping a PR, for saying, this is okay, but also for learning. If you’re new to a code base, think reading PRs is one of the most helpful things you can do to learn about.

how others on your team review your code, what kind of style they prefer, what’s happening in the code, and things like that. And I think, at least in the start of my career, I was always scared to review or ask questions because I would think, I’m not qualified to review this PR, or I’m not, I don’t know what’s happening, so I’m not well-suited for this PR.

But in reality, asking questions is one of the best ways you can learn and also flag things that might not be understandable that the PR is introducing. So it only helps the PR author and yourself and your team with that documentation to ask questions and just try to understand what’s going on and point out things where it might not make sense.

Erika (10:19) Yeah.

Jonathan Tamsut (10:20) There. I’m sorry, where’d go?

Erika (10:22) say the flip side of it is too I wish that I could claim that I’ve never had ego in reviewing a pull request but I recognize it in myself when I go to a pull request and I don’t have any feedback and I always have this voice in my head where I ask myself like what am I even doing if I don’t have any feedback on this PR like if all I’m doing is giving it a check mark like am I a valuable reviewer but some some pull requests

Like that is all they need. Like all they need is a stamp and it’s okay to like give it that and move on with your life. Yeah, you don’t have to like always have feedback on everything.

Jonathan Tamsut (10:58) Well, I’ve seen PRs and I’ve been, you know, unfortunately like had my PRs reviewed where they just get beaten to death. Like I think there are some people who are very thorough, but who are kind of reluctant. You know, I mean, we all have different, you know, personalities, whatever. And like some people are really reluctant to give you the stamp, especially if they’re a domain expert and there’s like, there can be like, you know, 30 back and forths. In which case I think, you know, like.

doing something synchronous is better. But yeah, think like, know, where you stand kind of matters because sometimes you don’t have anything to say. Sometimes you have always like one more thing you could say. And I think it’s, you know, I think impact as Brittany put it is important. Like you don’t wanna hold someone back, you know, nothing’s ever gonna be perfect. Your code base is constantly gonna evolve. So yeah.

Erika (11:45) Yeah, Brittany kind of mentions this as well in one of her posts of your job as a reviewer is not to catch every bug or potential problem, but to be a thoughtful safety net. And I think that’s a really great description of the job of a reviewer. And it also like calls to attention sort of like what you mentioned, Bethany, about sort of this fear of reviewing or what you were talking about of like, you know,

people being really reluctant to approve of PR. I think it’s also dependent on the culture of your team and I’ve unfortunately heard people sort of…

tribute blame to a PR reviewer for something that didn’t go well, like a PR that went through and, you know, it’s like, well, this person approved it. And so it’s like partially their fault that it caused this problem or this incident or something. And like, you know, in a good team culture, like it’s blameless and we all share the responsibility. So, you know, that kind of stuff doesn’t happen. But unfortunately, I have heard that.

And so in some places, yeah, you are kind of thought of as a responsible individual when you approve a PR.

Brittany Ellich (12:54) Which is not how it should be. At all. Yeah. Yeah, that sucks.

Erika (12:55) Right.

Well, awesome. So let’s move on to the act of creating a pull request. So on the side of the code contributor.

How do we make sure that our PRs are easily reviewed? And why would we even care about this? We sort of talked about this as the reviewer side, but having a PR that’s ready to review and that sort of meets these checklist guidelines that we’ve been talking about can help reduce that back and forth and get your PR through faster.

some of the things that Brittany mentions, which I definitely do and try to do in my PRs, is answering the questions. What problem does this solve? How did we solve it? And then calling to attention anything that our viewers focus on. And you also call out test steps, which is great and important for that documentation and also for viewers.

So any other thoughts from the group on checklists that you do for your own PR? What kind of thought process you go through when you’re something for review?

Bethany (14:06) I always like to, one, make sure that all the CI has passed, there’s no red or anything like that because I think that’s often kind of, it’s not the worst thing as a reviewer but it means that you’ll need to probably re-approve it or re-review it in the future so I want to make sure it’s always as complete as possible. I’ll also definitely give a read-through

of the PR, but also using copilot is super helpful to give a different perspective. Even it’s automated, so it can catch a lot of those very small things that you might have missed or you might have braced past. So that’s something that I’ve been really, really liking. I also will comment on my own PR if there’s something that might be confusing or that I’m not sure of or I want to invite discussion.

for, so I’ll leave a comment saying, hey, this is why I chose this. If you have any other thoughts or suggestions, please tap in or leave anything below. And I find that a lot easier of introducing maybe pros and cons in those discussions rather than including it in the description of the PR itself.

Brittany Ellich (15:10) Agree on all those points. really like having, making sure, I guess you already mentioned the things that I like to have in a PR description, but particularly if there’s a PR that has a lot of back and forth, I find that those can also get outdated pretty quickly. So making sure that before you merge that if there’s anything large that changes about the logic that it’s reflected in the PR description, I think is helpful. And then definitely plus one on doing like a pre-review before actually submitting it for review.

Erika (15:10) together.

Brittany Ellich (15:37) I think that has saved me a lot of time, think, in finding really obvious things if I’m looking at it through the lens of if this was not my code, what would I say about it, or what areas would I be confused about, and adding a comment there. I’ve found that that’s really helpful.

Jonathan Tamsut (15:52) I think when you’re writing a PR, even before you write code, I think the goal of a PR should be, one goal should be obviously to solve whatever problem, you should do it in a way that reduces the cognitive load of the developer. So think a huge thing is your PR should be as small as possible. I think that’s a philosophy. The smallest code change you can make,

And like even if it’s, I think sometimes it’s annoying to like solve a problem over three PRs, but I think that’s always better or…

most of the time better. So I think making your PR small and then yeah, like you all have been saying, if there’s a little bit of logic that’s complicated, either think how could you make it simpler or if it’s as simple as you can reasonably make it. Add comments, add documentation, links in your code review, add links to issues that give context. I think all those things are.

Thanks.

Erika (16:43) Yeah, sorry for feeling your thunder, of reading through your post, but they’re really good. starting phase.

Brittany Ellich (16:43) So I agree.

Thank you. Yeah, this is great content.

Cross pollination here. I agree with John in theory, making small pull requests is great, but I find myself in practice. really depends on how long it takes to merge something. if I think that there’s like an optimal ratio, how long it takes, how much pain there is with getting something over the line and how large those PRs get, which is probably a good sign to, you know, invest in the developer.

experience of your CI process, but you know if it’s a long process to land something then I find myself sneaking more things into PRs just because I can make one change instead of multiple.

Erika (17:28) Yeah.

Bethany (17:28) Whenever

that happens, I’ll sometimes stack my PRs to still logically break it up into separate things, but maybe merge the last one into the other one and continue on with that. sometimes it’s…

solves that problem in a way that you can break down that complexity for any reviewers, but still get it in in one go for if that’s your deploy process. But in practice, do I do that? Well, not really.

Erika (17:56) Yeah, think thinking through what to include in the description too, or what to call out, I think that sort of calling out sort of the meta of the implementation, so like really zooming up and like talking through how it’s actually used is super helpful because I kind of try to think of

Somebody reviewing this who has zero context. Somebody from a different domain area who has no constant context on the project or the problem I’m trying to solve. What would they need to know to understand the change and the implications? But then also those detail things like if I’m updating a query, what…

are the performance implications of that update or if I’m changing some logic, what’s the impact of that on its downstream dependencies? So I think that that’s, yeah, we think of.

the pull requests for the reviewers and that’s obviously important but also like anyone else looking at this change because there are also references when something goes wrong you know when incidents happen

we look through the changelog and look at the PRs that may have been submitted recently that could have caused some kind of problem. And a lot of times the PRs are nothing that I have worked on or have any context on. So yeah, being super clear about where this fits in the overall picture I think is…

something I try to do, I may not be perfect at it, because I think sometimes you have so much context that it’s hard to zoom out. But Copilot can be helpful there, sort of as that rubber duck and being like, hey, what could be confusing? What would somebody need to know looking at this who has never approached this code before or something like that, like that sort of external voice?

we can move on to our last topic then of the review workflow and how to manage all of the review requests that come in. yeah, Brittany, you mentioned sort of senior engineers.

PR review workflow often taking 20 to 30 % of the time, your working time, which is a lot. That’s almost a third. So how do we manage all of the review requests that come in? yeah, what are the notification strategies or systems that…

we use. Brittany, you mentioned three really great suggestions in your post. Would you like to go over them this time?

Brittany Ellich (20:30) yeah, I need to think back to what exactly those were. I think the one that has worked the best for any team that I’ve been on is having a specific Slack channel for review requests. I feel like I come back to that on almost every team because I think every team has this struggle of, you know, making sure that things are reviewed in a timely manner. and I think having a Slack channel is fantastic. I think that a lot of the notifications, particularly from GitHub, if that is your PR reviewing tool,

can be really overwhelming, particularly, you know, working there. There’s even more that are, that you get. You get just like endless notifications. And there’s a really great article that I shared from Ben Balter on how he manages his GitHub notifications that I always refer back to. whenever somebody’s asking for, you know, tips on it, that’s a really great resource. But…

I think that, yeah, the dedicated Slack channel has been the most successful for me and just having folks reach out when they’re blocked. I’m never bothered by somebody DMing me and saying, hey, can you look at this again? Because it’s a lot easier than trying to keep track of every single thing that your team needs to review.

Jonathan Tamsut (21:35) And I find reviewing PRs is like, especially if it’s a problem or code base you’re not familiar with, is really difficult. It’s like a high cognitive effort task. You’re like reading something. There’s also like, I also, you know, I wonder if like the UX of PRs on GitHub, like.

could be improved. Or one thing that I think I often end up doing is like, and maybe there’s just a simple way for me to change the settings here, I’ll read the, it shows the diff, but sometimes it doesn’t always show the surrounding code. So I’ll have to open up the surrounding code either locally or wherever and look at stuff if you wanna understand the logic. But I think, I actually really like the idea of blocking out PR time.

Because I think that A, helps you just maintain focus and then B, of gets you in the mindset of, I have my coffee, I’m gonna think really hard, gonna really, what are the implications here to the various parts of the system? Because I find PR reviewing, I mean, I can spend hours PR reviewing and it’s just really hard. You can spend a lot of time on one PR if you really drill down.

Erika (22:43) Yeah, it’s always more draining than I think it’s going to be. I always think of it as kind of like a low effort task, but it honestly takes almost as much, if not more, like you said, cognitive effort, depending on the PR and your familiarity with the space, but.

Yeah, it can be really draining and not always the most enjoyable task. yeah, definitely sort of blocking out some time is a way to make sure that you get the reviews done that you need to. Another strategy you mentioned, Brittany, is to set expectations with your team. like letting them know

your capacity at any given moment if you might get to it in the next 24 hours or you need somebody else to jump in and help. Because yeah, sometimes as the contributor you might be looking for a review really quickly or…

you you might be kind of left hanging and you don’t know why. so having that communication back and forth of, hey, I can’t get to this, can somebody else jump in? Or yeah, specifically asking certain people for reviews can be helpful.

Brittany Ellich (23:56) Yeah, one thing I found really helpful there is to include a little bit of context when I’m posting a PR for review in our dedicated Slack channel on the size of it. So if it’s something really small, then I’ll say, hey, this is, you know, a text change PR just so people know going into it, like, okay, this isn’t going to take a ton of time. can.

do this really quickly and unblock her. Versus if it’s something that requires a lot of effort, then I’ll also post this as a chonker. Like, take some time to review this one. I think that that’s really helpful.

Bethany (24:23) I think two things that help me. Sometimes it is easier to just talk things out, especially if it’s a complex PR. So sometimes comments get long or there’s context or empathy that’s lost in translation through just text. So a lot of times I’ll review via like a Zoom meeting or something similar.

And of course it’s important to document anything that happened in between there in your PR, but sometimes it’s a little more effective to get your point or your intentions across or hear what somebody had in mind. Additionally, I like to state PRs as part of my work, mean, like we’re talking about right now, but in standup updates, if…

spend a significant chunk of time reviewing PRs, I’ll say that, because that’s part of the job. And you’re not just graded on how you code or how much you code, but also your impact for the team. So I think that’s still a measurable thing. And if there’s a PR that you’ve reviewed that you’re proud of, adding it to your Bragg docs too is really nice for future reflection and demonstration of your impact.

Brittany Ellich (25:30) Yeah, I think it’s also really helpful just related to this, um, in terms of setting those expectations to remember that the thing that you’re, the thing that is most important here is getting the work out the door and over the line. so, you know, sometimes that expectation is also, Hey, I’m going to fix this in a follow-up PR instead of, you know, fixing it in this case. Um, and I think that that’s something that if you have, a big fan of team agreements and I feel like that is something that is good to talk about as

team and have a team agreement. Like if something’s not going to break something, being okay with, you know, refactoring and making something more readable in a follow-up PR or, you know, changing the way that we’re logging something in a follow-up PR. Same with having team agreements on how, what the expectations are and how long it takes to get a review so that you know when it’s a good time to start bugging people. And, you know, if you just post something and it’s only been a couple hours, you know, you might have to wait unless it is really urgent and you need some

to approve it right away.

Erika (26:27) Bethany, it’s really cool hearing you talk about your approach to pull requests now compared to what you were describing with your early in career mindset of like the confidence and pride that you have in your pull request reviews. So that is a, I think a very cool illustration of progress and development and yeah, what a cool journey.

Bethany (26:47) that’s so kind, thank you.

Erika (26:48) Yeah,

awesome. Well,

Let’s move on to our final segment. So today, since we’re talking about pull requests, it’s a big part of the job. And I know that we all have some pet peeves and things that we really appreciate in a pull request. So we’re going to go around and state what our pet peeves are, either

Well, I guess when reviewing a PR, yeah, things that really bug you. But you have to follow it up with something that you always love to see, either in a review or in a PR. Any volunteers to go first?

Bethany (27:30) I can go. Okay, my biggest pet peeve is when somebody requests changes with like no… Okay, so when you request a change, if nobody has done this on GitHub before, it basically blocks the PR from going through until the same person re-approves it. And so it’s not necessarily that I think you should always…

approve PRs, but just if you press request changes either I think that’s only necessary when either the PR is going to go out unless you press that button and block it or there’s something severely wrong that you need to make sure is fixed beforehand and you concretely describe what those changes are. So I would say that’s that’s my biggest pet peeve. And then

When something’s just readable and thoughtfully documented and crafts the story, I feel like a lot of times we’re just kind of throwing out PRs there and I really love when there’s something that’s very, like you tie the value to it very concretely. So that would be my praise.

Brittany Ellich (28:33) Bye.

Okay, I think my biggest pet peeve is when I open a PR and see like a ton of reviewers, know, like six plus reviewers and 30 plus comments already on something. I feel like that occurs when something’s just been.

you know, lingering too long. And I think that the more people piling onto it, probably the less, the less helpful it will be to get that thing over the line. But I do really appreciate when that does happen. Cause I’ve been in that position before where I’m just like, please just everybody like, appreciate the feedback, but please let me get this out the door. I appreciate when the author then, you know, sets up a synchronous meeting to get everybody on the same page and says, you know, identify as much things are actually critical.

in which things could be moved off to another change in the future. Getting things over the line is something I appreciate.

Jonathan Tamsut (29:22) My peeve is large PRs. I actually have like kind of a horror story where at a company I worked at previously, Not GitHub, we like hired this person who was supposed to be like, you know, a coding guru who was gonna, he was like our director of engineering who’s gonna like elevate all of our code. And he spent like three months on a refactor on some branch and…

and then ended up like not really communicating to us what he was doing. And then he was just like, okay, I have this PR and it was just like, you know, this monstrosity of a PR. And we’re just like, okay, like this fundamentally changes our data model. Like we’re not even sure if this works. Like, so yeah, definitely don’t do that. And then praise, you know, I like it when people say,

knit when they specify this is a knit, know, or this is, you know, and so what that means is like, hey, you know, when people say this is just my opinion, I would do this differently. You don’t have to listen to me. This is just, I don’t know, you know, it’s my oftentimes my like OCD is triggered and I just like have to comment, but I totally get it that you’re you’re trying to do your job. You’re trying to get something over the line. You don’t have to listen to me if you don’t want to.

Erika (30:26) My pet peeve is when CI is broken and somebody requests for review without any additional context because I start to go in and debug why the CI is failing and it’s time that I don’t need to spend but I always end up doing it anyway. And my…

One of my favorite things is when people sort of like compliment or give positive feedback on a PR. Like if there’s something that’s especially good, they’ll call it out in addition to any suggestions. And I always think that’s a way to spread positivity and encourage the right things.

Cool, thank you all so much for sharing and for the discussion. To recap sort of what we’ve talked about, when you’re reviewing a PR, think through the impact that your review will have and recognize that it’s an important thing to block off time for and really be thoughtful about, but you’re never gonna catch everything, so don’t try to stress out about being a…

a perfectionist in your PR reviews. And when you’re creating a PR, be thoughtful and architect it to not only be easy for the reviewer, but also as a document for anyone who might be looking at your code or your changes in the future to reference what’s going on and the impact it might have.

So thank you all so much for tuning in to Overcommitted. If you like what you’re here, please do follow, subscribe, or do whatever it is you like to do on the podcast app of your choice. Check us out on Blue Sky and share with your friends. Until next week, bye bye.