Everybody knows one of those senior dev gatekeepers for codebase who just swat PRs away and block every code review that comes through. Today we talk about some healthier ways to deal with the gauntlet of approvers and the benefits of mob programming code reviews. Tuning in you’ll hear about the pros and cons of code review posses, the problem of people who just approve code without really looking at it, and the question of how nitpicky we should be. Our hosts also discuss personal code styles versus agreed-upon code practices and explain the reasons why they will block a PR. To discover the benefits of mob programming, tune in today!
Key Points From This Episode:
Transcript for Episode 247. Pull Request Anti-patterns
[EPISODE]
[00:00:01] MN: Hello, and welcome to The Rabbit Hole, the definitive developer’s podcast, living large in New York. I'm Michael Nunez. Rabbit Hole roll call.
[00:00:10] SC: Sophie Creutz.
[00:00:11] DA: Dave Anderson.
[00:00:11] WJ: William Jeffries.
[00:00:13] MN: Today, we'll be talking about pull request anti-patterns.
[00:00:17] DA: We love code review. It definitely has its place in the workplace. Sometimes it can go off the rails.
[00:00:25] MN: Definitely can go off the rails. I'm sure we all have some experience in how pull requests made our lives a little bit more difficult. We're going to talk about some of those anti-patterns. Then potential solutions to a lot of those anti-patterns. Can anyone in their experience give us a particular anti-pattern you've seen in your workplace?
[00:00:50] WJ: I think, a big one is everybody being afraid to do code reviews. Then, they just never have the time. Maybe there's one or two senior people on the team who are super busy and never get around to that. They're the only ones who the team for whatever reason seems to trust with the code review process. I think, it depends on the team.
[00:01:13] SC: Does that happen, though, do you think? What is the source of this? Fear and hesitation?
[00:01:20] DA: Yeah. I think, one source of it could be that there is a silo. Even though you're doing code review, which is like we say, the best thing to – compared to asynchronous pair programming for sharing context. People are not leaning into context that they're not aware of sometimes and get afraid. I don't want to touch that piece. This is Stripe integration. That's Bobby's job.
[00:01:53] WJ: Yeah. Sometimes you get one senior dev who thinks that their job is being the gatekeeper for the codebase. They just swat PRs away. That one person who blocks every code review when it comes through.
[00:02:13] SC: Oh, yeah.
[00:02:16] SC: Oh, yeah. Then I feel that tense – There are other things that can pop up in response to that. If people can find a way around that person, there's little Bobby's on the side of that big Bobby and they're like, “Okay. Well, just – improve my PR real quick.” He gets a chance to look at it.
[00:02:37] SC: Before the alpha dog notices. Yeah.
[00:02:42] MN: Right. I do feel it's self-imposed, though. Sophie is like, “Why does this happen?” I think, the lead developers can sometimes put themselves as the alpha dog to say, “I am the one who approve approves a pull request.” Give an example, it would be like, “Oh, if you need two approvers, the team may, like unwritten rule, the team may think, oh, one of those approvers have to be this alpha dog person.” It's like, well, not exactly, was y'all mentioned before, a person go behind alpha dog over here about, “Yo, yo. I need two people. Don't even look at it, bro. Just hit the like. Just hit the approve button, so I could get thing emerged, because I'm running late.” That kind of stuff. Which is toxic, because then you're just going around your teammates too, which is not ideal.
[00:03:30] WJ: Yeah. Has anybody been in a code review posse, where you have a group of people that you know are going to give you the plus one?
[00:03:42] MN: The code review posse.
[00:03:43] DA: You have a blood of – Well say, it’s really a blood thicker than blood of Yes. You're it's also it's really a body thicker than blood? It’s code.
[00:03:50] MN: I mean, I would say yes. I hated it when the bond have the code review posse is leveraged on a ridiculous pull request that has 2,000 line changes. I'm like, “Yo, bro. I know we’re a posse, but this is foul, bro! I have got to look at this and I have to sit down and take an entire day.” Don't flex on the posse like that. I mean, that's a problem too.
[00:04:17] SC: I know it’s a problem.
[00:04:19] DA: What's a more healthy way to deal with this gauntlet of approvers? If you're in this situation where you have multiple reviewers that are required and you get the big Bobby. Who is in charge and has a lot of limited time and doesn't want to give you the time of day, or always has changes and drags things out?
[00:04:46] MN: I mean, I think there's a few things in mind. Mob programming code review comes to mind when it comes to the gauntlet, because everyone is in on and can agree or disagree on changes together when there is a PR up on something. It could just be like, “Hey, if this is one person's opinion and they're sharing it. It becomes a discussion. Okay. Well, why do you believe this is the change that we need to do? Why do you believe –
[00:05:16] WJ: Is this mob programming, or mob code review?
[00:05:19] MN: This is mob code review. Let me rephrase that. It is a mob code review, more than mob programming, which is you're doing that a 100 percent of the time, not just on code reviews. I think that another thing, like there are systems. I've been at a place where we had an individual who was a gauntlet for code style related changes, which I always thought was really nitpicky for individuals. Another thing is there are systems in place to make sure you have the proper linter and be able to clean up the code, so that everyone agrees in the uniformity of the codebase is another –
[00:05:49] WJ: Although, sometimes as nitpicky comments, I think can be helpful. if I started a new project and I'm not that familiar with the conventions that the team has agreed on, the ones that aren't enforced by the linter. As long as it's not blocking. It's annoying when somebody is like, “Oh, you need to switch the way that you indent your code.” Then they block it [inaudible 00:06:08].
[00:06:13] DA: Unless, it's Python indentation problem.
[00:06:16] SC: Well then, it’s not true, but yeah.
[00:06:20] DA: Yeah. That's a side thing. Yeah. I feel like, I've been in a situation, too. I used to really despise code formatters, like the prettier and black. It'd be like, “Oh, why did it do this to my code? I had this beautiful structure of braces and commas and parentheses.”
[00:06:41] SC: Now, you totally rely on, so I suppose. I know I am, but I’m totally reliant on prettier. I love prettier. I’m in a situation where there is like, “You don’t have prettier? I can’t.”
[00:06:53] DA: Right. Right. I do remember those PRs, where someone would be like, “We don't break the line like that. When the art number of arguments is too long, we break it like this, and not the other thing.” It's like, why? Why does it matter? Then maybe, they're like, “Oh, but we do it like this in this time and like this in the other time.” Okay, let's not care about that. Let's use a format and not talk about that. Let's talk about something meaningful, or import orders, or something like that. Something that feels – sometimes inconsistent, also. Maybe one person is like, “Oh. Well, we should have React first, and then our things and then the other things.” Then someone's like, “Well, this goes up here. This is how I like it.” Then it's like, okay. Then someone's like, “Why are you putting the lines in there? Oh, you should put the empty lines in there.” It's like, “Oh, no. My brain is going to melt.”
[00:07:56] SC: That sounds maddening.
[00:07:57] MN: I think, that goes to another PR anti-pattern, which is something to avoid when you're reviewing a PR, whether it's alone. I sound sad. Whether it's alone, or whether you're doing a mob code review, is the marathon where you have to sit down and strike through every single line of code to ensure that the code is pristine, especially if you do this in a mob session that can take a long time, and you're just holding people up hostage on the pull request that's currently there. It's probably not ideal for the team to do.
[00:08:34] DA: Yeah. I feel like, this can take a couple of different forums where maybe the problem is that your PR is too big. It got too big. You got a 2,000-line PR. Therefore, it's going to take somebody three hours to finish it. It's like a Peter Jackson movie.
[00:08:55] MN: Oh, no.
[00:08:56] SC: Hey, no. Up straight up, Peter Jackson. I'm curious, if you're in an agile system, how that could occur, and if perhaps, maybe the idea of having incremental PRs could help with that situation.
[00:09:14] DA: Yeah. You might think about different slices for your stories, or incremental PRs, or a lot of different things.
[00:09:21] WJ: Yeah. I mean, I think if you have a toxic alpha dog type, then also, I don't know, maybe talk to them, like a conversation. There’s communication skills that come into play here. You can implement the process around them, and mobbing is a way to do that.
[00:09:40] DA: Yeah. I do like mobbing as a way— It's like a group intervention for that behavior. I definitely have been in situations where the person who has assumed that power is like, “No, I cannot talk to you. You're poisoning the well.” By me building a relationship with you and liking you more as a person, it's going to make me less strict on your code.
[00:10:06] MN: I definitely have seen that. Or, it's just like the power dynamic of owning the code base for what you believe – what that person believes is the right thing. It's like, well, okay, why do you want to write the brackets, or your code style in this way? “Oh, because I like it.” It's like, “Well, no. Really, come on. Let's not do that, because we like things.” We shall have a discussion as to whether everyone agrees on a particular code style or not, but I can see how the person will not want to change their behaviors, because that's how they code.
[00:10:45] SC: Yeah. I guess, just to play devil's advocate here, there's an aspect to which having a personal code style – that's part of the joy of programming. I just wonder if there's ways that you could allow people to have an aspect of that still when they're programming, but also, with some things, like agree on code practices that are universal throughout the team. Is there room for that? How do we make room for it?
[00:11:21] MN: I'm trying to think of what a thing that – I would look at a thing and be like, “Oh, that's so Sophie.”
[00:11:27] SC: Oh, my gosh. Apparently, the way I write if else's is unique, at least according to someone that I showed that to, who was a Swift developer. I don't know. We probably all have things that we don't even realize are our habits when it comes to programming.
[00:11:46] MN: I mean, I just don't want it to be disruptive to the team. Where like, I'm willing to make those changes to my art style. I think, we've had a podcast episode on formatters and how that changes the way we describe our code onto the screen. Whatever keeps it uniform, if it's ultimately not the way that I write a fat arrow function for whatever reason, then so be it. That's okay. I just don't want to be the main – cause people to look at functions differently, because I want to implement something, versus everyone else on the team. I want to be a team player.
[00:12:21] SC: Oh, of course. Of course. Absolutely.
[00:12:26] MN: One of the things that I think we were talking about the posse, the PR posse, is the idea of people who rubber – who are often rubber stampers to PRs, which I – I remember at some point in time in my career, where it's like, “Oh, yeah. No, no. I could give it to my Babu. Just say yes to it.” It’s like, is he or she really looking at the code, making sure that I'm not introducing a bug? I'm looking for a second eye to help me here. I'm not looking for a second pair of eyes to push this code forward. I felt like, that always used to trip me up from time to time.
[00:13:00] DA: Yeah. My impetus is normally like, “Oh, I need this to be more lean. I need this to keep shipping the code.” Sometimes, I find myself holding back on comments or things, when it's like, “Wait. Maybe I should be a little more critical. Maybe I should focus up.”
[00:13:27] SC: I mean, where's the lime, I guess? That's the question. It’s like, how nitpicky should we be?
[00:13:34] DA: Yeah. I think, sometimes those things, I try to make things synchronous, and approve it. Then talk through some alternatives that could be there, just to prime them going forward. Or maybe if it really stirs them, like if they get inspired, maybe they'll dig into it.
[00:14:03] WJ: I like to leave comments along with an approval. I mean, part of the benefit of the code review is that you get an outsider's perspective. The things that I will actually block on are things where it's like, this seems like this is going to introduce a bug. This seems like this is actually broken, or I don't know. This is really, really unmaintainable and other people are going to have a hard time lots on this.
[00:14:31] MN: I think, for me as well, code that could introduce confusion, if you're duplicating code, or that code is duplicated elsewhere, I would call it out to make sure that we're referencing the same function. Typos, you got to call that out for sure and hope that – you don't want to put up block of – You don't want to block a PR for a typo. You need to make sure that that typo gets fixed before it gets introduced into the codebase. I mean, it may not introduce a bug, but the variable name is completely different. If you were to search for that, then you won't see it, because there's a typo.
I think, what William mentioned, if it's going to introduce a bug in some way, shape or form, but on top of that, if it's going to cause confusion, if there are some anti – some code smells that we could reduce, then I think that would be – I would leave that up for a comment, but I don’t know about a lot.
[00:15:22] WJ: Yeah. The thing with the blocks is you have to remove your block leader. This is also putting a burden on you as a coder to be quick in responding to whatever changes their original [inaudible 15:34] the PR makes.
[00:15:37] SC: Yeah. It sounds like, you don't want to get too in the weeds with your code reviews. So that, if there's something that is not critical to shipping the software, maybe don't want to hold the PR up for that reason. Yeah. William, I love your idea of practice of leaving an approval, but also leaving comments that's effective.
[00:16:04] MN: Yeah. I mean, I think one thing to note, I guess, in some of the PR anti-patterns, I guess, with the team, definitely discuss what is the purpose of the PR, right? If everyone comes into an agreement that hey, the PR is so that you get an outside perspective. We will leave comments there. There's a line that we're going to draw that we will block PRs on and it's when you, for example, do X, Y or Z, then everyone's aware of that. Then everyone can make an address and do the best that they can for those code reviews. I'm sure that there's a lot of other code review stories and pull request stories that we have, that we didn't even surface on, which I think would be really interesting to talk at another time.
[OUTRO]
[00:16:49] MN: Follow us now on Twitter @radiofeerabbit, so we can keep the conversation going. Like what you hear? Give us a five-star review and help developers just like you find their way into The Rabbit Hole. Never miss an episode. Subscribe now however you listen to your favorite podcast. On behalf of our producer extraordinaire, William Jeffries and my amazing co-host, Dave Anderson, and me your host, Michael Nunez, thanks for listening to The Rabbit Hole.
[END]
Links and Resources:
