Ep. 18: Pull Request Commenting and Conventions

Transcript#

Claire:

Welcome back to Word Wrap with Claire...

Steph:

and Steph! You can find the transcript for today's show on WordWrap.dev.

Claire:

On this episode of Word Wrap, we're gonna talk a little bit about PR comments and just etiquette, kind of how we feel about it, how we've gone through our careers with good PR comments, maybe some bad PR comments. And we're just gonna, we're gonna talk about PR comments today.

A lot of folks that use GitHub or other version control systems have a way to get feedback on your code. And that's what we're gonna talk about today as a roundabout way of saying it.

So PR comments they're loathed, they're welcomed. They're very odd creatures, cuz every, it means different things to different people. For example, when I was younger, I took offense to comments because I thought my code was good. And then I would be, I would be told that that was not correct. And at first I thought that that was kind of a personal attack, but you know, I think it's just kind of a maturity thing.

As, as you get a little bit further along into your career, you kind of figure out that everyone's just trying to help. We're all just trying to get to the same destination. You know, there are instances where maybe some folks have personal grudges upon others, but for the most part, that is not the case. And that's taken some time for me to get used to.

And yeah. So we're just gonna talk about PR comments today. Steph, what do you, what do you think about PR comments? What do you think about pull requests in general?

Steph:

Yes. Well, I have to interject real quick, the story that I just thought of, or the anecdote, which is that you and I learned git together.

Claire:

Oh, we did. Yes.

Steph:

Or at least. I guess more accurately, you had an inkling of what it meant. And you tried to teach it to myself and our other coworker at that time at our previous place that we worked together. Which was six years ago? Eight years ago? I don't know. It was a long time ago.

Claire:

It was 2016. So yeah. A while ago, six years ago.

Steph:

Yes. And so at that time, we were just like, how the heck do you even branch? That team, when I, by the time I left it, which was after Claire had already left, we still weren't doing pull requests. So we were just strictly using the version system. But we were also all sitting together and there was only two and a half or three of us at any given time.

But yeah. So PR comments, I think to your point, or, you know what you mentioned, like taking them personally. It is something where if you are more junior or it's just a new process for you, it definitely can feel like that. Especially, if in that scenario of joining a team that's already like very, has those very embedded in their process. And so they already have like a way that they tend to deliver them.

And you know, now in our remote world too, I'm thinking of when I joined my current team, our current team. And yeah, when, when those conventions are already in place, that can be as tricky as anything. Because if you're, first of all, learning how to do PRs. I, I was used to PRs, but you're learning about the team, but you're also learning this sort of extra layer of language that they use. And so it might be partly about the comments, but also about the taxonomy that's all around the comments and around the PR process. And so all, obviously, all those things lead-in.

But I think what we wanna talk about today, or more so focus on is the actual comments and feedback part. So regardless of whatever else is going on in your process, you know, how, how do you deliver feedback that is useful, but also essentially remember there's a human receiving that. And I think Claire and I both have different styles, definitely, folks on our team have different styles.

So there's a part where of course you know, your own personality, your own bias, your own experiences enters the picture. But then there's also a side where having some true conventions is really helpful to keep feedback a little more consistent.

And so you prevent going down a huge tangent or rabbit trail in a PR and find a more appropriate way to have that conversation. I think is also kind of an interesting part that's still tricky for me to figure out in PRs.

Claire:

Yeah. I think you kind of bring up a good point of like, I think we've at least I've had the instance where you kind of get into a discussion in a PR. And you're like maybe we should take this out of GitHub and like make this a meeting or make this, you know, something else that might include other folks that might have opinions about it.

But to your point, there is a human on the other side.

Generally, code is written by humans and at least for now.

And, it might be one of those things where it's like a passion project. It might be something they're really proud of. It might be something where they learn something and they got it, you know, figured out. And they're really proud of that.

And I, I mean, I've been on the receiving end and the giving end of those comments. And I think it's just really important to, you know, if you're the receiver of comments that it's not generally a personal attack, it's more of just like maybe it's a style thing. Maybe it's a, a performance thing. Maybe it is maybe it's a typo, you know, and, and we all make those mistakes.

And, and then if you're on the giving end, just realizing that maybe, maybe the I hesitate to bring style into it because I feel like some folks comment just based upon style. And that's why we have linters. That's why we have conventions, you know, especially if you're on a team you have convention set up. Or maybe you should have convention set up so that those discussions don't even take place in GitHub because you've got technology or part of your build process that, that kind of helps, you know, figure that stuff out.

The reason why I brought this topic up today is because I think it's definitely something that I've had to learn a lot just in the course of my career that, you know, PR comments are I feel like they're more rare than, than I wish they were. And.

You know, getting someone to review your code and not just put a checkmark on it I think is is a big undertaking, both for the reviewer and for the person receiving the comments.

So with that, I kind of wanted go into more of like maybe approaches to how to comment. There are several different schools of thought about this. Maybe, you know, some folks like to explain their rationale, maybe an example.

GitHub recently has given - at least in GitHub, I, I don't have a lot of experience with GitLab or Atlassian's products. BitBucket, which is a fantastic product, I just don't have a lot of experience with it. So this is gonna be in the frame of GitHub, but GitHub recently, I mean recently as in the last year started to allow suggestions, code suggestions. Which I think is one of the biggest welcome, you know, changes to PR comments in a long time.

Because we can sit here and explain something all day, or we can just write out the code and be like, Hey, maybe you should change it to this. And, I think it is in and of itself, it's not a critique then. It's just a, Hey, maybe we just write it this way.

And it's kind of like this asynchronous pair programming to a certain extent, which is, I think validating in a way.

But there are also other conventions. One of my favorite one is called ConventionalComments.org, which actually kind of lays out a kind of a framework for how to go about giving certain types of feedback. Whether it be showing examples of comments that are maybe not helpful or by just kind of stating like what we should comment, maybe what we should leave out a comment. I, I won't completely go down the list of what it says on the, the website. We'll put it in the show notes. But it's interesting that this particular convention actually just utilizes like prefixing comments with labels, like suggestion or nitpick. And then that can kind of give you the ability to prioritize based upon, you know, what kind of comment that is.

I think with PR comments there's a lot of power dynamics that aren't really talked about a lot.

Especially if PR comments come from like a senior developer or maybe someone, maybe a manager, it kind of changes the dynamics of, do I have to listen to this? Is this something I have to do? And I think if everyone was kind of working on the same page of, you know, using a certain convention, it could be any convention. I'm just using conventional comments as a rubric, I guess. You know, if you use those a certain convention and your team's gonna be different, you know, and you might find that teams might already have that and you come into a new team they already have it. But working on the same framework will help alleviate some of those power structures that otherwise, like it might make you feel like, you know, you're not necessarily doing good work or whatever.

And then also I think something that isn't talked about a lot is PR comments can sometimes just be like, this is amazing, great job. Like, that feels so good to, to see. And I mean, we've been in this for years now and each time I see like a, "This is amazing. How did you do this?" Or, "I didn't think about this like that." Especially from a developer that you, you know, may respect or like, like that that's, that's huge in terms of confidence.

So, you know, we've talked about imposter syndrome a lot on this show, and I feel like, you know, ways to kinda level the playing field is to compliment when you've done good. And cuz I think PR comments are one of those things where it's like, we only check the bad, we only check the, what might be wrong and that's not necessarily the frame you need to use to, to write your PR comments with. So that was my little plug. Steph, do you have any kind of general tips for PR comments?

Steph:

Yeah. So, you know what I, you were also kind of making me think about is PR comments really start back when you initiate a PR. And everyone has a different style there.

I, you know, sometimes you just kind of throw it out there, you give it a title because that's required and you may not even put anything in the comments. And sometimes that's okay. Sometimes it's very self-explanatory. But I think that that's your opportunity to invite comments. Like if you're looking for it, just being able to clearly state like, Hey, I'm actually not sure about this solution.

Or sometimes what I'll do and others on our team will do is you go ahead and make the PR, maybe give a little background if you need to, but actually go to the files then and add your own comment that specifically calls out those areas. Like I'm not sure about this, or here's maybe some background you need to understand why this is happening.

Because despite working on the same code base, not everyone has the same history, the same context you probably as the PR owner for whatever change you're making or feature you're developing.

You obviously have way more context, or at least likely. Sometimes, you know, cuz work just falls out of different places. You might be working solely with a particular project manager and you've had side conversations. So being able to bring that context in or go ahead and more directly link to, you know, the layout that you had received or call out specifically, "Hey, go check the work item. There's more documents that you're gonna need to validate this work."

You know, so just basically thinking, how can I set the reviewer up for success? Which might feel like, seem a little strange, like you're like, that's their job. Like we have to review each other's PRs. And that's definitely something I've improved at recognizing was needed as I've just grown as a developer. Those things that I mentioned aren't stuff I always did, but they can be very useful. Especially I think I started using them much more now that we are completely remote because I can't just go walk over to someone's desk either and bug them, or they can't do the same and holler at me, "Hey, what's what's going on here?" and we can look over each other's shoulder.

So keeping that in mind and also just on a personal life, I've done more open source work. And so it, most of what I'm doing, doesn't really involve PRs, but still, you know, also keeping your commit sizes small and making those messages useful. That's part of the PR review. That's part of helping a reviewer understand. So if you can chunk up your work, all of that helps in getting a successful review, a quality review and ensuring that the review accomplishes its purpose, which is, does this code work? And does it meet the objectives for this change or feature?

Claire:

Yeah, I think that's something you actually said brought up some thoughts in my mind. Like that description, that, that, that opening comment basically that you, that you add when you create a PR is kind of like your selling point. It's your, it's your thing that like gets the reviewer interested in the, in the thing that they're reviewing. And it's your opportunity to use, like, you know, screenshots, screenshots are huge.

You know, at companies prior we've had templates in the comments to kind of state, like, what is this, what are we doing? What is this effect? You know, some of that stuff can be supplanted by technology. Like, you know, build bots and stuff like that, but the admittedly I've gotten bad about that recently. Lazy, you know, just not taking screenshots all the time.

But, you know, commit messages are huge too, because I think, I think we all think that folks might review a PR in the same way we might, where, you know, maybe we, we go straight to the files tab. Or maybe we go straight to you know just the the testing environment or something like that. But there might be folks out there that, that look at things commit by commit to kind of figure out like how you went about making this change.

And if the commit message is "more changes," which I literally did, like maybe a week ago cause I was frustrated. We're all guilty of that, but it's one of those things where when someone's reviewing it, they might lose that, that context of why did you need more changes or why, you know.

We started talking about PR comments, but I think it's kind of this whole ecosystem of like sharing code and like getting feedback on the code and then merging it in.

It's all the things encompassing. So, yeah. I think it's interesting because like, I've had PRs that, you know, lay out the problem, lay out the solution and you know, some folks might not get it. And then I've had other PRs that have absolutely no description. And I get the best PR comments in the world. And it all depends on who is reviewing it. So I think you know, just having a, a broad understanding of what your team's looking for in a review and looking for, in terms of like how we want to review things.

If you know, we're a shop that just merges in any code, you know, then, then I guess PR comments really don't matter. But PR comments are probably one of the best ways for, for that, I've been able to learn both by my peers and just learning that I did something wrong or may maybe I could have done something in a better way. And so it's a huge impact on at least my development experience. So.

Steph:

Yeah, I think that's, we talked some point in season one, or maybe just one of our un-aired episodes here. I can't remember, about the value of teams. And I would say that there are, well, there's many reasons why I am not ready to do a freelance jump. But having that feedback is, is a huge benefit.

You may know the language that you're working on inside and out. But, it's your work.

You've been staring at it too long, I promise and you just need that second pair of eyes. You just do.

So, you know, being on a team, there are tools that we can use, like GitLens to jump back and see further in the history. But you still, nothing beats someone who can give you the, you know, more immediate feedback, the more real-time feedback.

So PR comments don't - like Claire had mentioned, you know, it's great to receive positive - but PR comments usually aren't... well, they really never should be a personal attack. Hopefully. If that's happening to you, please find a different team. But yeah, so it's, it's really just about improving you, your work. And if you are more junior, you know, hopefully, your more senior developers are using it as an opportunity to look out for you, too. And also look out for the code base at large. Like, is this something that's maintainable long term? Is it scalable? And every team and every project's gonna have different concerns like that.

So Claire kind of mentioned that ConventionalComments.org, we use some similar behaviors. So we don't strictly follow that on our team. But one that the team learned and that I picked up from joining this team was just in, just in block brackets square brackets, saying "[nit]". So in other words, Hey, you know, this could probably be cleaned up basically is what it means. But it's not gonna block anything. And, you know, depending on urgency of the PR, we could skip it and then decide like, you know, you can either have a little conversation. Maybe we do need to just log it as a bug, but it's not blocking, we can do it a different time.

But that thought process, whether to use the [nit] convention or not, the thought process is important as a reviewer. Like, you know, are you, you want it to be helpful and you want to drive towards that, you know, meeting those objectives. Like I mentioned earlier, that's, that's the goal at the end of the day.

And then if you need to expand on that, then consider that too. Like, is, is this something I need a history of in the PR? Is this the appropriate place for the history of this decision to live? Is something that I personally think about. Or is it something where I need to have a side conversation, get more context. And then I can come back to the PR and drop the outcome or allow the, you know, the, the original, like the PR owner to handle if relevant, basically.

So, the history matters, I think when you are developing such a large project in particular. Technically, you know, some of that history is gonna live in your work items. That's a different topic that developers don't necessarily control. And so what I mean by that is that might have artifacts, but it may not have like a history of the decision.

And so, I think that's a benefit of using GitHub or other version control is that theoretically future developers can hop back to that PR to get that context when they don't have a team member available with, just the real-life history of what happened there. And obviously, everybody's not gonna remember either, even if they have been there a long time.

Claire:

Yeah. I think I think something you hit on was GitHub PR comments are kind of like asynchronous rubber ducking kind of a little bit. For those who might not be familiar, rubber ducking is you basically have a rubber duck look at code and see if it thinks it's wrong or something, but the rubber duck is another person instead. That was a bad analogy. Please roast me about it. I don't care.

But I think one thing that I picked up from what you said, Steph was calling out if your comments are non-blocking. Is kind of going back to those power structures of you know, if you have a senior developer say something and, or even just another developer, in general, say something. You know, if it's non-blocking, like, if it's okay to merge the PR while also making the comment, then make sure that that's said. Because especially a junior. Especially a lot, lot of times in my career, I've seen a comment that didn't tell me that it was not blocking and it ended up making that change. And it ended up taking me a lot, lot longer because I didn't know that that was just a, it was just a comment. It wasn't a request for changes, if that makes sense.

So yeah. Any parting words on, on PR comments, Steph?

Steph:

I just, one more kind of, part of it too, is as a reviewer, I would say, get familiar. If you're not using GitHub, get familiar with what that environment offers you. So at this point in time, GitHub has just recently added the file tree and my knee-jerk reaction to that was I hated it. And now I appreciate it. Because it means you can easily skip past... so like in our, our main thing we work on there's files that are just auto-generated. They have to exist for the site to build basically, and they don't need separate review. So like, I can either skip past them now in the file tree, or you can use the file filter and just filter out those, if they have a particular extension that you can filter out. So do that make, make your life as a reviewer easier.

I also am embarrassed that I just learned that you can toggle off white space.

So if the change was just changing indentation you can have GitHub hide that. Oh my gosh. Life changer. If you did not realize that was available, it's kind of a hidden option. So if, if you also didn't know about it, now you do please make your life easier with that. Cuz that happens to us a lot, that that's the type of changes that just bulk up a PR.

So basically what I'm saying is, you know, make your life as a reviewer easier so that you can do your best to provide a quality review to, to that PR.

Claire:

Yeah, I think at the end of the day, PR reviews exist because we all need to make sure that the code is able to be used in a way that it's intended. Like I've said many times before, computers only do what we tell them. So if we tell them something wrong, then they're gonna do that thing wrong.

So PR comments are just another part of the ecosystem of development.

Whether you are you know, in open source, whether you are on a team. You know, even just freelance having someone else just look at the code, maybe not even having context, all of those things might help.

Steph:

So we hope that was useful and definitely get in touch with us if you have other ideas for how you have made the PR process, more streamlined, more enjoyable, or other tips on how to communicate to your teammates in the PR.

Claire:

Thanks for listening. Be sure to subscribe and keep in touch on Twitter by following us @WordWrapShow. If you're able to cover show costs, join us at Patreon.com/WordWrap. We'll see you next episode!