The Dysfunctional Developer

KBall (00:01.362)
Hello and welcome to the Dysfunctional Developer where we dive into the good and the bad of what is really happening in software development these days. I'm K-Ball. I will be your MC today. I am joined by my friends. Let's start with Amy Dutton. Amy, how are you?

Amy (00:17.888)
Hello! I'm so good. I'm living the dream.

KBall (00:21.926)
Living the dream. Also Chris Boneskull-Hiller, how's it going Chris?

Boneskull (00:27.32)
Super, how are you?

KBall (00:29.406)
I, Mr. Enthusiast, I love Chris's affect.

Amy (00:29.568)
I was too enthusiastic.

Nick Nisi (00:30.66)
Yeah, wow.

Boneskull (00:34.606)
That's as good as it gets.

Amy (00:37.014)
Hahaha

Nick Nisi (00:37.23)
Man.

KBall (00:39.226)
All right, and you can also hear hinting in there, the one, the only, Nick Nisi.

Nick Nisi (00:44.046)
Ahoy hoy things are on fire, everything is crazy. No, things are actually pretty good. I just had to be a little contrarian, because everybody else is so cheery.

KBall (00:53.126)
I did recently send somebody the message that when they asked how I was that I felt like I was the this is fine dog, where it's like, my life is actually quite good. There's a lot of good things. My job is good. And it feels like the world is on fire.

Nick Nisi (01:00.324)
Yes!

Nick Nisi (01:07.256)
Mm-hmm.

KBall (01:09.672)
So whether you're the this is fine dog or your world is on fire and the world outside looks sunny, today we're gonna talk about that lovely topic that shows up for any developer once they get past a team of one, which is pull requests and pull more than that PR reviews. I can see Chris's face already looking concerned. So I think one of the things we can dive into is just like starting with

What sort of processes do you all have on your teams around PR reviews?

Boneskull (01:47.086)
Well, on my team, everybody must get their PR reviewed by somebody. It's a small team, it's a team of three or four. I don't even remember. It's a small number that I can't remember, but it's required. The project that we're working on is, a security tool.

Nick Nisi (01:47.3)
processes?

Boneskull (02:13.531)
And so having a second set of eyes is really just like table stakes for what we're doing.

KBall (02:23.72)
Security is often a driver for this. We are also a small team and we started, we went up for SOC 2 certification. And as a part of that, they insist more or less that every PR has to be reviewed.

Nick Nisi (02:42.658)
Yes, I also have that constraint. think you can set it up in your GitHub branch rule sets, which is really nice to require at least one. And oftentimes I will, I'm a little unique right now in that I am on a team of one. So I often have to, I always have to reach outside of my team to find someone else to review my stuff. And they're not like exactly, you know, they're,

into it and they are incentivized for it to be good, right? But they're not on my direct team, so it's more like a yes and question or a proposal rather than like, can you please do this because you're my teammate? We're all teammates. It's more complicated than that. But I'm often having to do that. And then oftentimes the people I get to find, you can also have in GitHub like teams or code owners for different pieces of it.

I have to find the match game of someone who is willing to do it and on the team that is required by the code owners file.

Amy (03:49.792)
Hopefully this won't get me in trouble. We just are merging everything right now without any PR review. But this is not the main Redwood branch. This is kind of like a second repo where we're trying to build in features and just trying to move as fast as possible. And the sentiment, some of it has to do with time travel. Most of the team is in South Africa and I'm in the United States. So it's like, don't.

Nick Nisi (03:55.392)
Yeah.

Amy (04:19.178)
wait for anything, just move fast and you can always put in a second PR that will also be maybe unchecked, but you could also put in like a second commit to fix anything or if there's anything that needs to be changed that can come in later. I will also add in right now all of this code is experimental, so it's not like it's core infrastructure that's gonna break multiple things, but that is a very different answer than the other three of you.

Nick Nisi (04:45.944)
Just before we go any further, I'd like to level set with the four of us if we can do that. So I have a request. Can you go to your profile on GitHub?

Amy (04:52.576)
Mm-hmm.

Nick Nisi (05:04.608)
On your profile, GitHub has like a nice little chart, like a cross, and in the top is code review, on the right is issues, on the bottom is pull requests, and on the left is commits. And I'm just curious on two of those for this discussion, and that is the code review number and the pull request number, maybe more so the code review number, but I'm just curious like where we're all at with that.

KBall (05:29.458)
Where are you seeing this?

Nick Nisi (05:32.301)
You know like that colorful green chart, that specifically green chart? It's just below that. There's like a cross diagram.

KBall (05:34.344)
Yeah.

KBall (05:39.983)
I'm not seeing it.

Amy (05:41.445)
I've seen that on other people's profile, but I do not have it on my own.

Boneskull (05:48.192)
It, it, yeah, it may be something you need to...

KBall (05:48.254)
Is that a configuration?

Boneskull (05:54.126)
configure. I have the data. Mine says code reviews 31, commits 49, pull requests 13.

Nick Nisi (06:06.35)
Okay, mine is 4 % each for code review and pull request, and then 92 % for commits and zero for issues.

KBall (06:17.918)
I'm still trying to figure out how I turn this thing on. Managing the visibility contribution settings, activity overview.

Amy (06:19.264)
Yeah, same.

Nick Nisi (06:20.799)
I'm sorry.

I thought this would be an interesting thing, but apparently...

Amy (06:26.796)
This is so dysfunctional.

KBall (06:30.078)
Ah, I found it. So you have to go to Contribution Settings on the Contributions list and then to Activity Overview. All right, so stepping back again, there's these numbers, percentage commits, percentage code review, percentage pull requests. What do they even mean?

Boneskull (06:37.698)
Mmm.

Nick Nisi (06:49.292)
It means... that's a good question actually. What does that mean?

Boneskull (06:56.6)
mean, if I had to wager a guess, I would say that every comment that you have left in a code review, like you click the, you know, add your review button, and you like do your approve or deny or whatever. And maybe it's just like, maybe it's just that button at the end. But maybe it's that, and then of course commits is commits.

Pull requests would be opening pull requests. Issues would be opening issues.

KBall (07:31.624)
Got it. OK, so this is distribution of your activity on GitHub. So I have it open now. I have 71 % commits, 19 % pull requests, 10 % code review. No issues, because all the projects I'm in aren't using GitHub issues. They're using other ticketing systems.

Boneskull (07:36.909)
Yeah.

Nick Nisi (07:50.372)
Okay. How about you, Amy? Did you find it?

Amy (07:53.1)
Yes, I did. And it also represents my previous statement in terms of code review. I have 1 % code review, but I have 78 % commits, 2 % pull requests, and 19 % issues.

Nick Nisi (08:07.364)
I was sure that I was gonna be at the bottom, but thank you Amy

Amy (08:11.146)
You're welcome.

KBall (08:15.378)
Well, so I think it's worth talking about both sort of these different ends of the spectrum and when they're appropriate. Because I actually think what Amy's doing for the context she described is totally fine. And I think there are a lot of situations where like,

doing a pull request review on everything that gets merged is not the right answer. Early stage startups where you're just rolling super quickly and trying to experiment, especially if you have a team of one or two, like there's a trade-off there between your velocity, your ability to keep going, and quality. But quality of code is not the main measure of quality at a very early stage. The main measure of quality is how quickly can you get something there that'll let you learn more. So that might be fine.

On the flip side, if you're at Boneskull's company working in security, it feels pretty important.

Nick Nisi (09:13.976)
And mine are skewed because I did spend a year not committing anything to GitHub. And the only thing I did was commits to my .files, which I don't pull requests. So that's why that's way up. But it's still pretty low. Before that, even code reviews and pull requests were pretty low compared to commits themselves.

Boneskull (09:39.626)
I haven't worked in a place where we didn't do code review for a long, time. So I don't remember what that's like.

KBall (09:51.327)
So let's maybe talk then about code review, because I've also seen a wide range of practices and how people approach those. So I'm curious, what do you all do today? I mean, or Amy, guess, what have you done in the past when you've been in a location where there's code review? And is it functional? Does it work?

Boneskull (10:16.376)
So our practices around code review is that somebody who isn't you must approve it.

Of course you use your discretion. And we do use it to expect a review from somebody else. We do use code owners, but it's kind of fuzzy. so, I mean, that can be a challenge. PRs can sit. Especially if we're working on separate things.

And maybe I don't have the right context to be able to tell somebody if what they're doing is right, you know what I mean? So all I can do is at that point is kind of, I can ask questions. But unless I'm reasonably sure that it's not gonna break anything, I will just kind of...

be like, leave the comments and I'm not gonna click approve because I don't want to green light a thing that I don't understand, essentially.

Nick Nisi (11:32.644)
Yeah, at Meta they actually had a feature built into their code review tool that was instead of approving you could endorse which meant I like what I see but I'm maybe not the right person to give you the green light Which was a really cool feature that I wish could have

Boneskull (11:50.038)
Yeah, not a bad idea.

Also, so that's in terms of how many you need to get merged. The other thing I think we're pretty good about is when we do leave a comment, we leave some sort of, I don't know, flag or something that says, okay, this is like a thing you must do, this is just a suggestion, this is just my opinion. And I think we're pretty good about not

not blocking on those things that aren't, that are just like opinions or suggestions. And if I would endorse a PR and I may still have suggestions, I'll click approve anyway because I just have suggestions. And we are free to take those or not.

Nick Nisi (12:51.576)
Real quick related to that, I'm curious, have you ever seen the paradigm or the practice of conventional commits?

Boneskull (13:01.932)
Yes, we use them.

Nick Nisi (13:03.65)
Yeah, it's like a way of describing what what type of action is happening in the commit and then you can You can like build a change log off of that or know that something is a breaking change based on The label that you added to it in the beginning Well, there's that's defined at conventional commits org and there is another site called conventional comments org and that's related to what you were saying Don't skull and something that I followed for a long time where I just put a simple label at the beginning of every comment that I do and that could be

This is a nitpick or this is an issue or this is a question and if it's non blocking I will specifically put in parentheses non blocking just so it's very clear like I'm asking about this I think maybe this is worth looking into Don't hold up because of this but then other things that are explicitly not labeled that Maybe I want something more like if it's more than a nitpick. I want like some kind of understanding or clarification on it

Boneskull (13:59.99)
I didn't know that was a thing. I'm going to go check that out. We were doing it and maybe somebody knew and I just picked it up, but I didn't know it was like conventionalcomments.org. Is that one word or was it a dash?

Nick Nisi (14:10.148)
Yes. All one word.

KBall (14:15.624)
I love that. OK.

Amy (14:15.648)
When it comes, yeah, I was gonna say when it comes to commits, I'm a big fan of Gitmoji and that particular project, have you heard of this? I will share a link. Basically it's, wait, I don't see the chat.

Nick Nisi (14:23.767)
Not, but I'm intrigued.

Nick Nisi (14:32.376)
You mean there's other emojis than my beloved millennial laughing with tears?

Amy (14:35.052)
Yes, straight on, not tilted. Yeah.

KBall (14:37.806)
I've seen people using this and I've never understood what it was.

Amy (14:42.956)
Oh, funny. So with Gitmoji, it basically assigns an emoji to every type of commit. And the thing that I love about this, I use this in all my projects, is I can tell at a glance what type of commit it was, whether it was a CSS change, whether it was documentation, whether I was adding a feature, whether I'm just moving files around or removing files or fixing a bug.

And so it just makes it really nice that you don't even have to read the commit message. You can kind of skim through and find exactly what you're looking for.

Nick Nisi (15:15.978)
Is this just go in like the title?

Amy (15:19.038)
Yes, so I just put it in the main thing. And I tend to use it even with my PRs if I'm adding a feature. mean, most PRs are going to be the introducing a new feature thing. But there's also a construction one. So if I am just opening a PR to have it as a placeholder, I'll do that construction symbol one, say, WIP, work in progress, and then say this is the feature. And then I'll go back and change the title of the PR once it's been committed.

Nick Nisi (15:19.843)
And if.

Nick Nisi (15:47.685)
Wow. And then one more question. If you have, if you have like, you're making a CSS change that is also fixing a bug, would you have the art emoji and the bug emoji? Like, do you just have a string of emojis?

Amy (15:49.664)
Merged, yeah.

Amy (15:56.702)
Hmm, gets tricky. I just do one, but you do you. You do you.

Nick Nisi (16:02.508)
Okay.

KBall (16:06.414)
gonna be like trying to figure out how many different emojis he can fit into a commit while still being compliant.

Amy (16:10.828)
You

I feel like that goes against the singleton pattern, right? Where you want one commit per thing.

KBall (16:23.31)
That is actually also worth talking about in terms of PRs, which is how you group changes into PRs. In particular, I tend to lean towards requesting lots of small PRs because they're much easier to review, but sometimes that then results in things kind of not staying. Figuring out how to decompose logical chunks is not always simple when you're

doing larger new features or things like that. So I'd be curious how you all approach that.

Boneskull (17:00.298)
I use stacked PRs wherever I can. If you don't know what that is, basically it's a PR that instead of targeting the main branch, it targets another PR. And this is pretty cool. It works well. But the problem is you kind of need tooling to make it work well. Because so like you need something to sort out.

Okay, are we gonna rebase? Like if you change one thing in the stack, it needs to update everything else. If one thing gets merged, it needs to retarget and all this stuff. And so I use a tool called Graphite to do that. And I think I'm the only one on my team that's using it, but that's how I like to work. If PRs sit, if my PRs sit for a while, they're just gonna get bigger and bigger.

and because work's gotta get done, I'm gonna push my code and so I try to stack things but I guess I could stack things even more. Like my PRs get out of hand real quick. So that's one thing I need to work on. But yeah, I agree that a smaller PR is easier to review. Like personally, don't mind. I'm maybe the only person in the world, I don't mind reviewing a large PR.

KBall (18:26.974)
actually don't mind it either at this point in my career. I have found that if your team is largely very senior, they often can keep all of that context in their head. But when you bring people in who are a little bit more junior or you have more team churn or things like that, it becomes a lot harder.

Boneskull (18:46.08)
I mean, I think it's also about if do you enjoy reviewing code? Do you like to do code reviews? Because if you don't, then of course you don't want to do a large PR. But I like reviewing code. you know, I don't mind large PRs, but, you know, definitely keeping them small is the ideal.

Nick Nisi (19:10.4)
so many questions man yeah this is the stacked PR thing is interesting and it's interesting I didn't know I always assumed that if I wanted to do that like the graphite thing that I would have to like get a team on board but you're doing it by yourself that's really interesting

Boneskull (19:26.68)
So, it's free for open source projects that have fewer than 10, like 10 or fewer people in their org. And so, that's us. So, you apply or whatever and they'll check it out and they'll let you use it. That's about the, that's how it works. Otherwise, you're gonna pay for it.

Nick Nisi (19:49.124)
Ahem.

KBall (19:51.807)
Interesting. I also use stacked PRs, but only relatively limited and I haven't used graphite.

Nick Nisi (19:51.907)
Got it.

Boneskull (19:58.988)
You know, there may be, I think I've heard of the existence of other open source tools that do this sort of thing. But, be interested in checking them out.

Nick Nisi (20:13.326)
There is this, I don't know for sure, but I assume that this kind of came out of the way that they do it at Meta or maybe the way that they do it in Mercurial, but like the diff tool there, you did per dip or per commit. wasn't a, here's all of the commits that I want. Cause there was no concept of branches. So you just had like a stack of commits and you would review them one by one. So you could keep them relatively small and atomic. But when I'm writing code with GitHub or Git,

like a lot of times there's there's different philosophies about how to do this right but like the benefit of a PR is you can have like lots of tiny little commits that end up not mattering but nobody reviews those individual commits one by one you just get a PR that has every change for every commit that you are proposing to merge into main and I think that that's the big delimiter but there's like pros and cons to that like the con is obviously you have to commit or you have to review the whole thing at once

I think you probably could look at each individual commit, but that'd be more tedious. But then like most teams also kind of adopt a squash and merge strategy where they'll squash all of those commits down into one commit. And, but then that's the feature. And if you need to revert the feature, you're reverting the whole thing rather than trying to look at how the sausage was made and pick pick out which piece might be wrong.

KBall (21:24.638)
I hate that.

KBall (21:36.232)
So on the one hand, yes, and I like the pull request history for exactly that, for that feature level history. However, if you introduce a bug somewhere along the way, being able to get bisect against that history is really nice.

Nick Nisi (21:52.484)
True.

KBall (21:55.859)
Like you're eliminating information when you do that squash. And sometimes like that's a useful thing to do because there's context overload, like mental space and whatever. But in my experience, the net benefit of it for commit histories is negative and becomes negative once you get to the point where you're having to untangle significant bug introductions.

Nick Nisi (22:24.91)
Good books aren't written, K-ball, they're rewritten. You gotta rewrite that history to match what you wanted to happen.

Amy (22:27.926)
Ha ha ha ha.

Boneskull (22:33.996)
The stack PRs can prove more difficult for reviewers because of like, have I reviewed this or not? I don't know, it becomes even worse. so what Graphite is selling essentially is a review tool, like a SAS review tool. So you can use their site and it helps you figure out what you've reviewed and what you haven't reviewed.

Nick Nisi (23:00.782)
So you're not doing reviews in GitHub, you're doing them in their tool.

Boneskull (23:03.06)
I am, mean, I don't use their tool. But my teammates could conceivably use it and it may help them to review my code, but I try not to make things too complicated. I'm not gonna, I'm not gonna, so if I have a stack of PRs, I'm only gonna merge, I'm only gonna ever merge the one that targets main, okay? I'm gonna merge that one. And then,

Nick Nisi (23:06.08)
So confused.

Nick Nisi (23:16.085)
got it. Okay.

Boneskull (23:32.884)
the next one in the stack or next ones get retargeted domain, then we can merge those. If I was some kind of crazy, like, you know, I don't know, but if I started merging things into like their parents in some random part of the stack, nobody would know, like, what is this thing? Why does this PR have this other PR? did I not do like review this already, you know?

KBall (23:56.595)
What did I review? What did I not? I have had a situation where my team actually asked, we talked about it we decided to squash down before a final review because the stacked PR, the second level, not the one targeting main, gave a better example that we could then use for documenting and reviewing all the capabilities of the first one. was like first stack was, first PR in the stack was like refactoring to existing code to a new way to enable new capabilities.

And that was kind of interesting, but did not expose the whole motivation for doing it, where the next PR was like, let's actually use this now to go beyond what we could have done before. And after looking at both of them, they're like, I want to see these together. Let's not separate them. Let's squash them. So I think there are cases where squashing the later PRs in the stack can help.

Boneskull (24:51.39)
And it would be merging the other PRs in the stack into their targets, not squashing in terms of squashing.

KBall (25:00.402)
Yes.

Yeah, sorry, not squashing, squashing is, yeah, correct. I used the wrong word. Merging the later PRs in the stack into their targets before doing the final review and looking to main so that you could get a easier complete picture. This was engineers who don't seem to reviewing large PRs also, which does make a difference.

Nick Nisi (25:25.124)
What does that mean? When I hear you talking about that, just I'm curious what What you're reviewing like when you sit down for a review and there's a huge PR or a small PR Like what are you reviewing? What are you looking for? Are you are you running the code in your head? Are you pulling down and running the code? Are you running the tests? Are you relying on CI for that? Are you you know, there's so many questions and there's so many ways to

Boneskull (25:26.476)
What's matter, Nick?

Nick Nisi (25:54.949)
attack this I'm just curious like what is the most important thing especially when it's you know a 4000 line PR and you're you got to focus on something

Boneskull (26:05.025)
Nick, you should know the answer this.

Nick Nisi (26:08.164)
It's all of the above? Fair, fair.

Boneskull (26:09.674)
It depends.

KBall (26:11.238)
Yes, you beat me to it Chris.

Boneskull (26:19.48)
But yeah, sometimes I'll pull it down. Usually no. I will often like pull down. if we use a lot of tooling, right? Renovate instead of Dependipot. So we use Renovate. And if Renovate sends a PR and it wants to upgrade something and that something breaks, then I'm going to pull down the PR and I'm going to fix it. Unless it like...

Nick Nisi (26:30.264)
Mm-hmm.

Boneskull (26:47.96)
directly impacts my work, I'm probably not gonna do that for other people's PR.

Nick Nisi (26:56.558)
So then are you just looking for like logic things that you might spot? Like, this, I don't know. This if statement doesn't have curly braces and it's, you know, it's not pulling in the right things or something like that. I don't know.

Boneskull (27:08.544)
No, we've automated all our formatting, so we don't need to worry about that. look at logic, I spend a lot of time looking at types actually. So I think I'm probably the most type-experienced person on our team, and so I tend to give a lot of guidance there.

Nick Nisi (27:11.272)
yeah.

KBall (27:34.943)
can jump in on some things that I look for. So types is actually a big one. I try to just make sure that I understand that the whole big picture makes sense. So I'll usually try to, if they didn't already write documentation for it, I will try to describe back in words what I think this thing is doing and how the pieces fit together. And if anything feels weird or confusing, I'll often ask there.

I also, at least in the current code bases I'm running, I tend to have one of the broader understandings of all the different pieces that are going on. So I tend to look for things that feel like they are similar to stuff that we've done before and ask about reuse and why are we doing this? Or is there a reason we're not using that? Or is this an opportunity to do these things together? So that's another big area that I will look for.

Boneskull (28:35.192)
Yeah, I think for my own understanding, I will ask a ton of questions just, why are we doing this? Why are we doing that? What's this PR for? Like, who is this going to impact? Like, I'll just throw out a ton of these questions. you know, personally, I want to understand, but, you know, it's open source, and so somebody else is gonna come along someday and they will...

maybe I have the same question and so often I'll ask for comments. Please describe the intent of whatever weird thing this is, right?

KBall (29:11.09)
That is another thing that I look for is just code that looks weird. And then I want to know why do we need to do it this weird way? And there's probably a reason, but I want to know why. And sometimes when I ask that, there's a much simpler way that we could be doing it.

Boneskull (29:26.114)
And I think recognizing when something is weird is a thing that comes with experience too. I've seen enough like...

Nick Nisi (29:33.614)
Mm-hmm.

Boneskull (29:40.142)
I've seen enough JavaScript over the years that know when something sticks out or something is done in way that I wouldn't expect. And so I can highlight that and ask about it.

Amy (29:53.846)
How much stylistic preference do you infer? So you see something you're like, I wouldn't have done it that way, but I didn't write this code. So I'm gonna let it go.

Boneskull (30:06.606)
I'll leave the comment. I'll be like, this is how I would have done it. That's it. And that's all I'll say. It doesn't imply that it must change. But if you like my idea better, go ahead and adopt it.

Amy (30:15.5)
Mm-hmm.

KBall (30:28.2)
similar. I will often also leave like, you know, to the point of the conventional comments, I'm not, I am not on that system yet that I would like to, but I'll be, I will often kind of indicate I'll be like, I don't love this approach. This is something that I might do in this way or something, but I'm not going to block on that, like, however you you want to do it. Including up and including like there were things that I actually think do need to be fixed or wrong, are wrong, but that sometimes there are reasons where I say, okay,

I think this needs to be addressed. It doesn't need to be in this PR. If you open a ticket to follow up on this, like we can proceed with this and then just make sure we address it and clean up later.

Boneskull (31:08.386)
Those types of stylistic issues are never going to block a PR for me.

Nick Nisi (31:14.619)
Yeah, I agree I do that as well I was also thinking in terms of this and kind of like one thing that I like About the the conventional comments type thing is like when I do that and I'm labeling Things appropriately like I'm saying explicitly. This is a nitpick or this is an issue or this is whatever I'm In a way, and maybe it doesn't come across this way, but in my mind I'm coming across as having no

personality or or Yeah personality for lack of a better word like I want it to be as Non-personal as possible, you know, cuz I'm not trying to like attack you I'm just nitpicking because I can I have a nitpick label or whatever and I can I can do that and you can choose to ignore them Like because if it was anything beyond a nitpick, I would label it differently but like I'm taking a lot of my personality out of that and for some reason it makes me feel better about like

quote unquote, attacking code.

Boneskull (32:14.168)
Yeah, and that's something that for someone who has worked in open source for a long time and is very used to getting their own code reviewed, I know that that can be a difficult experience for some people, especially some people who are new to the field. yeah, my teammates say that I'm nice, okay? So I think I'm doing a good job there.

KBall (32:42.758)
just throw in that that becomes even more important when you get in a role where you have any sort of power or anything like that, right? Like I'm a manager on a small team, I'm still in the code, reviewing code, things like that. But like if you're reviewing your reports code, like you have to be real careful to be very clear about like trying to remove yourself and your power structure and all those other things from this. And that sometimes will lead me to be more permissive on code.

Boneskull (32:49.25)
Mm-hmm.

KBall (33:11.228)
actually, than I otherwise would. But it is an important thing to be thinking about as you are leaving those comments is like, how is this going to be perceived on the other side, not only because of what's written, but because of who I am and my relationship to that person.

KBall (33:37.894)
Alright, so it feels like we've beaten this particular part of the topic to death. I want to ask about a slightly different area around pull request reviews, which is timing. Do you have an SLA or anything like that on like how long before certain requests must be reviewed?

Nick Nisi (33:49.763)
Hmm.

Nick Nisi (33:59.141)
We have about a day, I think, is what's there. I've had PRs open for two weeks. And I ask every other day in our standup, I'm like, hey, I have this out there. Anybody want to review? And sometimes people are busy. And it's less priority work is the way that I was picturing it too. It's like, I need to move this along and check it off my list. But it's not the end of the world if it's in tomorrow. So I kind of like.

There's that it depends for me, I suppose.

Amy (34:31.468)
I'm gonna make an assumption here that it's also harder in your case if you're trying to work cross-department because it's not like it's affecting an immediate coworker and blocking the work that they're doing, but you're trying to also reach into somebody else's work to get help.

Nick Nisi (34:37.465)
Mm-hmm.

Nick Nisi (34:47.736)
Yeah, and it's it's like this this particular issue was specifically like something that's it's a feature ad and it's not really like super important and it's not gonna be released on its own it's like a building block and I'm already on to the next building block like on a Branch off of the feature branch effectively So it's it's not that critical that it gets in tomorrow or anything like that But if it is then that's when I'm reaching out in slack and things like that. So yeah, it's more by ear but

Typically a day is the SLA.

Boneskull (35:22.656)
and we don't have anything like that. mean, in a perfect world, it would be nice if we did, and I could have some sort of guarantee that my code would be reviewed after I pushed it within, I don't know, a little while. But we are not, the things we're doing are not, there's no,

strong deadlines. Basically we have like quarterly OKRs or whatever and we have to and some of the things we're doing in the project are going to be part of those but otherwise there's no

Nick Nisi (36:20.996)
I have another question. Unless you were going to continue with this cable.

KBall (36:26.782)
Go ahead.

Nick Nisi (36:27.652)
I'm curious about when you get that review. So somebody left a review and they left notes Right and they left some things that they'd like to see fixed or some, you know simple things Do you and you go fix them? Because you agree and whatnot or you have a discussion but like let's say that Everything that they said you agree with and you're gonna fix when you fix it and push up that Do you go resolve those comments? Do you wait for them to resolve it?

KBall (36:58.448)
interesting question. I usually resolve if if if there was no disagreement about it, right? If I'm like, yeah, that's good. I'm gonna fix that. I will fix it and I will resolve the comment. If I'm not sure if my take is actually what they meant, what I will do is I'll make the change, I'll push it and then I'll comment in the thread there and I'll be like, I made a change around this. Can you take a look and see if that matches what you were looking for?

So that's kind of my general approach. if I'm pretty sure, and I could be wrong, but if I'm pretty sure I understood what they said and I agreed with it and I addressed it, I will resolve it.

Boneskull (37:37.506)
Yeah, if it's obvious. do you actually, in GitHub you can set it so like branch rules or rule sets or whatever. everything has to be, all comments have to be resolved before it can merge. Do you use that?

Nick Nisi (37:56.654)
Mm-hmm.

KBall (37:58.141)
I don't usually.

Boneskull (38:02.616)
find that especially on larger PRs can be burdensome for reviewers. Especially since, well, for everybody. Because if you get a lot of comments, it collapses, right? They all collapse. And you can't find them exactly. so I... Yeah, if that was turned on, I would complain a

KBall (38:19.198)
How do you find all the comments that you need to resolve? Yep.

Nick Nisi (38:23.268)
Yeah.

Nick Nisi (38:30.36)
That feels like less of an issue as opposed to the other pull request setting where, like let's say that you commit, or you comment on my PR with like three nitpicks, right? It's like, typo here, this here, this here, but you approve it, right? And they're just like super tiny things that actually wouldn't matter if they made it into the code base. And then I go and change, and you approved it, but then I go and change those. There's a branch rule where you can say like,

Boneskull (38:50.158)
Mm-hmm.

Nick Nisi (38:58.486)
It has, you have to get a new review because he pushed up. yeah.

KBall (39:00.222)
I hate those. I hate that.

Boneskull (39:01.294)
Nobody likes that.

Amy (39:01.632)
Hahaha!

KBall (39:04.422)
I ran into that at some point. I don't know how it got enabled. I was just like, what is this?

Nick Nisi (39:08.57)
Yes, it's the worst.

Amy (39:15.734)
I use that resolve comment feature as like my checklist of things that I need to do. So I do check those off. But as soon as all that's clear, I'm like, okay, I'm good.

Boneskull (39:26.028)
And also when you resolve these things, they get hidden and that's not always desirable, you know?

Amy (39:30.122)
Yeah, that is, yes, agreed.

Nick Nisi (39:33.038)
But I will also say, yeah, I agree. But if they were going to leave a comment that was very plain, like, it's very obvious what I should do to fix it, you know? GitHub has a suggestion feature. I feel like you should just use that and make it explicit that this is the suggestion that you are offering.

Boneskull (39:51.395)
yeah, I use suggestion as much as I can.

Nick Nisi (39:55.694)
Same.

KBall (39:56.499)
that. So I get it. also coding within the GitHub UI interface is obnoxious enough or different enough that I totally get why you wouldn't.

Nick Nisi (40:09.602)
Maybe your opinion isn't worth it if you're not willing to put up with that plain text field. I'm joking, of course.

Amy (40:14.028)
you

Boneskull (40:16.45)
That's actually never bothered me. I don't know, you know.

KBall (40:20.328)
I mean, I'll use it sometimes, but every time I'm like, this is an uncomfortable, or like an annoying.

Boneskull (40:26.571)
I- I,

Nick Nisi (40:26.71)
If it's very easy, the white space can get out of hand very quick and that is a super pain.

KBall (40:31.806)
Yeah, one-liners totally fine. if it's like, I would love a refactor of this in this, you know, or like shift this around in some way that's going to be anything more than a one-liner. I find that at least the last time I tried it, which it's to be fair, I've been a little while. I don't know, maybe it's changed, but I find the suggestions interface in GitHub to just be like unpleasant as a user interface for coding.

Boneskull (40:54.348)
You know what the problem is? The problem is that if you press tab, right? Because you want it to insert a tab or two spaces or whatever. You press tab, your field loses focus.

Boneskull (41:11.352)
So that is like, I feel like that's the core nugget of pain.

KBall (41:19.602)
I'm not going to argue with you.

Amy (41:21.516)
You

Nick Nisi (41:22.174)
we can devolve this into a tabs versus spaces talk right now. You can't use tabs and have that feature.

Boneskull (41:25.664)
No, no, no, has nothing to do with tabs.

KBall (41:26.494)
No, no, no, no. It only has to do with, yeah, tabs in a browser-based editor.

Boneskull (41:35.382)
Right, because like the UI it wants for, you want to be able to navigate with your keyboard and so tabbing goes between the things with the focus number and blah blah blah.

Nick Nisi (41:45.668)
But I'm saying if you're a project where you have your prettier settings to use tabs, you can't use that suggestions feature unless you are making no white space changes.

Boneskull (41:54.67)
Oh, oh yeah, that I didn't think of that. don't use tabs, but if we did, that would be horrible.

Nick Nisi (42:00.77)
Yeah, because you could have that suggestion and then it will fail your formatting check in CI and then you're gonna have to go fix it anyway.

Boneskull (42:09.324)
I wonder if there's something in like the refined GitHub extension that would like disable tab navigation. Not a bad idea.

KBall (42:24.67)
Have any of you been in a place where PR review either the way that it was done or the process around it or anything like that was like completely dysfunctional?

Nick Nisi (42:33.892)
Yes Not on github not in git but yes the You can't you can't figure out what I'm talking about I Mean you can't figure out the place is what I meant but the

Boneskull (42:47.916)
Yeah, well, okay, tell, tell. Spill the tea.

KBall (42:51.368)
Spill the beans!

Boneskull (42:55.01)
Right.

Nick Nisi (42:59.256)
there was, like, I really do think that like stacked diffs or stacked PRs is a great way to do it because you can make things really small and then you can, it's very easy, know, a couple of minutes here per stack or per diff to review, like you can get through reviews very, very quickly, which is awesome. But when you incentivize the number of diffs that you actually commit and make that part of some metric that you're,

using at performance review time. Then you have people gamifying that system and they have the tiniest, most meaningless diffs because they need to get that number up. And then the rest of us with real lives who don't have time to break everything down into individual bits, then we are left to complain. That's all we can do is complain about the system that incentivizes the gamification of this stupid metric.

Boneskull (43:58.06)
I'm sorry, Nick.

Nick Nisi (43:59.365)
You

Amy (43:59.701)
you

KBall (44:03.666)
I feel like the reasons for your departure just keep trickling out.

Amy (44:08.127)
You

KBall (44:12.422)
Okay, now that sounds pretty dysfunctional. Any other stories? War stories from folks?

Boneskull (44:19.466)
I mean open source projects like some very popular ones have dysfunctional review processes.

KBall (44:31.88)
Say more.

Boneskull (44:33.399)
What?

KBall (44:35.528)
Say whore.

Boneskull (44:36.702)
well, maybe there's a project that, you know, they're very egalitarian and you want to send a PR and well, anybody who's a lot of people can go in and say, I don't like this, I don't like that. And, you know, they can block it and of course...

The longer it sits there, the more people show up and more people have suggestions and the longer that sits ad nauseum. yeah. That's bad. You don't want that to happen. You don't want to have like, co-owners that is 60 people and they're all like, gunning for ya, you know? So, uh...

they'll show up sometimes or maybe not. You never really know. so, yeah, I feel like that's dysfunctional. It's like, imagine if your team was that size, right? And you need, and anybody on that team could go in and...

poke around and things there's like some rule that says it has to be You can't you can't merge quickly like you have to wait at least 72 hours or something So so enough of the hundreds of people can go and take a look right so that's that's a dysfunctional review process in my mind for some people they think this is Unfortunate but it is how it should work so

KBall (46:16.35)
I like I've learned in California with our environmental review and building how bad it is when anybody can veto something. Like nothing will ever happen.

Boneskull (46:28.494)
Nothing gets done unless you, well, politics essentially.

Amy (46:37.13)
flip side with open source, it can be so demoralizing when you've spent time to contribute to code and then it just doesn't get reviewed. It just sits. And I'm not talking about like huge features that you're like, well, you should have submitted a PR or discussion to talk about. I'm talking about like obvious things where you're fixing a bug in the documentation. It's not even changing the logic. I've submitted PRs before and then nothing happens.

And so it's like, why did I take the time to pull your repo down, follow your rules for submitting a PR, getting everything formatted correctly and submitted back if it's just gonna sit? That's really frustrating.

Boneskull (47:21.76)
on the flip side, unless there's like some sort of guarantee, If the project is just some dude in his basement or whatever, whatever the stereotype is, you might not have time to look at anything.

And some people, they follow best practices and they add all this documentation and contributing docs and all this crap and they have no real intention of following through. And maybe that's...

that's probably not a person with malicious intentions, maybe more like a company that's just using open source for marketing purposes, but yeah.

KBall (48:22.238)
feel like we could have many episodes on the ways that open source ends up going dysfunctional in one form or another.

Boneskull (48:29.046)
Yeah, yeah, let's save that one.

Amy (48:30.86)
Which is so unfortunate.

KBall (48:34.01)
It is. But we can bring those into others. So I guess to close out our episode on pull requests, what do you like? I don't know. Do you all have like a favorite thing that you bring to whatever company that you're doing around pull requests, like conventional comments or something like that? If you could design the perfect pull request process, what would it look like?

I'm going to throw that to Amy because you've had the least to talk about here given you're not doing them right now. So you're going to a place you want to do pull requests for some reason or another, or maybe you don't. Maybe current state is perfect state. I don't know. But what would that ideal state look like?

Amy (49:05.9)
Yeah.

Amy (49:16.906)
Yeah, I think for me, I put a lot of intention in how I write the pull request to begin with, because that just makes it so much easier for everybody when you, sorry, my kids are at the door. I unlocked the door. Give me just a second.

Amy (49:42.636)
Okay, I'm gonna start at the beginning, so hopefully they'll make the edit easier. So one of the things that I'm really intentional about is how I write the pull requests. So I have a pull request template that I use on all of my PRs. A lot of the things that I'm doing is front-end related, so I'll show what the UI looked like before, and I'll show what it looked like after. If I wrote any tests, I'll document that. I'll explain why I'm making this change. And so hopefully, not only does it make the review process easier,

but it also provides documentation further along in the project if you're trying to figure out why a change was made. And so I think that just makes it all around easier for everybody when those things are there to begin with. And then in terms of like the actual review process, I love the things that we've talked about with conventional comments, conventional commits, and incorporating all those things within the process. I think one thing that we didn't really talk about, well, maybe a little bit, you talked about re-,

removing personality yourself from that. But I think just the level of trust that your team has, that we're all trying to reach the same goal, makes it easier because it's not me versus you. It's not my ideas versus their ideas. It's, we all want this feature to be better and the code base to be better. And so I think what makes that process good is if we can all approach that from the same angle.

Boneskull (51:12.13)
Yeah, like, like, so we have a dock site and a thing that I brought is, hey, let's automate previews of the dock site. And that's kind of like what you said, Amy, where you have the screenshot before and after. so I'm probably going to add a bunch of tooling and automation around that, around, around.

like I added like a, I don't know, an auto labeling thing and just random other things that help. But if there was like one thing that I would, like the perfect PR, like the system would be one in which people actually care. can't make all of your teammates care necessarily.

they may only care about what they're working on. They may not really kind of see the bigger picture or care about the bigger picture. that's what I like. That's my ideal is where we have a team that's...

really looking out for the team as a whole and the project or projects as a whole and not just their individual metrics or what have you. And people who want to review and do a good job at it. I guess that's the ideal and I don't know how I would make that happen. You can't just make it happen. You look into it.

Amy (52:56.084)
No, one thing that you mentioned with the tooling that we did at Redwood was we implemented change sets. I know we haven't really talked at all about that, but we had somebody on the team that was writing the upgrade guides for everything, and they were combing through every single PR and commit that went into a major to be able to write the notes. And I was like, this is ridiculous because one, it's a ton of work, but two, the best person to write that is the person that actually committed the code.

And so we started requiring those chain sets so that they could just use the tooling to help generate the upgrade guide and make that process a whole lot faster.

Nick Nisi (53:38.169)
Yeah.

Boneskull (53:38.22)
Yeah, I've seen that used. I haven't used it myself, so.

Nick Nisi (53:41.39)
That's something that you could probably do with conventional commits, right?

Amy (53:45.153)
Mm-hmm.

Nick Nisi (53:46.756)
Kind of.

Amy (53:49.238)
That's right.

Nick Nisi (53:50.617)
Yeah, it was funny you were talking about like putting a lot of that work into the pull request like, you making sure that the pull request is very clear and understanding and I'm just over here hitting that GitHub copilot button. I really like that. It's putting more context than I ever have.

Amy (54:08.63)
But I think that's okay, right? That's okay. It's still a, it's accomplishing the purpose, because you're trying to communicate to somebody else what's happening. I have no problems with that.

Nick Nisi (54:20.386)
Yeah, but I do think that like the best thing that happened at github is Actions like being able to have all of that built in and having the common having the common like way to do all of that across projects and then being able to just Automate away all of the mundane stuff like you said earlier. We don't we don't comment about formatting anymore that's because of tools like prettier, but it's also specifically because

there is a formatter running in a GitHub action that checks on every commit in the PR. So we don't have to think about that anymore. And the more that we can get rid of the noise, the higher signal we have and the higher you're incentivized to provide that signal and for your voice in PRs to matter. I think that automating away as much as possible is a good thing.

KBall (55:16.338)
bring us home with two things that I really like for PR processes. One, or actually for when you're submitting a PR. So one is we talk about documentation documenting in the code, but the thing that we've started doing, especially as we've leaned into, know, copilot or cursor or other AI generated tools is having a doc. Anytime you're introducing a new feature capability, something like that, having a doc that is both there for people to understand what this does, but also to

give a set of examples that an LLM can use to generate code that utilizes this and having that be a part of our process. you know, at the PR level, you're submitting a thing, where's the doc for it? You know, where's the thing that I can feed as context to cursor when I'm ready for it to go? So that's one big thing. The other big thing I really like is if you're submitting a meaty PR, you should actually pre-review it for people. You should go through and like using the interface, comment like, this is a meaty section, check this out.

Nick Nisi (56:08.119)
Yes.

KBall (56:14.514)
This looks like a lot, but it's just boilerplate. It's from here. Like add your notes to help them understand valuable places to focus and give any sort of important context that doesn't necessarily belong in the code, but might be helpful to understand the thinking.

Boneskull (56:29.164)
Yeah, that's a great practice.

Nick Nisi (56:31.524)
You don't think it should exist in comments then?

KBall (56:34.94)
Sometimes it should and sometimes it shouldn't. It depends.

Boneskull (56:36.886)
Yes, yeah, it depends.

Nick Nisi (56:39.076)
Yeah

KBall (56:42.534)
All right, well, I think we are about at time, but thank you all for joining in this conversation of PRs and PR processes digging into the meat and potatoes of how we get our jobs done.

KBall (56:58.11)
We need an outro music. We really need an outro music.

Nick Nisi (57:02.212)
The White Lotus since they don't have it anymore.

No? Okay.

Boneskull (57:10.638)
Didn't one of you have like a MIDI controller or something? No? That was somebody else that we had on. No, no.

Amy (57:17.034)
I thought Boneskill was gonna play his kazoo.

Nick Nisi (57:20.824)
The nose flute thing.

Amy (57:21.676)
you

KBall (57:21.726)
We gotta use like an AI music generator, generate an outro for...

Boneskull (57:28.724)
Let's do it.

Nick Nisi (57:28.77)
I do have Suno, we'll just make up a song.

That was good, that was really good conversation. Very dysfunctional.

Boneskull (57:36.024)
Mm-hmm.

Boneskull (57:40.878)
man, Nick.

What? The- the- are we still recording? Yeah.

Nick Nisi (57:46.936)
We are. Should I stop?

KBall (57:50.111)
You