WEBVTT NOTE This file was generated by Descript 00:00:04.741 --> 00:00:05.911 Colin: Welcome to Build and Learn. 00:00:05.911 --> 00:00:06.851 My name is Colin. 00:00:07.201 --> 00:00:10.411 CJ: And I'm CJ, and today we're talking about reviewing poll requests. 00:00:10.771 --> 00:00:14.611 If you recall, a couple episodes ago we talked about authoring prs, but today 00:00:14.611 --> 00:00:17.701 we're talking about the other side of the puzzle, which is reviewing them. 00:00:18.421 --> 00:00:22.111 Before we get into that, we wanted to just talk about some upcoming conferences 00:00:22.531 --> 00:00:26.191 and I'm, I'm really jealous because there is this really epic conference that's 00:00:26.191 --> 00:00:28.441 coming up that I really wanted to go to. 00:00:28.501 --> 00:00:30.901 And Colin is , Colin is headed there. 00:00:31.171 --> 00:00:32.954 That is Rails SaaS. 00:00:32.967 --> 00:00:37.597 So yeah, like what is Rails SaaS and when is it happening? 00:00:37.597 --> 00:00:39.039 Colin: Yeah, so let me just pull it up here. 00:00:39.039 --> 00:00:40.989 So it's October six and seven. 00:00:40.989 --> 00:00:42.879 I think this is gonna be a little interesting to see 00:00:42.879 --> 00:00:44.349 when this episode comes out. 00:00:44.358 --> 00:00:47.473 Rails SaaS may have already happened when this episode comes out. 00:00:47.473 --> 00:00:50.566 But we've got a lot of really cool speakers that are talking about the 00:00:50.566 --> 00:00:52.636 intersection of rails and business. 00:00:52.636 --> 00:00:57.616 So you've got some folks who are building SaaS businesses on top of rails, like 00:00:57.676 --> 00:00:59.706 literally, like things like GoRails. 00:01:00.016 --> 00:01:06.393 Hatch box hammer stone.dev, or they're building and releasing gems and things 00:01:06.393 --> 00:01:08.433 as, as a, you know, as a business. 00:01:08.465 --> 00:01:11.629 And then I imagine that there's gonna be a little bit of you know, 00:01:11.659 --> 00:01:14.569 kind of like the stuff that you see with that Saastr conference. 00:01:14.570 --> 00:01:17.546 There's a lot of just, you know, the business side of 00:01:17.546 --> 00:01:18.806 rails, which I'm excited to. 00:01:19.676 --> 00:01:20.116 CJ: Cool. 00:01:20.456 --> 00:01:20.636 Yeah. 00:01:20.636 --> 00:01:24.986 So SaaS, if you're not familiar, is software as a service. 00:01:25.074 --> 00:01:27.564 This is typically like a subscription based thing. 00:01:27.594 --> 00:01:29.634 We all know these and love them. 00:01:29.744 --> 00:01:29.864 If. 00:01:29.864 --> 00:01:33.444 own one and like loathe them if you have a bunch of subscription payments that 00:01:33.444 --> 00:01:34.884 are hitting your wallet every month. 00:01:34.884 --> 00:01:39.543 But yeah, so I, yeah, SaaS in general is something obviously 00:01:39.548 --> 00:01:41.143 that I'm like pretty excited about. 00:01:41.154 --> 00:01:44.137 And yeah, I've been working on some content for Stripe teaching people how 00:01:44.137 --> 00:01:46.535 to collect recurring payments and how to. 00:01:47.010 --> 00:01:48.690 Build a SaaS basically with Rails. 00:01:48.690 --> 00:01:50.430 And that's why I'm like super bummed. 00:01:50.430 --> 00:01:53.100 I'm gonna get, I'm gonna miss this, but we'll be on a family vacation that 00:01:53.100 --> 00:01:56.580 we planned a super long time ago, even before the conference was announced. 00:01:56.585 --> 00:01:59.676 So yeah, but I'll be missing all, all my all my homies that are gonna 00:01:59.676 --> 00:02:03.966 be in, it's in, is it in Hollywood or it's just in LA somewhere. 00:02:04.151 --> 00:02:06.905 Colin: It's in Hollywood and it's it's organized by Andrew 00:02:06.905 --> 00:02:08.615 Culver from Bullet Train. 00:02:08.615 --> 00:02:13.565 So they have like a, they kind of bill it as the Ruby on Rails SaaS template. 00:02:14.070 --> 00:02:14.460 CJ: Hmm. 00:02:14.615 --> 00:02:18.125 Colin: so they give you kind of an out of the box framework for building 00:02:18.175 --> 00:02:19.415 SaaS companies on top of Rails. 00:02:20.390 --> 00:02:24.650 So very cool to see like a thematic conference like this instead of just 00:02:24.650 --> 00:02:28.310 focused on, you know, the language and the tooling and the processes. 00:02:28.310 --> 00:02:32.180 It's gonna be talking about like, how do you use this to be super 00:02:32.180 --> 00:02:33.680 productive in building a business? 00:02:33.727 --> 00:02:33.979 Right. 00:02:33.979 --> 00:02:36.464 Cause building and building something that people want and are willing 00:02:36.464 --> 00:02:37.484 to pull out their credit card. 00:02:37.489 --> 00:02:41.853 And, you know I guess Stripe would be very relevant here because of how 00:02:41.853 --> 00:02:45.743 many subscriptions and, and things are built on top of Stripe and Rails. 00:02:46.443 --> 00:02:51.276 CJ: Yeah, and I think, yeah, in Andrew's bullet train part of the I 00:02:51.276 --> 00:02:54.516 think it's like the pro version or something you can install like a. 00:02:54.519 --> 00:02:57.609 A few Stripe tools that will set up routes for you and a 00:02:57.609 --> 00:02:58.938 few other a few other things. 00:02:58.938 --> 00:03:03.678 I haven't actually bought the paid version of Bullet Train to test it out, but it's 00:03:03.678 --> 00:03:06.888 my understanding that you get a lot of features outta the box with that sort of 00:03:06.888 --> 00:03:09.828 like plug into the bullet train starter. 00:03:09.888 --> 00:03:10.308 So 00:03:10.428 --> 00:03:10.758 Colin: Cool. 00:03:10.758 --> 00:03:14.118 And what other conferences are coming up that you are able to make? 00:03:14.418 --> 00:03:18.218 CJ: Yeah, so I I submitted to speak at Ruby Comp this year. 00:03:18.578 --> 00:03:19.718 I put in two talks. 00:03:20.078 --> 00:03:22.028 I have not heard back yet. 00:03:22.028 --> 00:03:25.418 We'll see by, definitely by the time this episode airs you, I will know 00:03:25.478 --> 00:03:29.255 whether or not those were accepted . But yeah, Ruby Comp is in end of November. 00:03:29.255 --> 00:03:30.365 It's in Houston, Texas. 00:03:30.365 --> 00:03:31.625 Excited about that. 00:03:31.625 --> 00:03:34.085 Excited to see all my Ruby friends. 00:03:34.145 --> 00:03:36.335 Are you going to Ruby Conf this? 00:03:36.950 --> 00:03:38.150 Colin: I was not planning on it. 00:03:38.150 --> 00:03:40.820 I have not really looked at November yet, but. 00:03:41.190 --> 00:03:41.810 CJ: Okay. 00:03:42.290 --> 00:03:42.740 Yeah. 00:03:42.860 --> 00:03:46.135 And then another one that's happening in November is Chirp, which is 00:03:46.135 --> 00:03:47.695 the Twitter Developer Conference. 00:03:47.695 --> 00:03:50.981 I think this is the first time they're having it in many, many years. 00:03:51.011 --> 00:03:52.481 So this is gonna be in San Francisco. 00:03:52.781 --> 00:03:54.041 It's a one day conference. 00:03:54.096 --> 00:03:55.218 So yeah, that 00:03:55.218 --> 00:03:56.328 should be, it should be pretty fun. 00:03:56.718 --> 00:03:57.108 Colin: That one. 00:03:57.108 --> 00:03:58.594 I'm definitely gonna make it down for. 00:03:58.594 --> 00:04:03.137 I, they claimed it was the first chirp in 10 years, but I did see that there was 00:04:03.137 --> 00:04:05.507 like a conference in 2015 that they held. 00:04:05.927 --> 00:04:06.347 This one. 00:04:06.347 --> 00:04:10.037 I'm really excited to kind of see how and what they've learned from, because 00:04:10.147 --> 00:04:15.947 Twitter has a history of kind of letting down developers in the past, and so I 00:04:15.947 --> 00:04:19.907 think they've realized like they've got a repair their relationship with developers 00:04:19.907 --> 00:04:25.427 and we'll see how that goes with Chirp and I'm hoping for a lot of new API features. 00:04:26.582 --> 00:04:30.842 We are using the Twitter API more and more at Orbit, and there's some 00:04:30.842 --> 00:04:35.762 stuff that's like very lacking in the API that are obvious things, so I'm 00:04:35.762 --> 00:04:39.462 excited to see that they're reinvesting in in that relationship and the api. 00:04:40.507 --> 00:04:43.147 CJ: Yeah, so they shipped in API V two recently. 00:04:43.194 --> 00:04:46.753 And as part of that, they have like a whole new OAuth 2. 00:04:47.088 --> 00:04:47.848 Flow. 00:04:47.863 --> 00:04:51.193 So yeah, I've been playing around with the new APIs quite a bit and yeah, 00:04:51.193 --> 00:04:55.093 there's a few things that I would love to see, like way better webhook 00:04:55.093 --> 00:04:57.043 support or webhook support at all. 00:04:57.093 --> 00:04:59.374 There's like no way to get the email address of the person. 00:04:59.374 --> 00:05:01.624 There's like, yeah, a bunch of like little things where I'm like, Oh 00:05:01.624 --> 00:05:01.954 gosh. 00:05:01.954 --> 00:05:02.714 Like, Huh, 00:05:02.884 --> 00:05:03.394 Colin: that we should, we 00:05:03.484 --> 00:05:03.814 CJ: I know. 00:05:03.814 --> 00:05:03.994 Yeah. 00:05:03.999 --> 00:05:04.504 We could dig, 00:05:04.509 --> 00:05:04.915 we could dig into. 00:05:05.164 --> 00:05:06.124 Colin: API audit. 00:05:06.739 --> 00:05:10.053 CJ: Yeah I, I've been using it a lot lately too, just for like fun little side 00:05:10.053 --> 00:05:14.090 project things and yeah, I think they've definitely turned a corner in terms 00:05:14.090 --> 00:05:19.490 of listening to the developer audience and building, starting to build towards 00:05:19.490 --> 00:05:21.290 like what the, what the audience needs. 00:05:21.295 --> 00:05:22.596 So yeah, excited to 00:05:22.596 --> 00:05:23.526 go check it out and see 00:05:23.616 --> 00:05:24.066 how things go. 00:05:24.291 --> 00:05:24.621 Colin: to do it. 00:05:24.621 --> 00:05:26.781 A little audit of, of various APIs. 00:05:26.860 --> 00:05:30.216 Especially like for us, we, we added a feature that we're working 00:05:30.216 --> 00:05:33.576 on an orbit, a little preview here of like being able to send dms. 00:05:34.311 --> 00:05:35.391 CJ: Ooh, Nice. 00:05:35.621 --> 00:05:39.306 Colin: us permission to everything in order to send a dm. 00:05:39.711 --> 00:05:40.321 CJ: I bet. 00:05:40.321 --> 00:05:40.721 Yeah. 00:05:40.776 --> 00:05:42.636 Colin: was like, Why is this not just one? 00:05:43.191 --> 00:05:44.391 Like scope. 00:05:44.841 --> 00:05:45.331 CJ: Yeah. 00:05:45.351 --> 00:05:48.801 Colin: have to have, I could do anything I want on your Twitter account, which, 00:05:49.131 --> 00:05:53.511 you know, we don't, we only send dms and there's only code to send the dms. 00:05:53.511 --> 00:05:56.691 So it's not like we can accidentally do anything on your account. 00:05:56.691 --> 00:06:00.081 But it's, it is that scary oauth screen of like, you're allowing Orbit 00:06:00.086 --> 00:06:01.671 to have access to all this stuff. 00:06:01.671 --> 00:06:02.931 And so 00:06:03.201 --> 00:06:06.861 CJ: Yeah, I think, yeah, I, I would love to do episodes about that, just 00:06:06.866 --> 00:06:11.492 like look at an API and do like a breakdown of the DX and so yeah, 00:06:11.492 --> 00:06:12.302 that would be, that would be fun. 00:06:12.307 --> 00:06:12.692 I think. 00:06:12.932 --> 00:06:17.192 Yeah, there's probably a bunch of APIs too that we're both using just for fun. 00:06:17.288 --> 00:06:19.208 Like for fun and profit, right? 00:06:19.208 --> 00:06:24.586 Like on the side , like YouTube api, Twitter api Orbit API or Stripe api. 00:06:24.591 --> 00:06:26.866 So yeah, it'd be, it'd be cool to go through All that. 00:06:27.556 --> 00:06:31.576 Colin: So I guess in building those APIs, you're gonna have a whole lot 00:06:31.576 --> 00:06:35.746 of PRs in your team, and hopefully there's small reviewable prs like we 00:06:35.746 --> 00:06:39.784 talked about on build and learn.dev/six, if you wanna check out that episode. 00:06:40.354 --> 00:06:43.624 But today we're gonna dive into that other side of the fence 00:06:43.624 --> 00:06:45.274 where you've submitted your pr. 00:06:45.634 --> 00:06:49.114 And you're waiting for someone to review it, that person's gonna pop 00:06:49.114 --> 00:06:52.114 into GitHub or GitLab or Bitbucket. 00:06:52.624 --> 00:06:56.644 And now you have this little bit of science, a little bit of art, 00:06:57.004 --> 00:07:01.864 of reviewing someone else's code and giving constructive feedback. 00:07:01.885 --> 00:07:05.233 Making sure that you kind of match the, the rest of the style of 00:07:05.238 --> 00:07:08.383 the code base, things like that, especially for newer developers. 00:07:08.923 --> 00:07:10.755 So yeah, we can kind of dig in. 00:07:10.755 --> 00:07:10.935 We've. 00:07:11.910 --> 00:07:16.800 A link to a pretty thorough tip like list of tips from thought Bot, 00:07:16.800 --> 00:07:18.050 but that we can kind of review. 00:07:18.050 --> 00:07:22.220 But I think it'd be great to kind of hear like what you first look for when you, 00:07:22.280 --> 00:07:25.220 you know, once you get that notification that it's time to review something. 00:07:25.780 --> 00:07:26.050 CJ: Yeah. 00:07:26.050 --> 00:07:30.983 So first and foremost, something that I think is important is responding quickly. 00:07:30.988 --> 00:07:32.213 Like trying to like actually. 00:07:32.463 --> 00:07:36.393 Look at, if someone opens a pr, try to unblock them and respond to 00:07:36.393 --> 00:07:39.393 their pr because the longer that you wait, the longer that it's gonna 00:07:39.393 --> 00:07:40.623 take them to like ship their thing. 00:07:40.628 --> 00:07:45.154 So yeah, jumping in quick and giving them some feedback or at least telling 00:07:45.154 --> 00:07:46.864 them like, Hey, I'm really swamped. 00:07:47.104 --> 00:07:50.554 This is gonna take me 24 hours or something to get back to you. 00:07:50.634 --> 00:07:53.364 Yeah, so that's kind of just like a, a preface, preface thing. 00:07:53.364 --> 00:07:57.875 But another thing that I tend to look for is any instructions 00:07:57.875 --> 00:08:00.455 in the description of the pr. 00:08:00.485 --> 00:08:03.755 Like, do they have pointers for what they're looking for? 00:08:03.755 --> 00:08:07.465 Do they have requests for reviewing specific files? 00:08:08.045 --> 00:08:12.425 And, you know, oftentimes the description will say something like, Oh, a lot 00:08:12.425 --> 00:08:17.465 of this was auto generated, but can you please look at file X to you? 00:08:18.260 --> 00:08:21.920 You know, make sure that there's a really good thorough review of security 00:08:21.920 --> 00:08:24.480 on, on this like section of whatever. 00:08:24.519 --> 00:08:28.945 So yeah, looking at the description and yeah, being timely in your reply, I guess 00:08:28.945 --> 00:08:30.155 is like the first thing I would say. 00:08:30.225 --> 00:08:34.245 Colin: Do you guys have like a how to test section of prs, like in your template? 00:08:34.680 --> 00:08:35.250 CJ: We do. 00:08:35.280 --> 00:08:35.610 Yeah. 00:08:35.610 --> 00:08:39.990 So we, it's actually not how to test, it's how it was tested. 00:08:40.170 --> 00:08:42.840 So we ask people Yeah. 00:08:42.845 --> 00:08:47.008 Like to, you know, outline exactly how you tested it, and that could be, 00:08:47.008 --> 00:08:49.828 I wrote a bunch of automated tests, or if it's a change to like a docs 00:08:49.828 --> 00:08:53.158 page, it could be like, here are the screenshots of the changes that 00:08:53.158 --> 00:08:55.378 I made, and also here's a link to. 00:08:56.218 --> 00:08:58.978 The staging server, where the docs are or whatever. 00:08:58.978 --> 00:08:59.368 So 00:08:59.435 --> 00:09:03.047 Colin: Cause along the lines of not blocking them, like some of our 00:09:03.047 --> 00:09:07.517 PRs require, like as a tester, like we know that the other person who 00:09:07.517 --> 00:09:10.187 wrote it has already gone through those steps, like you mentioned. 00:09:10.187 --> 00:09:14.537 But I do like to like then run them on my machine, Right. 00:09:14.567 --> 00:09:19.057 Or run them on testing, like a staging or integration server. 00:09:19.057 --> 00:09:22.160 But for some of those, you know, hopefully they're small reviewable 00:09:22.160 --> 00:09:25.070 prs, but some of 'em you have to like do a little bit of world building or. 00:09:26.165 --> 00:09:29.105 A script that creates all of the, the scenario that, that 00:09:29.105 --> 00:09:31.265 you need to test this thing in. 00:09:31.265 --> 00:09:35.015 And sometimes there's like a bug or something that's hard to replicate. 00:09:35.075 --> 00:09:38.916 And so relying on tests is sometimes all you can do. 00:09:38.921 --> 00:09:42.756 But you know, for us, we do like to, especially with integrations, we 00:09:42.761 --> 00:09:47.376 want to test them on another machine so that it's like, hey, like some 00:09:47.376 --> 00:09:50.346 environment variable didn't make it into the encrypted credentials. 00:09:51.126 --> 00:09:55.071 Something like just about their machine or their setup is just not quite the same. 00:09:55.071 --> 00:09:58.482 With integrations, we end up using things like ngrok and relying on web 00:09:58.482 --> 00:10:00.207 hooks and stuff like that a lot too. 00:10:00.207 --> 00:10:03.520 So like making sure that it's like, Hey, yeah, I caught a web hook 00:10:03.520 --> 00:10:06.730 but it didn't hit the right path on my machine for some reason. 00:10:06.800 --> 00:10:09.339 Or just like some sort of environmental kind of ruling 00:10:09.339 --> 00:10:10.809 out environmental differences. 00:10:11.649 --> 00:10:13.869 which for some companies, if you're running in like Docker or 00:10:13.869 --> 00:10:15.339 something, those may not even be. 00:10:15.357 --> 00:10:18.998 We run pretty bare rails on, you know, Mac, so, 00:10:19.643 --> 00:10:19.943 CJ: Got it. 00:10:19.943 --> 00:10:20.093 Yeah. 00:10:20.093 --> 00:10:25.613 I was gonna ask, so our, our development environment is actually like external 00:10:25.613 --> 00:10:28.133 boxes that live inside of aws. 00:10:28.223 --> 00:10:32.895 And then also once we push a pr it will spin up staging 00:10:32.900 --> 00:10:36.765 environments just with that PR so that you can link to it from the. 00:10:37.425 --> 00:10:38.625 From the PR itself. 00:10:39.015 --> 00:10:41.925 And the nice thing about that is that the environment inside of 00:10:42.375 --> 00:10:45.645 AWS when you're building and when you're in staging and when you're in 00:10:45.645 --> 00:10:47.535 production is all the same, right? 00:10:47.535 --> 00:10:50.775 Like, it's gonna be on the same boxes, the same tools, the same like everything. 00:10:51.075 --> 00:10:56.171 So that is like, I don't know, it's, it's, I would say it's a nice experience. 00:10:56.171 --> 00:10:57.925 But it's also like super challenging. 00:10:58.285 --> 00:11:01.565 . There's a lot of like DevOps overhead stuff to get that set up on a team. 00:11:01.565 --> 00:11:01.740 So 00:11:01.980 --> 00:11:03.790 Colin: review apps on Heroku for that. 00:11:03.797 --> 00:11:07.757 So we get a little bit of that out of the box without having to have a 00:11:07.877 --> 00:11:09.846 whole DevOps team build that for us. 00:11:09.846 --> 00:11:11.838 So we do that when it's necessary. 00:11:11.898 --> 00:11:13.968 Some changes, like again, docs changes. 00:11:13.968 --> 00:11:15.918 We don't need to spin up a review app for that, 00:11:16.758 --> 00:11:17.088 CJ: Mm. 00:11:17.208 --> 00:11:19.098 Colin: like read me updates and stuff like that. 00:11:19.098 --> 00:11:19.848 Those are fine. 00:11:19.848 --> 00:11:22.938 We can review them for accuracy and stuff like that. 00:11:22.938 --> 00:11:25.795 But yeah, I think that's a, that's a pretty good process to get. 00:11:26.665 --> 00:11:30.667 CJ: So going, going back to timeliness, I assume that your team is so globally 00:11:30.672 --> 00:11:35.137 distributed, that sometimes your reviewer will be, you know, 12 hours 00:11:35.137 --> 00:11:37.057 difference in terms of time zones. 00:11:37.537 --> 00:11:39.997 Is that the case or are you mostly kind of like working with people 00:11:39.997 --> 00:11:41.167 that are closer to your time zone? 00:11:41.812 --> 00:11:43.972 Colin: Yeah, this is something that's kind of interesting for us. 00:11:43.972 --> 00:11:49.582 We do tend to have, like when I, my old team, I was the only person in the US and. 00:11:50.527 --> 00:11:54.757 It would be this thing where like by the end of my day I try to review any open 00:11:54.757 --> 00:12:00.937 prs for Europe so that they're waking up to a reviewed pr, that then they can, you 00:12:00.942 --> 00:12:06.577 know, either make changes or merge and then on their end, you know, I, on my end 00:12:06.577 --> 00:12:10.897 I'm trying to get mine, my code done so that when they also wake up, they have a 00:12:10.897 --> 00:12:12.877 PR for me that they can take a look at. 00:12:13.177 --> 00:12:17.527 And sometimes there might be this back and forth conversation around, Maybe it's a 00:12:17.527 --> 00:12:21.466 draft PR with questions because it might be a newer area of the code base for me. 00:12:21.856 --> 00:12:24.016 And so I do have to kind of think about that. 00:12:24.316 --> 00:12:26.776 It really forces me that time box might work too. 00:12:26.781 --> 00:12:31.876 It's like if I don't get to the thing I'm hoping to today, then I'm not 00:12:31.881 --> 00:12:35.686 gonna have the PR open and ready for review by the beginning of their day, 00:12:35.686 --> 00:12:39.196 which means now we're gonna have a whole nother 24 hour cycle of that. 00:12:39.766 --> 00:12:40.156 CJ: right? 00:12:40.156 --> 00:12:43.756 I is there Is there like a, an hour or two where you have overlap 00:12:43.761 --> 00:12:45.526 and you can kind of like talk live 00:12:45.776 --> 00:12:46.196 and. 00:12:46.516 --> 00:12:46.666 Colin: Yeah. 00:12:46.666 --> 00:12:47.136 We've got. 00:12:47.941 --> 00:12:48.901 In my morning. 00:12:48.959 --> 00:12:52.789 I have someone in, in Israel that I, we actually have a lot 00:12:52.789 --> 00:12:55.294 of overlap during the day because she works really late at night. 00:12:55.294 --> 00:12:57.947 And so she kind of has a different setup of her day. 00:12:57.947 --> 00:13:02.147 And so we do get that and like, we just restructured our team, so we do have more 00:13:02.152 --> 00:13:07.187 people in different, like I have more US folks on my team now, so you know, 00:13:07.187 --> 00:13:09.074 that's gonna change a little bit as well. 00:13:09.074 --> 00:13:13.589 So, you know, once we do that, It's nice to have those comments. 00:13:13.649 --> 00:13:17.009 I've noticed that, especially because it's a little bit async, someone might 00:13:17.009 --> 00:13:20.617 open a pr and then I guess this is on the more of the creating the PR side 00:13:20.617 --> 00:13:25.447 of it, but they'll preempt the review by going into the, like the filed diff 00:13:26.117 --> 00:13:26.317 CJ: Mm-hmm. 00:13:26.682 --> 00:13:28.507 Colin: add their own comments. 00:13:28.597 --> 00:13:34.057 So like calling out like, Hey, could you specifically look at this thing that I 00:13:34.057 --> 00:13:38.557 either had a struggle with or like, I think this is the best way to do this. 00:13:38.587 --> 00:13:40.318 You know, can you just like do a gut check? 00:13:40.318 --> 00:13:45.793 . You know, if there's any sort of things that need to be specifically tested 00:13:45.793 --> 00:13:50.323 from a security or like web hook and like ingest type of thing, then they 00:13:50.328 --> 00:13:54.733 might call that out Especially with integrations, it's like you have to 00:13:54.738 --> 00:13:59.273 do this world building again of, of having the development integration. 00:13:59.273 --> 00:14:01.908 So like, you know, maybe it's developer Twitter account. 00:14:01.914 --> 00:14:04.628 Connected to the developer app versus the staging app. 00:14:04.628 --> 00:14:05.678 Versus the production app. 00:14:05.678 --> 00:14:07.178 So there's just a lot of variables at play. 00:14:08.263 --> 00:14:08.463 CJ: Mm-hmm. 00:14:09.233 --> 00:14:13.913 . So when you're, when you're looking at security stuff, In the back of your mind, 00:14:13.913 --> 00:14:16.763 do you have or Yeah, I guess like in the back of your mind, do you have certain 00:14:16.763 --> 00:14:20.873 things that you look for when it comes to security or just like generally thinking? 00:14:21.353 --> 00:14:25.523 Colin: so if it's general web application security things, you know, we trust that 00:14:25.528 --> 00:14:29.243 the team, you know, can look at those things and if, if it's not an area of 00:14:29.243 --> 00:14:31.253 expertise, we can tag in somebody else. 00:14:31.613 --> 00:14:34.553 If it's like something related to like SOC2 or something like that, 00:14:34.553 --> 00:14:36.323 then it might be someone who's. 00:14:36.598 --> 00:14:41.038 Specifically like security engineered type of person that would look at that stuff. 00:14:41.056 --> 00:14:42.286 You know, and that's not just at orbit. 00:14:42.286 --> 00:14:45.886 That would be like at past companies where you have the luxury of having 00:14:45.886 --> 00:14:48.636 somebody who can specifically do that. 00:14:48.706 --> 00:14:51.881 And then you also hopefully have some of these like scanners and things too 00:14:51.881 --> 00:14:53.741 that are running on your, on your. 00:14:53.797 --> 00:14:58.042 PR and CI that are looking for common vulnerabilities and things 00:14:58.042 --> 00:15:01.192 like that in the code base as well, like strong prams and things 00:15:01.192 --> 00:15:02.992 like that come to mind for rails. 00:15:03.053 --> 00:15:06.138 You know, CORS, stuff like that, that that'll pop up. 00:15:06.858 --> 00:15:09.438 CJ: Yeah, cross site scripting, vulnerable, like I'm trying to 00:15:09.438 --> 00:15:11.478 remember the name of it, but yeah, there's a bunch of tools where you 00:15:11.478 --> 00:15:15.828 can just say like, point this at my app and like try to run all the 00:15:15.828 --> 00:15:17.688 vulnerability scanning things on it. 00:15:17.750 --> 00:15:18.185 Yeah. 00:15:18.311 --> 00:15:18.801 Cool. 00:15:18.801 --> 00:15:21.261 Colin: like one of one of them was called like Hound, I think. 00:15:21.441 --> 00:15:22.656 CJ: Hound interesting. 00:15:22.702 --> 00:15:28.242 Yeah, I think like looking, the one big one that comes to mind is SQL injection 00:15:28.242 --> 00:15:35.065 attacks, which really, if people are not parameterizing or like not sanitizing user 00:15:35.065 --> 00:15:38.815 input before they're passing them down to sql, then you kind of get in trouble. 00:15:39.145 --> 00:15:43.075 But yeah, I feel like the rail strong params pattern has, you know, if 00:15:43.075 --> 00:15:47.065 you kind of like stick to the, the normal patterns, you're usually okay. 00:15:47.070 --> 00:15:50.575 But I have seen a couple times where maybe it's like a search endpoint and 00:15:50.575 --> 00:15:54.535 you're writing a custom where clause that has a bunch of likes and whatever. 00:15:54.535 --> 00:15:59.485 If you're not passing like the right parameterization, then it's easier to get 00:15:59.579 --> 00:16:01.599 User input and pass that right down to SQL 00:16:01.604 --> 00:16:04.154 Yeah, so, Okay. 00:16:04.304 --> 00:16:05.174 Super cool. 00:16:05.174 --> 00:16:07.344 What else we got here, I guess? 00:16:07.344 --> 00:16:09.170 Colin: What about the content of the code itself? 00:16:09.170 --> 00:16:12.470 So like, let's say now it's time to jump into the code. 00:16:12.680 --> 00:16:16.670 What sorts of things, you know, when we've talked about this in past episodes, 00:16:16.670 --> 00:16:23.300 but like programming is so opinionated, so how do you approach this when you 00:16:23.300 --> 00:16:25.280 know someone spent a lot of their time. 00:16:25.685 --> 00:16:26.525 On the code. 00:16:26.555 --> 00:16:30.591 And I think as much as we like to try to disconnect our ourselves from 00:16:30.591 --> 00:16:33.501 it, it's something that we've spent a lot of time on and, you know, by the 00:16:33.501 --> 00:16:38.331 time it comes time to ask for review, hopefully it's quote unquote done. 00:16:38.331 --> 00:16:43.081 So how do you kind of approach those things that might be trade offs or things 00:16:43.081 --> 00:16:44.711 that you might, that might stick out to. 00:16:45.371 --> 00:16:51.611 CJ: I think going into every PR was an open mind and trying to come at it from 00:16:51.616 --> 00:16:54.071 a, like a perspective where you're. 00:16:54.551 --> 00:16:58.541 About what the original author intended can be helpful. 00:16:58.691 --> 00:17:03.381 And then also after you really understand what they were going for 00:17:03.418 --> 00:17:06.898 then maybe start to ask questions. 00:17:06.898 --> 00:17:10.678 Like ask leading questions instead of like criticizing, Right? 00:17:10.678 --> 00:17:14.908 So kind of like, Oh, I see that you were doing it this way. 00:17:14.968 --> 00:17:16.048 That's really interesting. 00:17:16.408 --> 00:17:18.718 Did you think about this approach? 00:17:18.718 --> 00:17:24.436 Or You know was there a reason why you know, you're taking this in, taking this 00:17:24.436 --> 00:17:28.096 as an argument instead of instantiating it inside of the class or whatever. 00:17:28.096 --> 00:17:30.766 Like there's an opportunity to talk about design patterns. 00:17:30.766 --> 00:17:34.767 There's also opportunity to talk about, like learning the code base 00:17:34.767 --> 00:17:39.337 itself as a, as a result of the pr. 00:17:39.337 --> 00:17:42.983 So, yeah, I guess maybe to answer your question, I think coming at it from a 00:17:43.193 --> 00:17:46.883 perspective of like, what can I learn from this code that I'm about to. 00:17:48.458 --> 00:17:48.908 Colin: Definitely. 00:17:48.908 --> 00:17:49.148 Yeah. 00:17:49.148 --> 00:17:53.468 The, the asking questions is interesting to me cuz I, I do think it's better 00:17:53.468 --> 00:17:57.188 than like, making demands or like saying like, change this to this cuz 00:17:57.998 --> 00:18:01.508 you don't necessarily, like you said, you don't know where, why they had to 00:18:01.508 --> 00:18:03.308 make some of the choices that they made. 00:18:03.314 --> 00:18:10.169 The asking questions thing I find is the art piece because it's sometimes 00:18:10.169 --> 00:18:14.729 when I'm writing a question on a pr I'm trying not to be pa like patronizing. 00:18:15.554 --> 00:18:18.554 It's like how do we get to the topic of this? 00:18:18.554 --> 00:18:23.684 Like sometimes there's a more direct way of saying things, but you do wanna 00:18:23.684 --> 00:18:26.114 avoid like judgment or assumptions. 00:18:26.654 --> 00:18:30.914 And when I've reviewed like a lot of prs in a row, it starts 00:18:30.919 --> 00:18:31.874 to feel a little bit weird. 00:18:31.874 --> 00:18:35.414 And this might just be in my head, but like, it's like rather than me just 00:18:35.414 --> 00:18:37.994 like calling it out, it's like, why don't, what do you think about this? 00:18:37.994 --> 00:18:39.344 Or have you tried this? 00:18:39.344 --> 00:18:40.034 And things like that. 00:18:40.034 --> 00:18:41.024 Because I'm. 00:18:42.494 --> 00:18:46.184 In some ways you wanna, you're not trying to lead them to the right answer, I guess. 00:18:46.184 --> 00:18:47.414 It's like, it's not the goal. 00:18:47.414 --> 00:18:51.794 The goal is just like to figure out, like, did they consider something else? 00:18:51.854 --> 00:18:55.874 Was there an issue with like, you know, is there a reason why this 00:18:55.874 --> 00:18:59.444 doesn't match the, the code that's over here that does the same thing or. 00:19:00.824 --> 00:19:01.904 Those kinds of things. 00:19:01.933 --> 00:19:05.563 And I would say like, if that's the case, like always ask for clarification. 00:19:05.563 --> 00:19:06.823 Like you could just ask a question. 00:19:06.823 --> 00:19:09.433 You don't have to be like, I need to get this review done. 00:19:09.433 --> 00:19:13.003 Maybe it's like, Yeah, I'm gonna need some more info to, to do this. 00:19:13.007 --> 00:19:14.442 But how do you feel about that? 00:19:14.442 --> 00:19:18.282 Like in terms of like when you've written something, are questions helpful or do 00:19:18.282 --> 00:19:20.532 they, do they feel like that sometimes to. 00:19:21.597 --> 00:19:25.733 CJ: So I think they are helpful in terms of like the ego part of 00:19:26.213 --> 00:19:28.133 receiving a code review, right? 00:19:28.133 --> 00:19:29.603 If someone is like, Oh, this is wrong. 00:19:29.603 --> 00:19:31.283 You should change this, it. 00:19:32.033 --> 00:19:38.608 Definitely chips away at like your identity that is tied inextricably from 00:19:38.608 --> 00:19:42.838 the code that you just published on GitHub and asked for a kind and gentle 00:19:42.838 --> 00:19:47.368 review and someone jumps in there with a bunch of hyperbole and never do 00:19:47.368 --> 00:19:49.558 this, and why didn't you just do that? 00:19:49.563 --> 00:19:52.540 And you know, this is overcomplicated or whatever. 00:19:52.540 --> 00:19:56.320 Like those kinds of, those kinds of comments can really like beat you down. 00:19:56.770 --> 00:19:58.300 I will say that I have worked. 00:19:59.170 --> 00:20:04.900 Several people who do their prs like that, and they're, they're like insanely 00:20:04.900 --> 00:20:10.780 direct, very hyperbolic and like really, really explicit and d and I don't 00:20:10.780 --> 00:20:12.670 know, very blunt about their feedback. 00:20:13.060 --> 00:20:19.375 And it was extremely painful, but, I think I, I also like got better at 00:20:19.525 --> 00:20:24.025 like, building up a shield of like, Okay, I'm publishing this, it's fine. 00:20:24.055 --> 00:20:27.505 Like I've gotta get , like torn up or whatever. 00:20:27.505 --> 00:20:31.937 But like I know that at the end of the day, like I, my hope is that that 00:20:31.937 --> 00:20:36.647 criticism is coming in and that I can receive it and become a better developer. 00:20:36.947 --> 00:20:40.667 So like, I always had to like try to take that perspective as the author 00:20:41.057 --> 00:20:43.367 and because I know that was so painful. 00:20:44.322 --> 00:20:48.557 and also it required a lot of like building up this thick skin. 00:20:48.977 --> 00:20:55.607 I always tried to like approach PRS that I am reviewing with that, that own like my 00:20:55.607 --> 00:21:00.947 own personal experience and try to like be really empathetic and really humble 00:21:00.947 --> 00:21:03.493 and really like, Okay well let's be. 00:21:03.562 --> 00:21:06.772 Let's use this as a teaching moment instead of like a, just 00:21:06.772 --> 00:21:07.912 make sure the code is fine. 00:21:08.002 --> 00:21:14.062 Like you have an opportunity to make other developers on your team know and 00:21:14.062 --> 00:21:17.512 understand the things that you know, and you can do that through the PR 00:21:17.512 --> 00:21:21.172 instead of just kind of like, and you can do it in like a tactful way instead 00:21:21.172 --> 00:21:22.964 of just being kind of like, Yeah. 00:21:22.964 --> 00:21:23.547 Attacking. 00:21:24.062 --> 00:21:28.287 Colin: that you had to go through that to then find the compassion, right? 00:21:28.287 --> 00:21:31.857 Like you would've probably had that compassion for your reviews anyway, 00:21:31.857 --> 00:21:35.277 but like that experience of having to build that shield, like did that 00:21:35.277 --> 00:21:37.887 also cause you to second guess? 00:21:37.887 --> 00:21:39.197 Like that it's ready to be puff. 00:21:39.397 --> 00:21:44.137 Submitted or like, I imagine like you're gonna check triple check everything when 00:21:44.137 --> 00:21:46.717 you are like, Oh my God, I don't know what negative feedback I'm gonna get. 00:21:46.747 --> 00:21:46.987 Cuz 00:21:47.467 --> 00:21:49.867 to me like, that sounds like an awful reviewer. 00:21:49.943 --> 00:21:53.993 Even if it's coming from a good place, like, you know, maybe they 00:21:53.998 --> 00:21:57.053 just were rushed and they shouldn't have reviewed it like right before 00:21:57.053 --> 00:21:58.643 they left the office or something. 00:21:58.643 --> 00:22:02.603 But that's, that's a rough experience, especially for newer developers who 00:22:02.603 --> 00:22:06.273 may not know that it's like, this isn't about not to take it person. 00:22:06.883 --> 00:22:08.173 But it is gonna be personal. 00:22:08.173 --> 00:22:09.673 Like there's no way to 00:22:09.913 --> 00:22:10.243 CJ: Yeah. 00:22:10.813 --> 00:22:10.993 Yeah. 00:22:10.993 --> 00:22:12.793 The, a couple things came out of it. 00:22:12.793 --> 00:22:17.023 One is when I would publish a pr, I would read all of the code 00:22:17.023 --> 00:22:19.513 myself and review it myself. 00:22:20.143 --> 00:22:24.043 For myself, like, like I would say, yeah, before you push the button that 00:22:24.043 --> 00:22:26.233 says, you know, submit pr, whatever. 00:22:26.233 --> 00:22:30.013 You can see the diff go through that diff and Polish. 00:22:30.613 --> 00:22:35.233 Take your last like chance to polish it up and anything that I started to build 00:22:35.233 --> 00:22:37.213 up this sense of like, okay, I think. 00:22:37.798 --> 00:22:41.548 This is the area of code that I'm going to receive the most criticism about. 00:22:41.608 --> 00:22:44.008 And so then like I would go back and maybe make a little, 00:22:44.008 --> 00:22:45.058 like clean it up a little bit. 00:22:45.118 --> 00:22:48.448 And when I didn't do that, I always got comments on those parts where I was 00:22:48.448 --> 00:22:53.081 like, Okay, like I think maybe this area might, you know, need some improvement. 00:22:53.148 --> 00:22:54.768 Colin: Like negative reinforcement though. 00:22:55.203 --> 00:22:57.303 CJ: yes, , it was, it absolutely 00:22:57.303 --> 00:22:57.633 was. 00:22:58.293 --> 00:22:58.683 No 00:22:59.008 --> 00:23:02.688 Colin: it probably made you a much better developer through fire. 00:23:02.748 --> 00:23:03.048 Right? 00:23:03.048 --> 00:23:05.013 It's not not through yeah. 00:23:05.133 --> 00:23:05.943 Ooh, that's rough. 00:23:06.468 --> 00:23:06.678 CJ: yeah, 00:23:07.593 --> 00:23:10.893 Colin: yeah, I, I come from a background of not always having, 00:23:11.448 --> 00:23:16.068 Developers to review my code, and so I would have to do that anyway, right? 00:23:16.068 --> 00:23:19.218 I'm like, I'm literally doing PRS to myself and then going 00:23:19.218 --> 00:23:22.908 and looking at the diff and like reviewing it like the next day. 00:23:22.998 --> 00:23:24.318 So like with fresh eyes. 00:23:25.248 --> 00:23:27.648 That was like when I was the only developer at a company. 00:23:28.038 --> 00:23:30.678 You know, I wonder if that's even a service. 00:23:30.678 --> 00:23:33.648 It'd be amazing to like, have like a contract code reviewer 00:23:34.748 --> 00:23:35.228 CJ: Mm-hmm. 00:23:35.268 --> 00:23:38.748 Colin: Can you just review my PRS as an external, you know, person? 00:23:38.835 --> 00:23:40.935 But I always had to develop that. 00:23:40.935 --> 00:23:44.415 And because of that, I never really got a lot of those, like those negative reviews. 00:23:44.420 --> 00:23:48.135 But because of that too, I never developed a really strong muscle for 00:23:48.135 --> 00:23:51.225 like what to look for in other people's. 00:23:52.200 --> 00:23:53.580 And I've had to develop that. 00:23:53.580 --> 00:23:56.812 You know, at orbit we have plenty of developers to review each other's code. 00:23:56.838 --> 00:24:01.878 And I've learned a lot on both sides from reviewing other people's code. 00:24:01.878 --> 00:24:04.698 I've learned a lot of things where I'm like, I didn't know Ruby could do that. 00:24:05.238 --> 00:24:06.978 know that Rails could do that. 00:24:07.338 --> 00:24:11.118 But then on the flip side, just seeing like what comments people do. 00:24:11.538 --> 00:24:14.157 You know, point out you know, especially if someone knows the 00:24:14.157 --> 00:24:17.277 code base in a different area better and they're like, Hey, we already 00:24:17.277 --> 00:24:19.077 have something like this over here. 00:24:19.767 --> 00:24:21.147 you considered reusing this? 00:24:21.147 --> 00:24:21.837 Things like that. 00:24:21.837 --> 00:24:26.128 So and I think, you know, some of the, the tips from thought bot when 00:24:26.128 --> 00:24:29.908 you're reviewing code, where like communicate things that you feel 00:24:29.908 --> 00:24:31.528 strongly about and those that you don't. 00:24:31.528 --> 00:24:35.038 So like maybe there's something else, like, we really shouldn't merge. 00:24:35.908 --> 00:24:41.008 Part as is, or like, Hey, over here maybe we, we, we should clean this up. 00:24:41.008 --> 00:24:44.728 But maybe that's like in a future pr, like, let's not block this one 00:24:44.728 --> 00:24:47.728 and ship because it's not gonna cause any downstream problems. 00:24:47.728 --> 00:24:51.208 But like, we probably need to create a ticket for like coming back in 00:24:51.208 --> 00:24:54.388 and, you know, renaming a bunch of stuff or something like that. 00:24:54.718 --> 00:24:55.078 CJ: Yeah. 00:24:55.078 --> 00:24:58.078 I think in the past, what I've done in that experience is like if I, 00:24:58.078 --> 00:25:04.558 if I see something that I don't think meets our quality bar for 00:25:04.558 --> 00:25:06.140 polish , but it's almost there. 00:25:06.440 --> 00:25:11.300 Then I will still leave comments about the stuff that is like insanely nitpicky, but 00:25:11.300 --> 00:25:16.160 then in like the overall comment for the entire PR review, I'll say something like, 00:25:16.430 --> 00:25:22.020 left a few nit comments, but it's like, this is, you know, fine to, to move on. 00:25:22.090 --> 00:25:24.670 So yeah, I think that's pretty clear or, yeah. 00:25:25.810 --> 00:25:30.340 when, I guess like in GitHub when you're reviewing, you can also like approve or 00:25:30.340 --> 00:25:34.870 comment and so you could say like, Oh, I left a few comments and then just comment. 00:25:34.870 --> 00:25:39.340 And that is kind of you implicitly saying that you don't approve of like 00:25:39.580 --> 00:25:42.640 where it's at right now and that some things need to be addressed versus 00:25:42.640 --> 00:25:46.720 like, I left a few knit comments, but then you click the approved button and 00:25:46.720 --> 00:25:48.550 so like it's technically approved, but 00:25:48.760 --> 00:25:48.895 Colin: Right. 00:25:49.225 --> 00:25:52.765 And they can still push up some more commits and stuff after that. 00:25:53.665 --> 00:25:53.845 Yeah. 00:25:53.845 --> 00:25:54.505 That's interesting. 00:25:54.505 --> 00:25:56.155 Like just you pointing that out. 00:25:56.155 --> 00:25:58.675 There is no, technically there's no decline button. 00:25:58.825 --> 00:25:59.125 Right. 00:25:59.175 --> 00:25:59.495 It's. 00:26:00.050 --> 00:26:03.920 Request changes or approve or comment. 00:26:03.980 --> 00:26:07.760 And you know, the comment one and the request changes are kind of the same 00:26:08.210 --> 00:26:11.360 at the end of the day, but one's a little bit gentler than the other one. 00:26:11.740 --> 00:26:12.100 CJ: Mm. 00:26:13.450 --> 00:26:16.960 Colin: And I would say, I actually found, I'll put this in the link to, in the 00:26:16.960 --> 00:26:21.580 show notes, we found a, like, I, I've been having this problem of not knowing 00:26:21.580 --> 00:26:26.770 when someone commented on a PR or even to a review, and I finally figured out 00:26:26.920 --> 00:26:29.080 how to configure the GitHub settings. 00:26:29.112 --> 00:26:31.710 Shout out to Anthony at Orbit for like sharing this. 00:26:31.710 --> 00:26:33.570 There, I tweeted about it a few weeks ago 00:26:33.895 --> 00:26:34.245 CJ: Mm. 00:26:34.620 --> 00:26:37.020 Colin: I was like, I keep miss, like, I don't get an email when 00:26:37.020 --> 00:26:40.380 someone comments like, Why am I not, like all my settings are set. 00:26:41.535 --> 00:26:46.545 So now I'm getting notifications in Slack like directly to me when it's like 00:26:46.545 --> 00:26:51.705 someone mentions me in a PR or tags my team in a pr, things like that, which I 00:26:51.705 --> 00:26:56.175 found to be more useful cuz I was like blocking people without realizing it. 00:26:56.175 --> 00:26:58.275 Or I'd be like, Hey, why is no one reviewed my PR? 00:26:58.275 --> 00:27:00.855 And I go look at it and there's a bunch of comments on it already. 00:27:01.510 --> 00:27:02.145 CJ: Mm-hmm. 00:27:02.146 --> 00:27:02.147 . Mm. 00:27:02.340 --> 00:27:03.690 Colin: told me about those things. 00:27:03.719 --> 00:27:05.915 And that's really important when you have like the request 00:27:05.915 --> 00:27:08.225 changes, comment, or approve. 00:27:08.225 --> 00:27:11.675 It's just nice to know that like, hey, it's ready to, to be merged, 00:27:11.675 --> 00:27:12.986 or we need some changes there. 00:27:12.987 --> 00:27:16.197 GitHub has a pretty nice thing where it's like you can comment everything and then 00:27:16.197 --> 00:27:21.507 you get that like final say, the like, it's either good to go, looks good to me. 00:27:21.567 --> 00:27:24.807 You know, we've got all this language now around ShipIt and 00:27:24.807 --> 00:27:27.117 L G, TM and all these things. 00:27:28.122 --> 00:27:32.032 I think ultimately are you, are you in an emoji and animated gif 00:27:32.052 --> 00:27:34.062 in your code reviews person or not? 00:27:34.587 --> 00:27:35.697 CJ: Definitely emojis. 00:27:35.697 --> 00:27:39.180 I, not as much the animated gifs, but yeah, I think it. 00:27:39.247 --> 00:27:42.440 I'm trying to think back if we were super into it at previous companies. 00:27:42.830 --> 00:27:46.490 I think at App Academy we used animated gifs a lot in prs, 00:27:46.490 --> 00:27:48.650 but not at my VR or Stripe. 00:27:48.650 --> 00:27:52.859 So yeah, like but I, I do think they add a little bit of fun and 00:27:52.859 --> 00:27:55.229 flavor to the experience, so yeah. 00:27:55.469 --> 00:27:56.189 That's a good call. 00:27:56.519 --> 00:27:56.879 Colin: Yeah, 00:27:57.269 --> 00:28:01.319 I'll use like the eyeball reactions on certain, like comments and stuff 00:28:01.319 --> 00:28:05.099 to let them know that I'm looking at it or, or reading it if it's 00:28:05.099 --> 00:28:08.279 like not, and I don't even know if you get notifications for those. 00:28:08.864 --> 00:28:13.766 I will use the ship it Squirrel and the ship in the, in the final PR comment 00:28:13.766 --> 00:28:15.476 if I, if I think it's ready to go. 00:28:15.686 --> 00:28:20.576 CJ: Yeah, I would say that emojis, so emojis can also be used for a lot of 00:28:20.576 --> 00:28:23.996 good positive reinforcement, which I do think is another really important 00:28:24.001 --> 00:28:27.806 part of reviewing someone's PR is like going through and saying like, Wow, 00:28:27.806 --> 00:28:32.216 this is super nicely organized, or, I really like how you use this pattern. 00:28:32.221 --> 00:28:34.567 Or like yeah, like maybe they. 00:28:34.987 --> 00:28:38.797 Not necessarily something clever, but something that might like, do 00:28:38.797 --> 00:28:41.737 a performance optimization that was kind of like something that 00:28:41.737 --> 00:28:43.057 they went over and above to do. 00:28:43.297 --> 00:28:46.387 Then I'll drop like sunglasses face or like, Oh, this is cool. 00:28:46.387 --> 00:28:49.608 Or even just like those little things that that you're calling out that 00:28:49.613 --> 00:28:52.688 are, that are good can help, I guess. 00:28:53.198 --> 00:28:57.408 Yeah, they, they, they help soften the blow for any critical feedback that 00:28:57.408 --> 00:29:01.683 might come later, but also, You're giving kudos where kudos are due for 00:29:01.683 --> 00:29:03.603 someone, you know, doing great work. 00:29:03.603 --> 00:29:09.264 So that's a, yeah, I think that's also like an important part of review. 00:29:09.264 --> 00:29:13.596 So at, at orbit, do you have checklists, like, do you kind of like have a team 00:29:13.836 --> 00:29:17.106 organized checklist that's like, okay, make sure that you're looking for security 00:29:17.106 --> 00:29:21.606 vulnerabilities and is it tested and is there observability and is there logging 00:29:21.606 --> 00:29:24.156 and is there, does it need legal review or 00:29:24.191 --> 00:29:29.311 Colin: Yeah, we do have a checklist in Notion we have like a PR. 00:29:29.906 --> 00:29:32.336 Checklist especially, we have a feature that's coming out that 00:29:32.396 --> 00:29:37.316 like has a specific QA checklist that is in the PR template now. 00:29:37.316 --> 00:29:39.399 So like that's still more on the PR side. 00:29:39.399 --> 00:29:43.701 Because the person opening the PR needs to go through, I guess both the reviewer 00:29:43.701 --> 00:29:45.321 and the PR before it gets merged. 00:29:45.321 --> 00:29:48.831 All the check boxes need to be checked just just for sanity. 00:29:48.919 --> 00:29:52.069 Cuz some of them are more like regression type things. 00:29:53.614 --> 00:29:55.744 We hope to catch in tests, but we don't. 00:29:56.104 --> 00:29:58.318 There's just some strange things that could happen. 00:29:58.318 --> 00:30:02.442 But we do have a notion like for onboarding that people read through. 00:30:02.952 --> 00:30:06.882 I wouldn't say like, we have explicit checklists that, you know, has 00:30:06.882 --> 00:30:08.452 to get checked off in every pr. 00:30:08.462 --> 00:30:11.852 Especially if it's like, you know, again, a README may not affect security. 00:30:11.852 --> 00:30:13.602 So we're gonna skip that section. 00:30:13.602 --> 00:30:14.483 Do you guys have something. 00:30:14.483 --> 00:30:17.335 CJ: We, I think each team does it a little bit differently. 00:30:17.365 --> 00:30:21.505 Each team kind of like individually maintains their process and 00:30:21.835 --> 00:30:23.153 their own code ownership. 00:30:23.153 --> 00:30:28.227 And yeah, I mean, we do use code owners to like say which files are 00:30:28.227 --> 00:30:31.797 owned by which teams and so people can automatically get pulled in 00:30:31.797 --> 00:30:33.597 if there's changes to their stuff. 00:30:33.626 --> 00:30:36.472 . But yeah, like I, I've definitely seen some teams where it's like, 00:30:36.952 --> 00:30:40.042 there's a really explicit checklist and it's, you have to go through 00:30:40.042 --> 00:30:41.362 and make sure there's that. 00:30:41.367 --> 00:30:44.452 You think about security and then you think about tests and then you think 00:30:44.452 --> 00:30:47.572 about n plus one queries and then you think about caching and then you think 00:30:47.577 --> 00:30:49.594 about whatever, you know, like yeah. 00:30:49.594 --> 00:30:52.024 And like another great one is migrations. 00:30:52.054 --> 00:30:55.744 I've been bit a million times, This is more in like Django land, but 00:30:55.817 --> 00:30:58.247 yeah, you're trying to roll out a, my data migration that's gonna add. 00:30:59.177 --> 00:31:02.957 Maybe you're gonna add a new column that needs an index or something 00:31:02.957 --> 00:31:07.187 and like you gotta do all the different steps that in Rails it's 00:31:07.187 --> 00:31:09.197 less painful than it is in Django. 00:31:09.197 --> 00:31:13.157 But like sometimes in Django you'd have to do like several prs where 00:31:13.157 --> 00:31:16.877 they were coordinated, where like the first one does something and then the 00:31:16.877 --> 00:31:19.097 second one does something else and you have to like deploy one and then 00:31:19.097 --> 00:31:20.447 immediately deploy the other one. 00:31:20.447 --> 00:31:20.849 And yeah. 00:31:20.849 --> 00:31:23.879 So I think we kind of built a pretty strong culture around. 00:31:24.434 --> 00:31:27.212 Reviewing data migrations and yeah, I don't know. 00:31:27.212 --> 00:31:29.402 I think it's, it's, it's also tough to like make sure that you're 00:31:29.402 --> 00:31:32.462 hitting all of the right things that are required for a feature. 00:31:32.467 --> 00:31:35.075 Like you know, do we have the right metrics in place to measure the 00:31:35.075 --> 00:31:37.038 success of this thing as it goes out? 00:31:37.072 --> 00:31:37.562 Yeah. 00:31:38.462 --> 00:31:38.642 Colin: Yeah. 00:31:38.642 --> 00:31:43.292 I think other than that checklist, I'd say to kind of put a bow on this 00:31:43.292 --> 00:31:46.922 one, like the big thing is to remember that the, the person, the the person 00:31:46.922 --> 00:31:50.702 on the other end of the review is human and like they're gonna mix their 00:31:50.702 --> 00:31:52.202 identity with the code a little bit. 00:31:52.260 --> 00:31:56.300 . And so just keeping that in mind and, and having compassion and pa 00:31:56.360 --> 00:31:58.669 compassionate code reviews is important. 00:31:58.669 --> 00:32:04.781 And with that, have you used any of the tools on like, I think one of the things 00:32:04.781 --> 00:32:08.831 that we've kind of talked about here is like to, for both the PR reviewer and 00:32:08.891 --> 00:32:13.945 the person authoring it I've been seeing a lot more like tooling and processes 00:32:13.950 --> 00:32:17.350 for like, Prs that make it easier. 00:32:17.350 --> 00:32:20.217 And the one that comes to mind is graphite.dev. 00:32:21.177 --> 00:32:22.107 Have you seen this before? 00:32:22.892 --> 00:32:24.842 CJ: I have not seen graphite. 00:32:24.962 --> 00:32:30.272 We used a tool called, I think it was called Reviewable. 00:32:31.442 --> 00:32:31.682 Yeah. 00:32:31.682 --> 00:32:32.762 We used reviewable. 00:32:32.907 --> 00:32:37.017 Colin: The idea behind graphite, and I think this is similar to like fabricator 00:32:37.017 --> 00:32:40.960 at Facebook and some of these other tools is That like we ran into this issue when 00:32:40.960 --> 00:32:44.650 we were building some of our integrations, we would usually build them as one pr. 00:32:45.340 --> 00:32:48.880 They were the most insanely large PRS you've ever seen. 00:32:48.910 --> 00:32:52.690 If people just wouldn't wanna review it because they were like, I don't like, 00:32:52.690 --> 00:32:55.570 Unless you were on the integrations team, you didn't know how to review it. 00:32:56.340 --> 00:33:00.250 what we've moved to right now is just small prs. 00:33:00.280 --> 00:33:03.910 Each PR being kind of like what you just said, that they're like coordinated. 00:33:04.225 --> 00:33:05.185 This is PR one. 00:33:05.185 --> 00:33:07.615 Once that lands, we can then merge two and three. 00:33:07.885 --> 00:33:11.875 The problem with that is that you end up with dependencies on branches 00:33:12.385 --> 00:33:17.935 and prs and so graph I aims to solve that and it essentially has these 00:33:17.935 --> 00:33:23.335 like stacks of prs that all end up rolling up into like one major change. 00:33:23.400 --> 00:33:24.570 I've been playing with it. 00:33:24.570 --> 00:33:27.300 I'm still not sure how I feel about it. 00:33:27.540 --> 00:33:30.540 It feels like something like your whole team kind of has to use if 00:33:30.540 --> 00:33:31.920 you're gonna really dedicate to. 00:33:32.865 --> 00:33:36.465 Or maybe you can use it for your own prs, but it has a really cool 00:33:36.465 --> 00:33:40.395 integration with GitHub, whereas each PR gets added and merged. 00:33:40.815 --> 00:33:43.995 Like there's a little running list that gets added to the GitHub 00:33:43.995 --> 00:33:46.335 PR of like, this is one of four. 00:33:46.575 --> 00:33:48.285 This one's been merged, this one's been merged. 00:33:48.290 --> 00:33:51.675 And then it auto rebates all of the upstream onto it. 00:33:51.675 --> 00:33:52.095 And this is 00:33:52.230 --> 00:33:52.440 CJ: Ooh. 00:33:53.055 --> 00:33:57.165 Colin: doing all the get like get does not translate well to audio at all. 00:33:57.870 --> 00:33:58.170 CJ: Yeah, 00:33:58.665 --> 00:34:00.915 Colin: when you start talking about merges and branches. 00:34:01.455 --> 00:34:05.258 And Rebasing, but definitely check out graphite.dev if you have this issue. 00:34:05.468 --> 00:34:09.936 There are other tools that are you know, available if you, if you search for like, 00:34:09.966 --> 00:34:12.587 merging and, and code review strategies. 00:34:12.827 --> 00:34:16.037 I'm still trying to find that, that thing that's like lightweight on 00:34:16.042 --> 00:34:20.443 the team where you actually can have branches and prs that are dependent 00:34:20.443 --> 00:34:25.603 on previous ones, but not block you as a developer from keeping, you know, 00:34:25.603 --> 00:34:27.073 development, you know, working on it. 00:34:27.748 --> 00:34:31.888 You know, doing it where you have to constantly rebase upstream or you 00:34:31.888 --> 00:34:35.218 know, if the thing you need is not in the branch that you're in right now, 00:34:35.218 --> 00:34:36.628 then that, that becomes an issue. 00:34:36.628 --> 00:34:41.855 And so little bit off topic for code reviews, but Graphite's own website claims 00:34:41.860 --> 00:34:45.935 that the biggest reason to do this is that it makes code review even easier 00:34:45.935 --> 00:34:48.485 because maybe the migration is in a PR. 00:34:49.220 --> 00:34:53.120 The model and the controller are and there on prs, and then you 00:34:53.120 --> 00:34:54.620 have the business logic in its own. 00:34:54.890 --> 00:34:58.910 And that way you can also start getting things into main faster 00:34:59.570 --> 00:35:01.400 feature flag things, right? 00:35:01.430 --> 00:35:04.430 You're starting to develop around feature flag development instead 00:35:04.430 --> 00:35:08.990 of like, you know, having that big launch day where you're afraid to 00:35:08.990 --> 00:35:10.280 see if everything's gonna land. 00:35:11.745 --> 00:35:11.985 CJ: Mm-hmm. 00:35:12.065 --> 00:35:12.545 . Mm-hmm. 00:35:13.940 --> 00:35:16.880 . Yeah, I, I'm trying to remember the features of reviewable 00:35:16.880 --> 00:35:18.320 that were killer features. 00:35:18.320 --> 00:35:22.917 I think many of the ones that we were using are now rolled into just 00:35:22.917 --> 00:35:25.617 like the default GitHub interface. 00:35:25.617 --> 00:35:28.757 So yeah, I'm sure there's features, new features that we, I haven't 00:35:28.757 --> 00:35:31.924 seen added, but yeah, tools can definitely help your flow. 00:35:32.134 --> 00:35:35.524 I think we also talked about having like an automated system for. 00:35:36.124 --> 00:35:41.134 Adding comments to the PR that like you kind of have a bot do a first pass 00:35:41.134 --> 00:35:45.244 of a review and make comments about things like style or, Hey, you know, 00:35:45.244 --> 00:35:48.454 at this company we put parentheses when we make our method calls or whatever. 00:35:48.454 --> 00:35:50.884 Like you can kind of apply those automatically, but. 00:35:50.974 --> 00:35:53.434 Colin: Let the, let the bot do the nitpicky stuff so you 00:35:53.674 --> 00:35:53.854 CJ: Yeah. 00:35:54.604 --> 00:35:55.204 . Exactly. 00:35:55.294 --> 00:35:55.594 Yep. 00:35:56.614 --> 00:35:57.094 Cool. 00:35:57.184 --> 00:35:59.023 Well let's wrap it up. 00:35:59.023 --> 00:36:05.293 We hope you enjoyed this deep dive into how to review PRS and doing code reviews. 00:36:05.773 --> 00:36:09.433 Next time we're gonna start talking about some developer content creation 00:36:09.438 --> 00:36:12.343 and a lot of the workflows that we use for dev content creation. 00:36:12.758 --> 00:36:15.818 Colin: As always, you can head over to build and learn.dev to check out all the 00:36:15.818 --> 00:36:17.588 links and resources in the show notes. 00:36:17.618 --> 00:36:18.998 That's all for this episode. 00:36:18.998 --> 00:36:20.108 We'll see you next time. 00:36:20.497 --> 00:36:21.187 CJ: Bye friends.