Should Developers ACTUALLY Pull Code For Pull Requests?

Transcript is auto-generated and may contain errors.

Hey folks, coming in from Arizona. Got another question from Feder Rico in the YouTube comments. And this one is related to the previous video. I'm not sure if you watched that one yet. This is also on sort of pull request and code review stuff. So the question is when doing reviews, do you actually run the code? I usually review both front and back end. So I run the branch with the uh the changes mostly to check the front end. And uh the context they had provided for the other question is they said I currently don't have anyone reviewing my code. I think probably small team small company uh outside of themsel but they said I review code from one other junior dev. So sounds like the team's pretty small. Um yeah I'm going to talk about this. I have some thoughts. I think there's some interesting things going on here but I'll share my perspective on this.

If you're new to the channel, you can submit comments uh below on software engineering or career topics. I'm happy to try and answer. I don't have all of the answers, but I can give you my perspective on it. I've been a software engineering manager for almost 13 years now. So, uh this channel is all about me trying to explain how I would navigate these things. Uh and where I can, I try to give perspectives that I might not even have, but I might just try to look at things from different angles as well. So, that's the goal. Hopefully, it's helpful. If you're not comfortable leaving comments below because they are public, you can go ahead and look for Dev Leader on social media. Send me a message. Happy to try and answer. It's my main YouTube channel as well with C programming tutorials. There's ré reviews where you can submit your resume for free and have it reviewed.

There's a podcast with other software engineers and a live stream every Monday, 7:00 p.m. Pacific, unless I'm on vacation in Arizona. So, um, for this one, I think it's interesting because I think what they're doing is an effective step. Okay. So, if you're looking at the goal of a pull request or code review, and the other question they had was about people talking about this on LinkedIn and stuff in terms of pull requests, but the theory here is that you'd be pulling down the code and you would be running it and you would be trying it. You would be exploring it in your IDE, uh, running it, seeing what's going on. Um, so I think that this gives you a really good opportunity to like actually understand what's happening and not just reading code. So, you know, on the surface I I I'm like, hey, this is great that you're doing this because you are going to try and catch stuff.

I don't know. It's making sounds in the bathroom here. There's no one else here. Um, I don't know if it got picked up on the microphone, but it's making weird noises. Um, so it seems like it's helpful, right? Because, hey, there's another junior developer. You're like just reading code. I'm assuming this person might be more junior as well cuz they say one other junior dev. um that to me sort of suggests like they're a junior dev and there's another junior dev, but they might be maybe maybe that's not the implication, but um I I feel like they're doing the right thing here because they're trying to understand the code change better so that they can give feedback on it more accordingly. Um, but what I wanted to share about this is like, and this is related to the previous video, the I think they're they're addressing part of the the life cycle of for software development that's in front of them, which is, hey, there is a pull request.

Hey, you don't have all the context. If you want to have more context, pull it down, run it, try it out. Which like that makes sense. But I think that if we look at this and we talk about it, um I suspect that this isn't something that scales very well. And I'm happy if like you if people do this and they've scaled it to larger teams and stuff and they think it works well, like that's super cool. If you've done that and you want to share that in the comments and like how that's worked for you, like super cool. Honestly, would love to hear it. It's not a facitious thing. I'm not being sarcastic, like let's talk about it. Um, I don't think that this scales very well.

And the reason I say that is because if you're going to use a pull request as a gating tool, that means that if you have x number of developers on the team and everyone has pull requests, what's going to be happening is that you're expecting that those developers are also pulling down code and running it and exploring it and kind of see it's almost like seeing it for the first time and having to go walk through it and run it and try it. And I I just don't think that this scales well. I think it's solving a problem, which which is good. So I'm not trying to like say like it's bad to do this or you no like never do this. I'm just saying I think it's solving a problem, but I don't think it's the the way that I would want to solve this.

So, um, if we're talking about pull requests and code reviews, uh, one of the things that you gain from this is that you are able to, uh, catch problems, right? Um, I forgot to mention this in the other video too cuz they were talking about uh, CI/CD pipelines and like isn't that enough cuz like if you've if you've done it in the run it through the pipeline, it's good to go. And I'm like, yeah, but what I forgot to mention was like it doesn't catch design decisions, right? Like just because it ran doesn't mean that the coding patterns like you could have a completely different set of coding standards and patterns and stuff introduced but it passes CI/CD and it works. So I forgot to mention that in the last video but I think what they're doing is they've set themselves up to be able to be better informed about the changes.

Uh make sure that they're like actually testing it out and stuff. So, like that's goodness, but I feel like it's almost too late in the process. This is my belief. This is what I've seen work well. I'm not saying it's the only way, but I would try to do this kind of stuff sooner. So, um, with respect to being able to run the application and try it out, right, they're saying I'm, uh, mostly checking the front end. Like, you're are you reviewing the code at this point or are you checking the functionality? Because if you're checking the functionality, I would say like maybe you could be having conversations about demoing this kind of stuff to each other, right? When someone has between either of you, when you have something that's operational that you can click through, like why not sit down together and go through this early, right?

This is if you have a product owner, get them involved. Like sit down with them early. You have something that's demoable that you can walk through. Like have the conversations early. So meta point here is like I don't think the conversation or the thing being done is wrong. I think it's more optimal to do it sooner. And if you do it sooner, I think you have to be less hands-on with it. If that makes sense. So in this case, at the point in their software development life cycle, they're pulling down code, they're building it. Those are two things I don't think you have to do. Like you're not gaining anything from that. It's in source control. it's going to be able to get pulled down if the build system passes. You don't need to prove that you can build it locally. You shouldn't have to. That might be some other problems with your build system.

So, I don't think that that should have to be something you you're testing on every poll request. So by the in my opinion by the time you have it pulled down and it's running like you're either exploring the code in a native tool for you that's you know maybe more familiar with uh than like your web browser. So reviewing it in like I use Visual Studio for all my C development. So I can walk through the code. I can F12 and stuff and go through to definitions in a familiar kind of tooling like there might be a benefit to that. So that's cool, but then running it, I feel like you're you're addressing a different thing in the pull request. Like you're not actually checking the code, you're checking the behavior. So my perspective on this is like if you want to check the behavior, check it super early and and you can do that more than once.

It's not like, oh, you did it so early and now you're going to miss stuff. Like do it multiple times. Have them demo it to you, walk through it. But I think that you can make that a collaborative conversation instead of like do it only once at the end and you have to go pull it down and walk through it. So that's one take on this. The other part is like exploring the code locally. This is something that like I think the local tooling could be a good good thing. Um but also I would say like I feel like we can address a bunch of this just by having earlier conversations about it. So, this is the one of the meta points from the previous video that I made on this was like if you're going to wait until the end. I'm not saying that that's wrong or I'm not saying it won't catch things or it won't add value.

I think it will catch things and I think it will add value. And I think over time what happens is people go, "Yeah, but it's a it's a pain in the ass because it happens so late in the process." So, if you're interested in what the other junior developer is doing, how they're designing things, have them put a draft PR up like super early, right? Talk about these things so that you see like, oh, these are the patterns they're going to go do or these are the spots of the application they're planning to touch. Cool. Makes sense, right? By the time if you're doing these things frequently and early and often and they can be lightweight and that might sound like it's going to be so much work to go do that, but by the time you go to do a poll request, you're looking at you're like, I already know all the that's in here.

We've already talked about it. It's not a surprise. I don't have to spend time now pulling it down and exploring it cuz like I already know all the spots they were talking about touching. I can spot check it. Feels good. Looks good to me. I don't have to go run it myself because they've already demoed it to me two times. Once early and then once later after I gave them feedback and they finished out the other things and that looked good. So by the if you do this stuff early, you reduce a lot of churn in my opinion um that happens when it's all late in the cycle and by the time you're doing the pull request stuff it's so lightweight that it doesn't feel like it's a big gating thing that's a pain in the ass. you literally get into this kind of situation where your reviews are kind of like looks good to me.

And it does because you've mostly seen all of this stuff already. Um caveat, I'm not saying that this is like trivial to do or happens overnight. In the situations where this has worked really well for me, I have been in teams uh managing them but also like developing on the team and we've been working together for already years, right? We moved in that direction pretty quick and I think for we were very fortunate we're on the same wavelength kind of thought about these things in similar ways. So there wasn't a ton of friction in the team to to at least be willing to move in that direction. You might have team members that are like wildly opposed to some of the things I'm saying. You might be wildly opposed to it and then if you're like well that's if that's the direction we have to move like you're going to be resisting it and it's going to take longer and it might not work.

So, I'm not not prescribing it, but um that's just sort of the background. Like, we were willing to move in that direction. We tried it. We kept iterating on it until we were at the point where it's like uh in the last video I was saying literally pull requests would be mostly looks good to me or hey, like there's a test scenario we might want to check out, but like I'm not going to gate your review on it. follow up with it or we might have to build more test infrastructure. It was extremely rare there were coding patterns. We're like, "No, man. Like, what's this?" Because if someone had a new coding pattern, they'd be talking about it because we knew we knew that we don't want it to get to the end. We're like, "Hey, surprise team. I built this thing. No one knows about it.

It's completely new. Why? Like, we don't want that. No one wants that. We've all been burned by that. Why would we wait? Hey team, uh I'm, you know, starting on this ticket and like I'm exploring this part of the UI. It doesn't really seem like it's going to be able to fit into here. So I'm thinking about adopting this other pattern. One really cool example of this is that we were building a um a desktop application and we had this toolbar, but a lot of the things that had to go into the toolbar, they were like we'd be building a feature and it also needed a toolbar component to go with it. So we said all these features we are building are kind of like they're pluggable like almost like plugins. So we said our toolbar isn't it's like very hardcoded. It's crappy to use. So they took it upon themselves to say hey look I want to make this pluggable.

I want to make sure that all of these pieces they don't need to know about each other but we need to maintain state. So they're like I'm going to try building this thing. And they built a little prototype in isolation. They walked us through it on the team to say, "Hey, here's what I'm thinking of building. We were like, "Cool, like checks out." We had conversations about it and then they started integrating it. So, by the time the review came around, we weren't like, "What the hell is this random this menu system with this toolbar? Like, this is crazy. Like, who where did where did this even come from?" We had already like seen code for it. We had already seen it working. And by the time the review came around, we were like, "Yep, that's that's the thing we talked about." So, that's just some thoughts on this.

Um, you may find yourself in a situation where you're like, maybe you're like, "That sounds cool. I would love to get in that, you know, that kind of position." You might disagree with it, but if you're interested in moving in that direction, it might be kind of challenging based on your team dynamics. It might be challenging based on other things. But my biggest recommendation is like In terms of team culture, team process and stuff like that, I think it's important that if there's friction, you talk about it. Even if it's a hard conversation, and you try taking incremental steps. Try things out to move in that direction. If they don't work or they suck, undo them. Try something else. But if they suck and you're not ever changing anything, they keep sucking. So, I'm of the mindset incremental improvements. Try things out, experiment, measure, gauge with the team, see how they feel about it, and keep moving forward, right?

If you're uh never changing anything, then anything that's frustrating or causing friction probably isn't getting better on its own. So, I hope that was helpful. Federico, thank you so much for the questions. Uh, and yeah, for folks, just a reminder, send them in the comments if you're interested in having something answered relating to career development or software engineering or send a message to Dev Leader. I will see you next time. Take care.