GitHub and Code Reviews with Sarah Vessels

Welcome to Syntax. On today's supper club, we have Sarah Vessels from GitHub here. She's a staff software engineer at GitHub, and she's been there for 8 years working on all kinds of things like GitHub Sponsors, discussions, branch renaming, pinned repositories, private graph contributions, and a ton of stuff. And we're gonna be talking a lot about code reviews. We're gonna be talking about GitHub, the process of merging code in a project and working in a collaborative environment. My name is Scott Tolinski. With me, as always, is Wes. Welcome to the show, Sarah.

Hi. Thanks for having me on.

Yeah. So code reviews. We have a lot of developers who listen to this show, and maybe they haven't been a part of big teams or they've been a part of big teams or, you know, they haven't done this process of a code review. I read a a blog post of yours that was extremely good. We'll link to it in the show about the process of code reviews. And I kinda wanted to start off there. So could you explain to developers who might not be aware, you know, what's the deal with a code review? What's the what's the premise? Why would somebody do a code review, and what does that entail?

Okay. So code review is having your team, having other people with context on what you're trying to build, the bug you're trying to fix, anything you're doing in a repository, getting them to look over your changes. This is assuming you're using, like, pull requests, merge requests, if you're on GitLab, that you're not just throwing your code out there to the main branch. So it's it's part of the process. I think you might have automated ESLint. You might have automated deploys to some kind of staging environment, stuff like that to help you assess, like, did I do the right thing? Did I fix the bug? Did I keep from breaking something else? Does this look right? Getting another human involved in the process to get their ESLint. Because 2 eyes can be better than 1. Like, for me, when I get to building something, it's like, I'll get fully into it. And you get into the weeds of the thing, you remember, like, oh, this I have to do this change because otherwise it breaks in this particular way.

And it's like, you can get real caught up in it and lose track of context around it, the bigger picture.

At least on a team, you're not developing in isolation. Like, other people are gonna have to deal with what you put out there. Besides the end users, like, actually using the software that you create, like, the other devs on your team today, the people who will inherit this code in the future, after a reorg, after team change, turnover, whatever.

Like, they need to be able to understand it and iterate on it too. And so code review is a great way of introducing your stuff to them, and it gives you, like, a space to explain what even were you thinking here. Like, why did you build it this way? Why is it ugly in this particular way? Like, there might be a good reason. And you can kinda document that with tests, with code comments, whatever. But some stuff, it's gonna be easier for them to ask on your pull request via a comment, like, why'd you do it this way? And then, like Yeah.

That pull request, that merge request, that acts JS, like, an artifact of the repository as much as any commit or piece of code, like, where you can search for it and look back on it later. And so, like, this is what people were thinking at that time. This is the series of events that led to the code being in the state. These were the issues they were working on. This was, like, the user bug report, like, whatever. Yeah. It's the breadcrumb trail.

Yeah. I always like seeing that in like, being able to go back in time and say, like, what the what the hell were they thinking at this time? Or why did we make that decision? Or even sometimes, I'll I'll put in a poll request, and someone will say, like, that is not the way I would have tackled it. You know? And and getting their 2¢ in, it kinda sucks because it's it's maybe a little bit too late for that, but it's better than having to, like, push something ESLint to merge something, and now you're responsible for that water bottle for the rest of your life. Yes. Yeah. And it is funny because that I mean, that tracks so well with, like, even

Git as a tool or GitHub. Right? It's a view into the the history of your code Bos, and Node reviews are a view into the history of your decision making around code. Yep. What in your professional opinion makes for a successful review process?

So there's a few different kinds of reviews. Like, usually, I think of code reviews as being one of the last gates before this code gets merged. It's when the author of the code change feels like, yeah. This is done. I'm, like, emotionally moved on from this code change. Like, CI, any automated tests they have passed. They are green. Like, everything from my perspective is it looks good. Let me have another human put their eyes on it. Sometimes I'll do kinda request for comment code reviews Wes it's like, I've got a pull request. It's still in draft state. I maybe haven't added tests at all. Things are breaking. But I just want commentary on, is this overall approach even, like, the right idea? Like, am I should I spend any more time going down this branch? And then I would bring in someone early for, like, just kind of a gut check on like the overall implementation.

That kind of thing often I think will get hashed out in, like, the issue before even a pull request is open.

But sometimes it's like, you might do a spike PR to see, like, I'm gonna try, can I talk to this API? Can we store this in this database over here? Will this work at all? And you spike it out. And then like, that's the kind of pull Wes that I would most often get, like, an early request for comment. Like, what do y'all think of this approach? Is this code that, you Node, were it fully tested and, you know, conforming to style guide and standards and stuff, would you want it to be in the codebase? I, like to put up requests very early. So, like, if I've got, like, a bug that I'm fixing, some particular issue, I might add a test that fails and then open a pull request immediately. And it's like, the pull Wes, I'm gonna fix this bug, but all it does right now is just add a failing test. Like, it's not doing anything to fix anything yet. I get that open early and I'm not necessarily gonna request my team on it immediately.

It's more like just a breadcrumb trail to show that, like, this is where I'm going. This is where I'm thinking. This is the stuff I've got on my plate right now. Once I get it to that finished state, then Wes use Slack at work.

Wherever your team is living and communicating, it's like you might depend on GitHub notifications or or other people might have different processes of, like, I know to check my inbox or the list of pull requests that I've been tagged for review on at, like, a certain time each day.

I like going Node notifications based. So, like, if I see someone in my Slack channel, like, hey. I've got this PR up for review. Or if I notice in my GitHub notifications inbox, like, I'll go check it out right then. And then other people, like, doing kind of blocks, like, this is my review time each day. When you say you write the failing test, do you always

do the test and the fix in the same pull request, or is there a a time where you would just pull request a failing test just to get it documented and then go back and fix it when there's some bandwidth for it? Mhmm.

No. So I could see you doing that. Like, maybe you put in, like, a skipped test or a to do test or something. Yeah. But I feel like those get lost, the same as, like, to do comments in the code. It's like you put in that to do comment. Node we'll find them in the GitHub code Bos. And it's like, oh, this was added 10 years ago, and it's still to do. Like, that's that's where they go to die, in my experience. Yeah. I would rather open an issue for something like that. Maybe you chuck it in the icebox part of your project board or something. And it's it's just something capturing the information, the context that, like, this is a problem we should deal with maybe someday. But, yeah, I would typically put the failing tests along with the fix that makes them not fail in the same PR at once. And what about, like, jockeying for

somebody to actually review your code? Because sometimes that's the hardest part JS that, like, everybody's busy, and I don't wanna have to take time out of my day to to work on it. It's I realize it's probably part of people's jobs, but that's something we see a lot is how do you get somebody to actually review your Node? And is there things you can do to make it easier on that person?

Yes. Okay. And that has historically like, every team I've been on, every company that I've been at, that has been an ongoing thing. So it's like a struggle. It's real. Mhmm.

To make it easier, making smaller pull requests helps a lot. Mhmm. If I see that you've got, like, some 500 or greater line change, and it's, like, across multiple files, and I see that you're doing the refactoring, but you're also, like, adding this new functionality, and maybe you're adding tests for that functionality, and the tests are huge. And, like, if I see all that, it's gonna be overwhelming. So I I try and pay attention to my own, like, emotional reaction when I see a pull request from others and let that guide the kind of pull Wes I ask for review on. And I will put together a pull request of my own, and then, like, I'll scan it myself before being like, hey, team, please review this. Like, I look through it, and is there anything where it's like if I had not just spent, you know, days or whatever working on this, I would wonder, what are they doing? Why is this in here? Did they really need to rename or move this code at the same time that they're making this other big functionality change? It's like you can kinda self annotate your own pull requests to make it easier for other viewers or leave those questions that you find yourself ESLint, like, let those guide you into, like, maybe you need to make multiple pull requests here. Maybe this should be split up. I have a blog post that I wrote a few years back for GitHub on the GitHub blog, I can link as well for y'all, about my process of, like, I get everything out in 1 giant PR just to, like, get my ideas on paper, as it Vercel, and then worry about sorting it out into, like, okay, this could really be like 3 separate PRs that are not a pain to review each. As for getting reviews, it's come up on different teams I've been on. You need to kind of engrain it into your team philosophy of Node reviews are important, and like, they become very obviously important when you have any kinda, like, merge restrictions on your repository such that, like, you can't merge this unless it has an approving review. So it's like if you want any kind of shipping velocity, like, you Scott bake that into your team process.

And some automations have been helpful. Like, we use, the GitHub Slack integration for putting, like, pull requests from our team, like, notifying in our Slack channel Wes everyone kinda hangs out. Like, it's a home Bos channel for our team that, like, so and so has a pull request up for review.

And, like, kind of adopting the attitude within your team of code review is more important or more high priority compared to continuing to iterate on your own branches. So, like, stopping what you're doing. It's like, okay. You know, I've got a break. I was gonna, you know, resume working on this PR that, like, I see I've got some failing tests or there's merge conflicts or whatever. Like, let me put that on hold. Let me actually pick up and review my teammate's pull Wes because theirs is further along than mine. Like, if you treat pull requests like we're the time at which you request review, if you do that after you feel done with it as the author and after, like, any automated tests have gone green, then that pull request is gonna be further along than any PR that's still kind of being worked on, failing tests, not fully finished, like, whatever.

And that helps you to see kind of code review, I think, JS more important

than continuing that in process work because it it's closer to being over the finish line. Yeah. That tracks a lot. And and I do I I feel that same way as, like, Wes, sometimes when those code reviews come in, you're like, ah, I'm busy. I'm doing stuff. But viewing it as more important is definitely something I I think I need to do more of. You had mentioned in your post about when you're breaking up PRs into smaller PRs, in smaller commits overall, the process of, like, using feature flags as being something to help with that. Did you have, like, a favorite feature flag system, or is there any way that you prefer to work in feature flags? And maybe I guess, maybe give the audience too, like, a 101 on what that is. Yeah. So feature flags just being a way of conditionally

choosing whether some new feature or change, is experienced by your users. So we tend to, at GitHub, use Flipper, by Jay Newenemacher. Sorry. That's his GitHub username.

John Newenemacher? Anyway, it's it's a public repo on GitHub, Flipper. And you can control it based on the actor. So like, who's the authenticated user? Are they in this feature flag? Should they see this new page that we're building? Should they get the new version of this function that we think is gonna be faster, but maybe is actually gonna have some bugs or something? Like, letting you control the rollout of something such that only 10% of your users, 50% of your users, all the users have this, change enabled. It's really helpful for observability for, like, looking at your exceptions for logs, seeing like, oh, are they hitting this bug that only we know only comes up with the new code? Like, maybe we don't wanna ramp up the feature flag. Like, let's turn it off real quick. It's helpful as, like, a kill switch for if you see a problem that only comes up within the feature flag, turn off the feature flag. Yeah. The controlling, like, who's in it, you you know, maybe it's based on user. Maybe it's a percentage of requests that you have it enabled for, So big on using feature flags to make a pull request safer and less scary for review. Because if any reviewer is looking at your code change, they're like, this could write production. They're gonna be less likely to approve it, and it's gonna be harder to get that approval and harder to get it merged. Mhmm. So it's like, make your stuff less scary. Like, an easy way to do that is Interesting.

Shrink your pull Wes. So it's like, oh, there's. You can wrap your head around it when you look at this thing. So, like, see, it's it's this tiny little baby PR. It's not gonna break any code.

And another way is to to give out. Like, put in a feature flag so that we don't have to go through another deploy. We don't have to ship more code to undo this change. Like, if there's some big problem with it, we just, like, go into this our feature flag management, like, app and turn off the feature flag, like, much safer. And that can give a sense of security to to your reviewers because, like, if especially if you're reviewers, like, often the team that would be reviewing my code change are also the team that would be on call if any of the code in our area, like, is broken and it it could page you after hours. So, like, you wanna kinda guarantee to these same folks that, like, Wes, this is safe. This isn't gonna wake you up at night. You're not gonna get paged because of this breaking change that I introduced. So let's flip a feature flag, or it's clearly, like, innocuous change here. Do you have any stories of that actually happening? Like, something that's something that has broken and if you you've gotten a call in the middle of the night? Nothing has come into mind for, like, particular features.

I feel like it's been more systemic things Wes, like, some access to the database is, like, having a problem or, like, this thing is slow and it's affecting our service as well as other services. I feel like most of the features I've worked on, it's like I'll notice when it's out there, like, during my normal daytime hours, because, like, that's when other people would be using it as Wes, or that's when we'd get the high traffic. And like, oh, there's this exception that's occurring for this, like, percentage of users. And it turns out they're in when they had this particular setup, we're not accounting for this, like, situation, that kind of thing. You can say the feature flags that I most use are for rolling out new features. Like, there is another tool we use, Scientist. I think it's GitHub Scientist JS our repo. I do believe that's public. I'll verify that. That's kinda like a feature flag, but it's it's mainly for assessing, is this new way of doing the same thing more performant? Does it give the right results? And so you set up, like, a scientist experiment to see, like, did you get the same end result when you ran it through the new, like, SQL query that's supposed to give the same results but is faster, that kind of thing. And it's it's a nice tool to use for because it'll end up using you have the control in the experiment. It'll end up using the result from the control, which is, like, the old code that we know works, but maybe has some kind of problem that we're trying to resolve. Yeah. But it will still run the experimental side so you get, like, an idea. It's like another kinda safety mechanism.

Like, a, b testing against your your refactored Node? Is was that was that fair? Yep. Yeah. Is does that run-in production, or is that just something that runs, like, at at the time you'd run tests as well?

I only see it we use it in production with the eye. Because usually, it's it's like, I always see it done with things at scale.

So you're you're run a 100000 people through the new code base and then take a look at the numbers, see if anything weird is happening? Yes. Yes. Okay. Cool. Are we still getting the same results for, like,

repositories you can access? Or, you know, it's it's always around, like, complex queries or expensive queries and seeing, making sure you still get the same end result, but in a more performant way. It's like run through them, but just use the control for now until we trust the experimental code to promote it to being, like, the only code route. Because we've verified with enough, like, actual data that, like, yes, this does return the same result each time, and it is faster.

That's so cool. Yeah. And how do you decide who who goes through the the new code base? Is is it, like do you send employees through it, or is it just a random percentage?

Gosh. I'm trying to remember what's on this. I feel like I've always ramped it up just percentage wise. Like, you watch your exceptions. You because they're getting the control, like, the known result each time anyway, like, it feels safe. It's it will slow things down by design because it's going to run both the control and the experimental branch of the code. So you're running, like, in parallel, these kind of things, but then only using the control. So it feels safe enough, unless your experimental bit of code, like, has some Node reference or other kind of error that it's gonna throw.

Like, it should be fine because you're always getting back the the known result. Mhmm. And it's just a matter of, like, are you going to hit any rate limits? Are you going to slow down or back up some queue by running both branches at the same time? And that might be a reason to, like, keep the rollout percentage low. Like, maybe only run this for, like, 5% of requests or 5% of users. Oh, okay. Is that just for, like like, pulling data? Or could you also use that for, like, a a mutation where, like, you delete a repo or

save something?

I feel like for something like that, you wouldn't wanna do it where it's a destructive action for real. Like, maybe you'd you'd have it riding, like, a flag.

I'd probably have that Yeah. It write data of, like, this is the one we would delete. And then you look at those logs or you look at those records afterward of, like Oh. K. It do Okay. It flagged right ones for deletion in the experimental branch kinda thing. Oh, okay. That's that's smart. Cool. That's really neat. I have not

I I like, I knew this type of thing existed, but it's it's really neat to get a sort of peer into how does it actually work at something so huge like GitHub. Yeah. Wow. That's a whole different scale than we're used to working at. I I have linked, Scientist is a a Ruby library

Mhmm. But there is a JavaScript port of it, so I have linked that as well.

From from the folks at Trello, which is pretty neat. Made it. Yeah. Oh, good. Yeah. Nice. Yeah. So I'll link that up in the show notes. I I'm wondering, like, who on the team should code review? Because Wes and I and CJ, we just kind of, like, assign each other willy nilly. But is there, like, a who who on the team should be the gatekeeper?

I think that's 2 different questions. So, like, who should be doing code review? Everyone.

Who should be, like, the deciding, like, well, so and so approved, so I know I'm good to merge? Like, that's probably team dependent. Who do you trust to have context? Who has the most familiarity with, like, what we're trying to build here, what we're trying to do? Like, that kind of thing. But for who should be doing code review at all, like, I want all the team to be doing it. One trap that I'll see folks fall into is like, oh, well, I see you already got that PR approved, so I don't need to look at it. It's like, no. I want your your comments. You will see different things. You will call out different stuff just by the fact that you're a different person. Like, we all care about Node notice different things. We have different particulars that it's like, oh, this is not gonna be good for accessibility, or I've seen this cause a production problem before. Like, your experience will lead and shape your code review. And I want, like, all the different voices to be involved in that. And I love getting juniors. Like, this is something I will see juniors not wanna leave a review or feel like, oh, I'm not I don't know enough or or something to get in there. No. It's like, that's just a different kind of code review. I want someone who is less experienced, someone who's new to the team to come in with their their new eyes and be like, this is weird. Or I don't understand this. Or this thing is not self evident just from looking at your change here. Like, why would you do it that way? Like, is this just how it works? Is this, you know, hard coded over here? Is this documented somewhere? The kind of questions that they can ask in code review are very good for figuring out how to improve documentation, how to look at stuff. Like, if you've gotten used to it and realized, oh, yes. This is the the ugly clunky bit that we used to be aware of, but we've just kinda gotten used to it. And it's like, that is kind of a pain. And anytime we wanna onboard new people to the team or someone else has to inherit this code base, it's like they're gonna deal with that weirdness too.

Like, making this stuff documented, putting it in an issue is like, yeah. Maybe we'll get to that someday kinda thing.

Even like, I love when people ask questions on a pull request. Like, if they can't if they don't feel comfortable saying like, yeah, this is good enough to be merged, or this looks right, or whatnot, that's fine. Asking questions gives me the platform to get on there and ramble about why I did do it this way. Like, it has to be this way because it will break this other thing if we don't, or because this issue over here is what happens when it's, like, done this other way. Like, this team here learned that we need to implement it in this particular way. And so I've tried doing that here, or this is the guidance for whatnot.

I love just having that space, and then that becomes part of this written record of this pull request that future people will find through searches. They will wonder why JS it written this way? Why did it get implemented this way? And you might see that discussion happening in pull request comments. Like they find your old pull Wes, and they can see, like, well, here's the screenshots of what it looked like when I didn't do it that way. And, you know, at the time, it's like Wes had this constraint or we wanted to ship it by this time. And, like, that's why we chose to do it the way we did. I love just preserving those kinda the history and context, and getting questions from folks who have new eyes or less experienced eyes to the thing is a great way of preserving that context.

You even mentioned reviewing your own code. I have, in the show notes here, Obama putting a medal on Obama, that means. So what's up with reviewing your own code? Yeah. Okay. This is a habit I picked up from my coworker. Paul CSS Smith is his GitHub username.

It's very helpful for like, again, you're super familiar with your branch by the time you finished with it. So you know why you did this, like, weird little bit. You know why you applied that particular class in your HTML or why you used this library instead of that Node. Because you were the one that were was banging your head on it to get it to work originally.

Some of the stuff that you chose to do might not be obvious. It might be necessary, but it not might not be self documenting. And so you kinda make that explicit, like, write it down in a comment. Like, I will comment on my own pull Wes all the time. Like, highlighting, this is the big change.

This was just a refactoring to make it more in line with, you know, this this style guide thing or this bit of guidance that we have of, like, this is the preferred way or doing this bit makes it more accessible. Like, whatever I did, like, even if it's something where if you think about it, like, when you look at the Node, it's like, oh, I see why she would do this. I will sometimes call attention to it to point out, like, this is helping us with this other issue and ESLint back to bits of things. Like this one little line here, I want to highlight JS like, this is load bearing. This is doing something important, or this is, you know, improving something in this way.

And if you want to see all of the errors in your application, you'll want to check out Sentry at century.ioforward/ syntax.

You don't want a production application out there that, well, you have no visibility into in case something is blowing up, and you might not even know it. So head on over to century.i0forward/ syntax. Again, we've been using this tool for a long time, and it totally rules. Alright.

At what point should that be a comment versus a something on the pull request? Mhmm. Do you have any thoughts of of where that line is?

I like to add code comments on tests if the test JS, like, a response to an issue we hit in production, like this is something a real user experience, go look here for details.

I don't often leave Node comments in HTML views. So I will note things just in pull Wes comments there. I don't know. Because it's a fine line. And sometimes, like, this will come up too that one can lead to the other. So, like, often Wes I'm reviewing other people's code, I'll ask them, why'd you do it this way? Or, like, usually, it's it's Wes did you get this? Like, you hard coded this straight. You hard Node Yeah. This URL. Where is that coming from? Especially when making API calls or something. Like, you're using this third party API, and you put in this thing that looks magical and load bearing. Where is that coming from? Is there a documentation URL you can put here as a comment? Oh, yeah. If they reply to me on the pull Wes, like, can you can you put this as a code comment? Like, preserve it to make it easier to find for someone like, when they see the code directly later. If you'd like them to have this information Yeah. Immediately upon seeing the code that it's relevant to JS opposed to, now, which PR added this? Did they note anything in the PR? Let me go track down the issue behind the PR. Like, how many steps should a future reader have to take to get that context?

I'm curious if we were to go through my code base or a syntax code base. There's lots of links to tweets of me just figuring things out in the open Mhmm.

Or, like, Stack Overflow or GitHub discussion of, like, this is weird, but this is why we do it this way.

Totally. Yeah. And a lot of that is from my future self too, Scott just for my my team. Future self does not always remember, what past self was thinking for sure. I'm curious about, like I don't know if this is the case at at GitHub. I I have no idea. But I know a lot of developers have big egos about their code, and they think that their code is the right way every time. How do you deal with egos in the code review process?

Some of it, I think, can be helped with asking for evidence.

Like, if there's a disagreement about like, oh, this is gonna be a problem versus not, getting them to prove it one way or the other with a test. Like, we don't have to just talk about this. We put in a test that shows, like, it will behave in this desired way when we hit it under this scenario.

That can kinda it makes it like less personal. It's like, oh, I'm not like arguing with you, the person. It's like we're just we're gonna make the machine tell us, like, what will actually happen. And that can make it easier, I think. Because you're not it's not like a personal attack. It's just we're going through the bits here. Otherwise, I don't know. Team personalities, it I think it can help to pair with a pair program with someone. To get to know them, like, personally.

It's also helpful, like, so we're a remote company. Being able to meet with your teammates in person, I think, helps a lot to just get to see them as more than just the face that you see weekly in Zoom kind of thing. Mhmm. And get a feel for, like, what they care about, how they respond, like, what is their personality and sense of humor, such that when you see that very dry, bare bones, like, kinda comment that's maybe criticizing on your pull request, it's like you might interpret it differently because you know the person behind it. And it's like, oh, yeah. That that's just how he talks. Like, he's fine. He's not, like, being mean here or anything. He's that's just the direct personality.

So getting a feel for who they are as people, I think that makes it easier when you receive criticism in code review on, like, it's my baby. I wrote this thing. It like, I know it's it works. It's, like, perfect. And then you get, like, some more critical comment of it. It's like you can interpret that better if you've got some kind of relationship established with this person. And then vice versa, I think my feedback can come better like, be better received when my teammates have, like, interacted with me and sat down to a meal with me, if you're able to get together in Vercel, or just having, like, a 1 on 1 on occasion, like, even in just a video call to get to know the people.

Yeah. That's such a good point. I mean, you can definitely read the same text in so many different ways. And by putting someone's personality behind that, I think that changes it substantially. I I always think about that with with emails. And if you don't know the person, like, how can you craft this so it's less ambiguous, your tone. Right? Tone is so hard to convey in that kind of stuff. I know a big part of, like, a successful process of merging code into a code Bos of any size is automation.

What types of automation do you feel is, like, essential? Like, what what would you need? I get a lot of mileage out of

notifications for when something is ready. We use, like, a GitHub action for checking, like, the status of CI, the required builds. And like, okay. This one's it's fully passing now. So all the required tests are okay. And, like, notifying that either by marking the the draft pull request as not a draft. And then in response to that, we have, like, a Slack integration such that, like, Node draft PRs opened by our team get dumped into our team's Slack channel, getting visibility on stuff. The PRs that are ready that are not mine so that I know to review them, the PRs that are ready that are mine so that my team can see them. I do a bit of I have a repo that I've a public repo under my account, so cheshire 137 on GitHub, for moving pull requests around in a project board, because I use a project board to to keep track of my work, and knowing, like, is this thing conflicting with the base branch? Is it kinda daisy chained so it's not against main? It's against some feature branch. And, like, I'm hoping to eventually Node day shuffle it into main. I'm very big on stacked PRs or daisy chained PRs. Keeping track of them that way and knowing, like, this is these are the ones that have a failing test. These are the ones that have conflicts and can't be merged. These are the ones I'm still waiting on reviews from, or these are just ready to deploy. And maybe I should get in the merge queue Node. The visibility, knowing what state everything is in. Let's talk about stylistic.

Minor knit is what everybody used to say, and and I think probably still to a point. But comments on a pull request about indentation, camel case versus snake case. Do you see that still a lot, or is that kind of a it's sort of gone because of linters and formatters?

I'll see stuff where there aren't linters. Yeah.

Yeah.

Like, we don't have linters, I think, on our ERB files at GitHub. And I'll see comments about, like, can we wrap this because it's so long or whatnot.

I'm alright with comments like that, and I'll make them from time to time too. Like, if it's gonna help readability, that's a big thing for me. But I'm never too strict about it. Like, you can do this or not. Take this, you know, as you like it. If you do choose to do it, you don't have to do it in this PR. You can make a follow-up PR that, like, addresses all these things, whatever.

For me, if we care about it, it should be in some automated linter. And if we don't have it automated, then, like, we must not care about it that much. So, like, it's it can't be that big a deal. So I would rather have an automated linter to enforce all that stuff to just, like, take it off the mental load.

And what about, like, formatting for indentation, things like that?

Whatever the style guide says. Like, if we have this in a linter, it's like, I don't have to personally agree with it. I will conform to it. It's like that's what the code base does. Automatic. Yeah. Yeah. And should that the big fight always is should that be done

at author time before the pull request, or should the style the indentation, like, author it however you want, but the the formatter will take care of it when you send the pull request?

We use some pre push hooks, and I've kinda come around to them.

Like, there's some that will will validate, like, linting and, like, are all the robocops passing, that kind of thing. As long as they're fast, that's all I care about. So, like, a pre push hook to stop me from sending my commits to, you know, origin JS fine as long as it's not too slow. Because it's I like to push commits, like, fire it and forget it. I wanna get back to what I'm doing. This is just Yeah. I send it, and I don't wanna have to think about it. So if it, like, cancels it, because, like, oh, you failed this ESLint, that's annoying. And I want that feedback to be very quick if it's gonna do that.

I honestly like, I am happy to have like, once I push my stuff up to GitHub, if some automation runs and makes a commit on my branch that just runs the automated linter, that's fine too. Like, I don't care if I then have to pull before I can push more commits because, like, something changed on origin. I don't know. A lot of people don't like for the bots to push to their branch, and they'd rather just see a failing test. I would rather have the Wes be green and have the bots committing to my branch personally.

Yeah. It seems like that might save some some time. Are there any underutilized tools in this process that you don't feel like people are are caught on to yet? Maybe daisy chaining PRs

in general. A big problem, I think, is just having too large a PR. If you have too many changes, if your diff is too big, you're gonna scare everybody. You're not gonna get fast reviews. You're not gonna get thorough reviews.

Having smaller PRs Wes everything that you are adding in that branch is well tested, it's easy to read, it's in, like, a good place, You Node? It's organizationally in the code. That's preferable to me than having 1 big PR that does the whole feature at once. It's like, invariably, there's gonna be some part of that new feature that's not well tested because your PR is already, like, 2,000 lines. I can't add on this extra test that involves like a snapshot file or a VCR cassette that's gonna be hundreds of lines itself. Like, make room for it by having a new separate pull Wes. And I will use stack PRs, daisy chained PRs, to get all of my work out but in like reviewable sized chunks.

Since like the 1st branch maybe adds the new method with all of its thorough test coverage. And like nothing's calling that method yet so you know it's safe to land. And then, like, a follow-up branch will actually use the method and I'll just base it instead of basing it off a main. You know, Node base it off of the 1st branch that I had the method and then, like, after I've got the the method in, I add the new endpoint that uses it and then I add the UI that, like, exposes access to the new endpoint. Like and I wish people would do daisy chained PRs more because I think it really encourages having smaller PRs that are each reviewable by themselves. And like you can look at the the overall chain of PRs to get, like, the full story of, like, by the time this last PR lands, we will have this whole feature spread out.

Nice. And what about, commits then? Do you do multiple commits per pull request? And if you do, do you squash?

So I do multiple commits per PR, and I don't squash just because that's the the repo setup that we have. Like, I've contributed to open source repos. Rails is the the main one that I can think of Wes they want it all squished down to a single commit by the time the PR merges. So your whole PR will be 1 commit and it might be a ginormous commit.

I don't care for that because I like the pull requests to be like the unit of work and, like, I'm not as concerned about the individual commits.

I like having many commits to work with because that makes it easier to cherry pick particular things.

So it's like I agree. My commit might be, add the test for thing, and then, like, a later commit, actually make the thing. Like, add the new method or, like, refactor this thing or something.

Because I like doing the stacked PRs and, like, how I get to a stacked PR setup JS I'll often start with, like, let's implement the whole feature at once in this 1 giant branch, and then later, I'll tease that out. And it's easier to tease out the different chunks of it if I have the more atomic commits. So, like, I don't care about, like, a clean commit history. I like looking at the the PR as, like, the unit of, like, this is when that feature was introduced.

Yeah. We we've changed the syntax repo over to squash.

And part of it, I don't mind. Like, if if I do a whole bunch of mess or or I make a commit, like, on a Friday, and then on Monday, I pick it up, and then I'll I might squash that just before I put in a pull request. But as soon as you start to, like, branch off or, like, you merge it and you've squashed it and then something else depends on that, it becomes kind of a pain in the butt to to work with it. That's fair enough. Good. I'm glad you said that. You heard it from from GitHub. Wes, I I prefer not to squash. I I I don't know who made that decision. I think it was Ben. Let's not talk with him. Yeah. Ben showed up and changing our our our way of doing things. Well, I think Ben changed it to no merge commit, which I think also flipped on the squashing. I forget. I don't know. Look into it. Yeah. I don't mind it. Yeah.

I feel like looking at the commit history can show you what the person was thinking, like, the struggles they went through, like Yeah. How how you would end up with the final thing. It's like, oh, swear words.

Wes.

The 8 commits trying to get the GitHub action to working. Yeah. Mhmm. Mhmm.

Yeah. So it it tells a story by itself. Maybe it's not the the granularity or the story they're interested in when you're trying to figure out, well, which actual merge, like, broke the thing.

And so I could see why having each PR is like a single commit in your main history could be useful.

So, actually, I'll I'll link to the blog post you did, the, secrets, the GitHub hacks and pro tips and hacks and secrets or whatever. You you described the daisy chaining really well in there. So listeners, if if you wanna see some examples of that that's in that blog post. What kind of, like, branching system do you use over at GitHub? I know that's like a such a a different type of scale, but, like, we just kind of keep a main branch. Mhmm. We and then we're always merging into main off of that the PRs. Is is that pretty typical?

Yes. Yeah. That's what we do as well. It's like, if I'm gonna work on a new feature, if I'm gonna fix a bug, it'll get its own branch. I will branch off of it's still master in our repo. We haven't, like, renamed. There were some dependencies that were leaning on it being hardcoded to master.

Many of the repos I work in do have main JS the main branch. We will tag releases, I believe, for like GitHub Enterprise Vercel. But otherwise just working off of main or master, doing feature branches off of those, and Node daisy chaining the feature branches.

I don't like rebasing because of the destruction that comes with it. Like especially if I'm working with someone else on a branch. It's just like, oh, we're we're working on, like, my teammate's branch. So it's like we'll both be pushing commits to it and whatnot. It's nice if you don't have to do any force pushes because, like, someone will be based off a master, and I don't wanna blow away their these changes and whatnot. See, our shared branches, I think, is great. But, usually, it'll be, like, 1 person to a branch and, like, we might pair together and, like, we're committing to my branch or something, that kinda thing.

Yeah. I'm glad you said that about rebasing. I feel like anytime I I do anything, people Yarn rebase rebase? And it's destructive.

Yeah. Fall off. Yeah. Yeah. I used to rebase a lot, and I I think I got burned, like, once, and I was just like, never again. You know? Yeah. Shouldn't have done that.

Oh, man. Have have you guys ever I'm I'm just curious about inside of GitHub, but do you have any hilarious stories of, like, get going wrong inside of GitHub? Because Git's hard, and I'm sure it's hard for people that work at GitHub.

The most common one that I see is when someone will ping, like, every viewer. So we we do stuff by Node owner. It's like, oh, this team owns the discussions feature. And so you're modifying discussions code, it will tag the discussions team for review.

And people will end up this happens regularly.

Like, new devs, they always feel so bad. It's like, I've got my pull request.

I was branched off of someone else's branch. I wasn't branched off of Maine, but then I merged Maine into my branch when the branch that I was based off of was not up to date with the latest Maine. So suddenly I bring in a 1000000000 changes, everything that's landed in Maine. And it's like, I just ping the world because every team is getting, like, tagged on my PR now. My PR is like 30,000 changes when really it was supposed to be like 30.

And they feel so bad. They're like, I don't know what happened. I'm just gonna close this PR and make a new one. And, like, think it's a great way to kinda meet the other devs. It's like, oh, I see what you're working on here. I got tagged for review on this.

Let me just come say hi to, like, everyone getting pinged now. The rite of passage.

Yeah. Kind of. That's happened to me a 1000000 times where, yeah, you're you're you fork off a branch, you merge main, and there's been 20,000 commits or more since then. And then you see them all as part of your poll Wes, and you go, What did I do? So what's the what's the correct way to solve that and not ping the world?

So if your PR is a draft, I think notifications won't happen. So it's like you can end up with, like, your 30,000 lines of change because you accidentally merged in master. But, hopefully, none of those teams will get actually pinged because your PR is still in draft notes. So every PR I keep that's not based off the main branch, I keep it in draft. Because, like, approvals are gonna get dismissed anyway when the base branch changes. So it's like, I might as well wait. Yeah. But no. The ideal thing JS, like, it's like, dealing with a loom or crocheting or something. You need to pull main through the whole daisy chain. Don't just, like, skip the branch in the middle. Like, you gotta pull main to branch number 1, and then your branch stuff of branch number 1 will pull branch number 1 into branch 2 and all the way through. But, yeah, it's also fine. Like, you'll if you've done this and it's like, I don't know what happened here. I can't clean this up. Let me just close this PR and open a new one and find what the right Bos branch should be based on what I just merged in here.

That's good. Cool. Thank you. You were the team anchor and tech lead on GitHub Sponsors. That's feels like a big old project. How big of a project is that? And, like, what is that like to work on at GitHub, such a, like, a a big, big thing?

Sponsors is fun. It is different too because, like, we are actively taking your money Scott to keep it. Like, let's take your money and then just give it to this other person. So that was weird, because, like, no other feature on GitHub is like that. You know? If the others take your feature or take your money, it's like, that's for GitHub. And so we had to be very careful about, like, marking these funds correctly, and they have to go through the right, like, channel and stuff. And, like, you're dealing with a very legacy like, an aspect of the code Bos that's been there for forever, because, like, GitHub has always been able to take your money. Right? Like, we've always had a billing system, but then GitHub Sponsors came out in, like, 2019, 2020, something like that. So several years after the start of GitHub. So it's like we're using a very old system that had to be updated to work with this novel way of dealing with money. Like, we wanna take your money but not keep it. And also, like, give you receipts and reporting on where that money went. So it's just strange. Like, you'd run into interesting stuff because of that. It was really fun though. So like, I'm not on the sponsors team anymore.

I've been working on just various other things. But it was really fun because of the nice impact that it would have where you'd, like, you'd see posts from, like, Caleb Porzio talking about how he made, like, 100 k a year or something with sponsors and seeing the results of this. Like, when GitHub would sponsor, like, some new set of maintainers or something, or seeing posts coming out from maintainers, tweets from, like, what they've been able to achieve or, like, how this let them, you know, stop working on this other thing and they get to focus full time on open source. Like, I love that. So it was such a, like, feel good thing to work on.

Yeah. And Sanity does a ton there. In Syntax, they they set up they built some kind of tool. I don't know what the name of this tool is, but they set up Syntax so that every single dependency that we use in our code base gets a percentage of Sentry's funds to distribute. So it's like it it allows for stuff like that to support these projects that we just kind of mindlessly use all the time. It's thanks dot dev.

Thanks Scott dev? Yeah.

Excellent. Yeah. Super cool. When you're working on something like that, I know you said you're accepting a lot of money and you have to be careful about the sources. Is legal, like, combing over every single line of code, making sure? Is is that, like, a deeply involved process?

No. It would be more like checkpoints where it's like Wes would implement a thing or we'd have, like, this new feature where we wanna start, like, allowing payments from this area or Wes wanna turn off PayPal payments because of this or whatnot. And there'd be, like, a review process where it's like, we TypeScript, here's what we're gonna do. Is this cool to, like, go forward with? And then, like, after the implementation, like, okay. Let's do a security review. Let's do a privacy review. Let's have folks look through the actual code and the things all at once, like, once it's done from our perspective, kinda like the team implementing it.

So it wouldn't be like an ongoing, like, while we're figuring it out process.

Cool. Well, thank you so much, Sarah. Next, next part of the show is where we get into sick picks and shameless plugs, where a sick pick can be anything you're enjoying, and a shameless plug can be something you'd like to to share with everybody to plug. So do you have a sick pick for us today?

I do. Alright. So this is for gamers.

So I'm big on PC gaming, and, like, I usually have a nice, like, setup. I'm I like to it's like I'll play at my desk after in the day and stuff. But, like, I've recently got a Steam Deck. And I didn't think I would enjoy this thing as much as I have, but it's been wonderful. Like, I've enjoyed playing, like, on a Switch where it's like, oh, let me sit on the couch. Let me go lay down the hammock out in the yard or something. But, like, I didn't think about that for PC games because It's like I have such a nice gaming PC with the big monitor and display. Why would I wanna play on the tiny little thing? But Node, it's very pleasant. Like I like that I can just be it's like I don't wanna sit at a desk after I've been on the computer all day. I'm gonna stretch out and just lay down with this game.

And a surprising thing to me is the kind of games that are so nice on the Steam Deck. So, like, I love Fallout New Vegas, and that JS the oldest game. And it does not look great on, like, my big fancy monitor unless I have some, like, graphics mods and stuff that, like, impacts stability of it because it's, like, it's an older game. It's not gonna look as good as it did back in the day. But you play it on the Steam Deck, like, I've got the OLED screen view, and it's a really nice screen on there. But just by the fact that it's such a small screen, like, it makes these older games look wonderful. And so, like,

it's a great way to play this game again. I love my Steam Deck. Like you said, I I it's almost the only way I I play games anymore. Because I have I don't have a good PC, but I have a a PlayStation.

And I don't play my PlayStation on the TV. I stream it to my Steam Deck. And then the street the Steam Deck battery lasts forever because I don't wanna monopolize the TV if my wife wants to watch TV or something like that. So it's like yeah. Now I got the the Steam Deck. Also, Wes, I don't know if you know anything about this device, but you get access to the actual Linux install on there. So you can do anything that you can do on Linux on here. You can even install Windows on it if you're adventurous enough. But, yeah, this thing rules. Yeah. It is a really solid like, it's just a nice piece of hardware. I love too,

Valve's attitude. Like, if you read in the specs page and stuff, they advertise that they use Torx screws Node, so it's, like, easier for repair and replace and, like, do whatever. And it's it's like, oh, they they, like, are sharing what kind of screw they use because everybody else Torx screws. Like, that's so nice.

Yeah. Just a lot of detail.

Yeah. Moving. I love it. What about that shameless plug?

We just talked about this, but, like, I was gonna say GitHub Sponsors because I feel like Yeah.

There are people that are building.

Like, they've got public repos, and it's like, oh, it's just a stinky little project. Like, I don't even know if people are using it. Like, why would anyone wanna pay me? Get up there on GitHub Sponsors. Like, make a sponsor's profile. Somebody will pay you. Like, folks will look around and just try and find like, oh, here's this dinky little library that I do happen to be using. Let me pay them some money for it. Like, folks would like to pay you if only they were able to. And, like, I don't know. I I like platforms like Sponsors, like Patreon. And, of course, Sponsors has integration with Patreon. So you can pay someone via Patreon if you've already, like, given your credit card info to Patreon or something. I like platforms like that just as, like, convenient centralizations where you're not having to enter your credit card information to some new thing. It's like just this whole site, and I pay for all like, let me support these open source maintainers all in one place. And so I just want more people to have sponsors profiles so we can throw money at them.

Yeah. Absolutely.

And it's it's effortless too. I know, like, we mentioned that syntax, sponsors things. But when I had my own company, it was just so easy to to give a percentage, give it give anything here and there. So yeah. If if you have a a business, you know, help support the things that you're relying on. I love that. So thank you so much, Sarah. This has been incredible getting to to pick your brain on this stuff. It's it's gonna change our process a bit. So, hopefully, if you're listening out there that you can, you can pick up some new tips and tricks from Sarah. So thank you so much. Yeah. Thanks so much for having