Colin: Welcome to Build and Learn.
My name is Colin.
CJ: And I'm CJ, and today we're talking about reviewing poll requests.
If you recall, a couple episodes ago we talked about authoring prs, but today
we're talking about the other side of the puzzle, which is reviewing them.
Before we get into that, we wanted to just talk about some upcoming conferences
and I'm, I'm really jealous because there is this really epic conference that's
coming up that I really wanted to go to.
And Colin is , Colin is headed there.
That is Rails SaaS.
So yeah, like what is Rails SaaS and when is it happening?
Colin: Yeah, so let me just pull it up here.
So it's October six and seven.
I think this is gonna be a little interesting to see
when this episode comes out.
Rails SaaS may have already happened when this episode comes out.
But we've got a lot of really cool speakers that are talking about the
intersection of rails and business.
So you've got some folks who are building SaaS businesses on top of rails, like
literally, like things like GoRails.
Hatch box hammer stone.dev, or they're building and releasing gems and things
as, as a, you know, as a business.
And then I imagine that there's gonna be a little bit of you know,
kind of like the stuff that you see with that Saastr conference.
There's a lot of just, you know, the business side of
rails, which I'm excited to.
CJ: Cool.
Yeah.
So SaaS, if you're not familiar, is software as a service.
This is typically like a subscription based thing.
We all know these and love them.
If.
own one and like loathe them if you have a bunch of subscription payments that
are hitting your wallet every month.
But yeah, so I, yeah, SaaS in general is something obviously
that I'm like pretty excited about.
And yeah, I've been working on some content for Stripe teaching people how
to collect recurring payments and how to.
Build a SaaS basically with Rails.
And that's why I'm like super bummed.
I'm gonna get, I'm gonna miss this, but we'll be on a family vacation that
we planned a super long time ago, even before the conference was announced.
So yeah, but I'll be missing all, all my all my homies that are gonna
be in, it's in, is it in Hollywood or it's just in LA somewhere.
Colin: It's in Hollywood and it's it's organized by Andrew
Culver from Bullet Train.
So they have like a, they kind of bill it as the Ruby on Rails SaaS template.
CJ: Hmm.
Colin: so they give you kind of an out of the box framework for building
SaaS companies on top of Rails.
So very cool to see like a thematic conference like this instead of just
focused on, you know, the language and the tooling and the processes.
It's gonna be talking about like, how do you use this to be super
productive in building a business?
Right.
Cause building and building something that people want and are willing
to pull out their credit card.
And, you know I guess Stripe would be very relevant here because of how
many subscriptions and, and things are built on top of Stripe and Rails.
CJ: Yeah, and I think, yeah, in Andrew's bullet train part of the I
think it's like the pro version or something you can install like a.
A few Stripe tools that will set up routes for you and a
few other a few other things.
I haven't actually bought the paid version of Bullet Train to test it out, but it's
my understanding that you get a lot of features outta the box with that sort of
like plug into the bullet train starter.
So
Colin: Cool.
And what other conferences are coming up that you are able to make?
CJ: Yeah, so I I submitted to speak at Ruby Comp this year.
I put in two talks.
I have not heard back yet.
We'll see by, definitely by the time this episode airs you, I will know
whether or not those were accepted . But yeah, Ruby Comp is in end of November.
It's in Houston, Texas.
Excited about that.
Excited to see all my Ruby friends.
Are you going to Ruby Conf this?
Colin: I was not planning on it.
I have not really looked at November yet, but.
CJ: Okay.
Yeah.
And then another one that's happening in November is Chirp, which is
the Twitter Developer Conference.
I think this is the first time they're having it in many, many years.
So this is gonna be in San Francisco.
It's a one day conference.
So yeah, that
should be, it should be pretty fun.
Colin: That one.
I'm definitely gonna make it down for.
I, they claimed it was the first chirp in 10 years, but I did see that there was
like a conference in 2015 that they held.
This one.
I'm really excited to kind of see how and what they've learned from, because
Twitter has a history of kind of letting down developers in the past, and so I
think they've realized like they've got a repair their relationship with developers
and we'll see how that goes with Chirp and I'm hoping for a lot of new API features.
We are using the Twitter API more and more at Orbit, and there's some
stuff that's like very lacking in the API that are obvious things, so I'm
excited to see that they're reinvesting in in that relationship and the api.
CJ: Yeah, so they shipped in API V two recently.
And as part of that, they have like a whole new OAuth 2.
Flow.
So yeah, I've been playing around with the new APIs quite a bit and yeah,
there's a few things that I would love to see, like way better webhook
support or webhook support at all.
There's like no way to get the email address of the person.
There's like, yeah, a bunch of like little things where I'm like, Oh
gosh.
Like, Huh,
Colin: that we should, we
CJ: I know.
Yeah.
We could dig,
we could dig into.
Colin: API audit.
CJ: Yeah I, I've been using it a lot lately too, just for like fun little side
project things and yeah, I think they've definitely turned a corner in terms
of listening to the developer audience and building, starting to build towards
like what the, what the audience needs.
So yeah, excited to
go check it out and see
how things go.
Colin: to do it.
A little audit of, of various APIs.
Especially like for us, we, we added a feature that we're working
on an orbit, a little preview here of like being able to send dms.
CJ: Ooh, Nice.
Colin: us permission to everything in order to send a dm.
CJ: I bet.
Yeah.
Colin: was like, Why is this not just one?
Like scope.
CJ: Yeah.
Colin: have to have, I could do anything I want on your Twitter account, which,
you know, we don't, we only send dms and there's only code to send the dms.
So it's not like we can accidentally do anything on your account.
But it's, it is that scary oauth screen of like, you're allowing Orbit
to have access to all this stuff.
And so
CJ: Yeah, I think, yeah, I, I would love to do episodes about that, just
like look at an API and do like a breakdown of the DX and so yeah,
that would be, that would be fun.
I think.
Yeah, there's probably a bunch of APIs too that we're both using just for fun.
Like for fun and profit, right?
Like on the side , like YouTube api, Twitter api Orbit API or Stripe api.
So yeah, it'd be, it'd be cool to go through All that.
Colin: So I guess in building those APIs, you're gonna have a whole lot
of PRs in your team, and hopefully there's small reviewable prs like we
talked about on build and learn.dev/six, if you wanna check out that episode.
But today we're gonna dive into that other side of the fence
where you've submitted your pr.
And you're waiting for someone to review it, that person's gonna pop
into GitHub or GitLab or Bitbucket.
And now you have this little bit of science, a little bit of art,
of reviewing someone else's code and giving constructive feedback.
Making sure that you kind of match the, the rest of the style of
the code base, things like that, especially for newer developers.
So yeah, we can kind of dig in.
We've.
A link to a pretty thorough tip like list of tips from thought Bot,
but that we can kind of review.
But I think it'd be great to kind of hear like what you first look for when you,
you know, once you get that notification that it's time to review something.
CJ: Yeah.
So first and foremost, something that I think is important is responding quickly.
Like trying to like actually.
Look at, if someone opens a pr, try to unblock them and respond to
their pr because the longer that you wait, the longer that it's gonna
take them to like ship their thing.
So yeah, jumping in quick and giving them some feedback or at least telling
them like, Hey, I'm really swamped.
This is gonna take me 24 hours or something to get back to you.
Yeah, so that's kind of just like a, a preface, preface thing.
But another thing that I tend to look for is any instructions
in the description of the pr.
Like, do they have pointers for what they're looking for?
Do they have requests for reviewing specific files?
And, you know, oftentimes the description will say something like, Oh, a lot
of this was auto generated, but can you please look at file X to you?
You know, make sure that there's a really good thorough review of security
on, on this like section of whatever.
So yeah, looking at the description and yeah, being timely in your reply, I guess
is like the first thing I would say.
Colin: Do you guys have like a how to test section of prs, like in your template?
CJ: We do.
Yeah.
So we, it's actually not how to test, it's how it was tested.
So we ask people Yeah.
Like to, you know, outline exactly how you tested it, and that could be,
I wrote a bunch of automated tests, or if it's a change to like a docs
page, it could be like, here are the screenshots of the changes that
I made, and also here's a link to.
The staging server, where the docs are or whatever.
So
Colin: Cause along the lines of not blocking them, like some of our
PRs require, like as a tester, like we know that the other person who
wrote it has already gone through those steps, like you mentioned.
But I do like to like then run them on my machine, Right.
Or run them on testing, like a staging or integration server.
But for some of those, you know, hopefully they're small reviewable
prs, but some of 'em you have to like do a little bit of world building or.
A script that creates all of the, the scenario that, that
you need to test this thing in.
And sometimes there's like a bug or something that's hard to replicate.
And so relying on tests is sometimes all you can do.
But you know, for us, we do like to, especially with integrations, we
want to test them on another machine so that it's like, hey, like some
environment variable didn't make it into the encrypted credentials.
Something like just about their machine or their setup is just not quite the same.
With integrations, we end up using things like ngrok and relying on web
hooks and stuff like that a lot too.
So like making sure that it's like, Hey, yeah, I caught a web hook
but it didn't hit the right path on my machine for some reason.
Or just like some sort of environmental kind of ruling
out environmental differences.
which for some companies, if you're running in like Docker or
something, those may not even be.
We run pretty bare rails on, you know, Mac, so,
CJ: Got it.
Yeah.
I was gonna ask, so our, our development environment is actually like external
boxes that live inside of aws.
And then also once we push a pr it will spin up staging
environments just with that PR so that you can link to it from the.
From the PR itself.
And the nice thing about that is that the environment inside of
AWS when you're building and when you're in staging and when you're in
production is all the same, right?
Like, it's gonna be on the same boxes, the same tools, the same like everything.
So that is like, I don't know, it's, it's, I would say it's a nice experience.
But it's also like super challenging.
. There's a lot of like DevOps overhead stuff to get that set up on a team.
So
Colin: review apps on Heroku for that.
So we get a little bit of that out of the box without having to have a
whole DevOps team build that for us.
So we do that when it's necessary.
Some changes, like again, docs changes.
We don't need to spin up a review app for that,
CJ: Mm.
Colin: like read me updates and stuff like that.
Those are fine.
We can review them for accuracy and stuff like that.
But yeah, I think that's a, that's a pretty good process to get.
CJ: So going, going back to timeliness, I assume that your team is so globally
distributed, that sometimes your reviewer will be, you know, 12 hours
difference in terms of time zones.
Is that the case or are you mostly kind of like working with people
that are closer to your time zone?
Colin: Yeah, this is something that's kind of interesting for us.
We do tend to have, like when I, my old team, I was the only person in the US and.
It would be this thing where like by the end of my day I try to review any open
prs for Europe so that they're waking up to a reviewed pr, that then they can, you
know, either make changes or merge and then on their end, you know, I, on my end
I'm trying to get mine, my code done so that when they also wake up, they have a
PR for me that they can take a look at.
And sometimes there might be this back and forth conversation around, Maybe it's a
draft PR with questions because it might be a newer area of the code base for me.
And so I do have to kind of think about that.
It really forces me that time box might work too.
It's like if I don't get to the thing I'm hoping to today, then I'm not
gonna have the PR open and ready for review by the beginning of their day,
which means now we're gonna have a whole nother 24 hour cycle of that.
CJ: right?
I is there Is there like a, an hour or two where you have overlap
and you can kind of like talk live
and.
Colin: Yeah.
We've got.
In my morning.
I have someone in, in Israel that I, we actually have a lot
of overlap during the day because she works really late at night.
And so she kind of has a different setup of her day.
And so we do get that and like, we just restructured our team, so we do have more
people in different, like I have more US folks on my team now, so you know,
that's gonna change a little bit as well.
So, you know, once we do that, It's nice to have those comments.
I've noticed that, especially because it's a little bit async, someone might
open a pr and then I guess this is on the more of the creating the PR side
of it, but they'll preempt the review by going into the, like the filed diff
CJ: Mm-hmm.
Colin: add their own comments.
So like calling out like, Hey, could you specifically look at this thing that I
either had a struggle with or like, I think this is the best way to do this.
You know, can you just like do a gut check?
. You know, if there's any sort of things that need to be specifically tested
from a security or like web hook and like ingest type of thing, then they
might call that out Especially with integrations, it's like you have to
do this world building again of, of having the development integration.
So like, you know, maybe it's developer Twitter account.
Connected to the developer app versus the staging app.
Versus the production app.
So there's just a lot of variables at play.
CJ: Mm-hmm.
. So when you're, when you're looking at security stuff, In the back of your mind,
do you have or Yeah, I guess like in the back of your mind, do you have certain
things that you look for when it comes to security or just like generally thinking?
Colin: so if it's general web application security things, you know, we trust that
the team, you know, can look at those things and if, if it's not an area of
expertise, we can tag in somebody else.
If it's like something related to like SOC2 or something like that,
then it might be someone who's.
Specifically like security engineered type of person that would look at that stuff.
You know, and that's not just at orbit.
That would be like at past companies where you have the luxury of having
somebody who can specifically do that.
And then you also hopefully have some of these like scanners and things too
that are running on your, on your.
PR and CI that are looking for common vulnerabilities and things
like that in the code base as well, like strong prams and things
like that come to mind for rails.
You know, CORS, stuff like that, that that'll pop up.
CJ: Yeah, cross site scripting, vulnerable, like I'm trying to
remember the name of it, but yeah, there's a bunch of tools where you
can just say like, point this at my app and like try to run all the
vulnerability scanning things on it.
Yeah.
Cool.
Colin: like one of one of them was called like Hound, I think.
CJ: Hound interesting.
Yeah, I think like looking, the one big one that comes to mind is SQL injection
attacks, which really, if people are not parameterizing or like not sanitizing user
input before they're passing them down to sql, then you kind of get in trouble.
But yeah, I feel like the rail strong params pattern has, you know, if
you kind of like stick to the, the normal patterns, you're usually okay.
But I have seen a couple times where maybe it's like a search endpoint and
you're writing a custom where clause that has a bunch of likes and whatever.
If you're not passing like the right parameterization, then it's easier to get
User input and pass that right down to SQL
Yeah, so, Okay.
Super cool.
What else we got here, I guess?
Colin: What about the content of the code itself?
So like, let's say now it's time to jump into the code.
What sorts of things, you know, when we've talked about this in past episodes,
but like programming is so opinionated, so how do you approach this when you
know someone spent a lot of their time.
On the code.
And I think as much as we like to try to disconnect our ourselves from
it, it's something that we've spent a lot of time on and, you know, by the
time it comes time to ask for review, hopefully it's quote unquote done.
So how do you kind of approach those things that might be trade offs or things
that you might, that might stick out to.
CJ: I think going into every PR was an open mind and trying to come at it from
a, like a perspective where you're.
About what the original author intended can be helpful.
And then also after you really understand what they were going for
then maybe start to ask questions.
Like ask leading questions instead of like criticizing, Right?
So kind of like, Oh, I see that you were doing it this way.
That's really interesting.
Did you think about this approach?
Or You know was there a reason why you know, you're taking this in, taking this
as an argument instead of instantiating it inside of the class or whatever.
Like there's an opportunity to talk about design patterns.
There's also opportunity to talk about, like learning the code base
itself as a, as a result of the pr.
So, yeah, I guess maybe to answer your question, I think coming at it from a
perspective of like, what can I learn from this code that I'm about to.
Colin: Definitely.
Yeah.
The, the asking questions is interesting to me cuz I, I do think it's better
than like, making demands or like saying like, change this to this cuz
you don't necessarily, like you said, you don't know where, why they had to
make some of the choices that they made.
The asking questions thing I find is the art piece because it's sometimes
when I'm writing a question on a pr I'm trying not to be pa like patronizing.
It's like how do we get to the topic of this?
Like sometimes there's a more direct way of saying things, but you do wanna
avoid like judgment or assumptions.
And when I've reviewed like a lot of prs in a row, it starts
to feel a little bit weird.
And this might just be in my head, but like, it's like rather than me just
like calling it out, it's like, why don't, what do you think about this?
Or have you tried this?
And things like that.
Because I'm.
In some ways you wanna, you're not trying to lead them to the right answer, I guess.
It's like, it's not the goal.
The goal is just like to figure out, like, did they consider something else?
Was there an issue with like, you know, is there a reason why this
doesn't match the, the code that's over here that does the same thing or.
Those kinds of things.
And I would say like, if that's the case, like always ask for clarification.
Like you could just ask a question.
You don't have to be like, I need to get this review done.
Maybe it's like, Yeah, I'm gonna need some more info to, to do this.
But how do you feel about that?
Like in terms of like when you've written something, are questions helpful or do
they, do they feel like that sometimes to.
CJ: So I think they are helpful in terms of like the ego part of
receiving a code review, right?
If someone is like, Oh, this is wrong.
You should change this, it.
Definitely chips away at like your identity that is tied inextricably from
the code that you just published on GitHub and asked for a kind and gentle
review and someone jumps in there with a bunch of hyperbole and never do
this, and why didn't you just do that?
And you know, this is overcomplicated or whatever.
Like those kinds of, those kinds of comments can really like beat you down.
I will say that I have worked.
Several people who do their prs like that, and they're, they're like insanely
direct, very hyperbolic and like really, really explicit and d and I don't
know, very blunt about their feedback.
And it was extremely painful, but, I think I, I also like got better at
like, building up a shield of like, Okay, I'm publishing this, it's fine.
Like I've gotta get , like torn up or whatever.
But like I know that at the end of the day, like I, my hope is that that
criticism is coming in and that I can receive it and become a better developer.
So like, I always had to like try to take that perspective as the author
and because I know that was so painful.
and also it required a lot of like building up this thick skin.
I always tried to like approach PRS that I am reviewing with that, that own like my
own personal experience and try to like be really empathetic and really humble
and really like, Okay well let's be.
Let's use this as a teaching moment instead of like a, just
make sure the code is fine.
Like you have an opportunity to make other developers on your team know and
understand the things that you know, and you can do that through the PR
instead of just kind of like, and you can do it in like a tactful way instead
of just being kind of like, Yeah.
Attacking.
Colin: that you had to go through that to then find the compassion, right?
Like you would've probably had that compassion for your reviews anyway,
but like that experience of having to build that shield, like did that
also cause you to second guess?
Like that it's ready to be puff.
Submitted or like, I imagine like you're gonna check triple check everything when
you are like, Oh my God, I don't know what negative feedback I'm gonna get.
Cuz
to me like, that sounds like an awful reviewer.
Even if it's coming from a good place, like, you know, maybe they
just were rushed and they shouldn't have reviewed it like right before
they left the office or something.
But that's, that's a rough experience, especially for newer developers who
may not know that it's like, this isn't about not to take it person.
But it is gonna be personal.
Like there's no way to
CJ: Yeah.
Yeah.
The, a couple things came out of it.
One is when I would publish a pr, I would read all of the code
myself and review it myself.
For myself, like, like I would say, yeah, before you push the button that
says, you know, submit pr, whatever.
You can see the diff go through that diff and Polish.
Take your last like chance to polish it up and anything that I started to build
up this sense of like, okay, I think.
This is the area of code that I'm going to receive the most criticism about.
And so then like I would go back and maybe make a little,
like clean it up a little bit.
And when I didn't do that, I always got comments on those parts where I was
like, Okay, like I think maybe this area might, you know, need some improvement.
Colin: Like negative reinforcement though.
CJ: yes, , it was, it absolutely
was.
No
Colin: it probably made you a much better developer through fire.
Right?
It's not not through yeah.
Ooh, that's rough.
CJ: yeah,
Colin: yeah, I, I come from a background of not always having,
Developers to review my code, and so I would have to do that anyway, right?
I'm like, I'm literally doing PRS to myself and then going
and looking at the diff and like reviewing it like the next day.
So like with fresh eyes.
That was like when I was the only developer at a company.
You know, I wonder if that's even a service.
It'd be amazing to like, have like a contract code reviewer
CJ: Mm-hmm.
Colin: Can you just review my PRS as an external, you know, person?
But I always had to develop that.
And because of that, I never really got a lot of those, like those negative reviews.
But because of that too, I never developed a really strong muscle for
like what to look for in other people's.
And I've had to develop that.
You know, at orbit we have plenty of developers to review each other's code.
And I've learned a lot on both sides from reviewing other people's code.
I've learned a lot of things where I'm like, I didn't know Ruby could do that.
know that Rails could do that.
But then on the flip side, just seeing like what comments people do.
You know, point out you know, especially if someone knows the
code base in a different area better and they're like, Hey, we already
have something like this over here.
you considered reusing this?
Things like that.
So and I think, you know, some of the, the tips from thought bot when
you're reviewing code, where like communicate things that you feel
strongly about and those that you don't.
So like maybe there's something else, like, we really shouldn't merge.
Part as is, or like, Hey, over here maybe we, we, we should clean this up.
But maybe that's like in a future pr, like, let's not block this one
and ship because it's not gonna cause any downstream problems.
But like, we probably need to create a ticket for like coming back in
and, you know, renaming a bunch of stuff or something like that.
CJ: Yeah.
I think in the past, what I've done in that experience is like if I,
if I see something that I don't think meets our quality bar for
polish , but it's almost there.
Then I will still leave comments about the stuff that is like insanely nitpicky, but
then in like the overall comment for the entire PR review, I'll say something like,
left a few nit comments, but it's like, this is, you know, fine to, to move on.
So yeah, I think that's pretty clear or, yeah.
when, I guess like in GitHub when you're reviewing, you can also like approve or
comment and so you could say like, Oh, I left a few comments and then just comment.
And that is kind of you implicitly saying that you don't approve of like
where it's at right now and that some things need to be addressed versus
like, I left a few knit comments, but then you click the approved button and
so like it's technically approved, but
Colin: Right.
And they can still push up some more commits and stuff after that.
Yeah.
That's interesting.
Like just you pointing that out.
There is no, technically there's no decline button.
Right.
It's.
Request changes or approve or comment.
And you know, the comment one and the request changes are kind of the same
at the end of the day, but one's a little bit gentler than the other one.
CJ: Mm.
Colin: And I would say, I actually found, I'll put this in the link to, in the
show notes, we found a, like, I, I've been having this problem of not knowing
when someone commented on a PR or even to a review, and I finally figured out
how to configure the GitHub settings.
Shout out to Anthony at Orbit for like sharing this.
There, I tweeted about it a few weeks ago
CJ: Mm.
Colin: I was like, I keep miss, like, I don't get an email when
someone comments like, Why am I not, like all my settings are set.
So now I'm getting notifications in Slack like directly to me when it's like
someone mentions me in a PR or tags my team in a pr, things like that, which I
found to be more useful cuz I was like blocking people without realizing it.
Or I'd be like, Hey, why is no one reviewed my PR?
And I go look at it and there's a bunch of comments on it already.
CJ: Mm-hmm.
. Mm.
Colin: told me about those things.
And that's really important when you have like the request
changes, comment, or approve.
It's just nice to know that like, hey, it's ready to, to be merged,
or we need some changes there.
GitHub has a pretty nice thing where it's like you can comment everything and then
you get that like final say, the like, it's either good to go, looks good to me.
You know, we've got all this language now around ShipIt and
L G, TM and all these things.
I think ultimately are you, are you in an emoji and animated gif
in your code reviews person or not?
CJ: Definitely emojis.
I, not as much the animated gifs, but yeah, I think it.
I'm trying to think back if we were super into it at previous companies.
I think at App Academy we used animated gifs a lot in prs,
but not at my VR or Stripe.
So yeah, like but I, I do think they add a little bit of fun and
flavor to the experience, so yeah.
That's a good call.
Colin: Yeah,
I'll use like the eyeball reactions on certain, like comments and stuff
to let them know that I'm looking at it or, or reading it if it's
like not, and I don't even know if you get notifications for those.
I will use the ship it Squirrel and the ship in the, in the final PR comment
if I, if I think it's ready to go.
CJ: Yeah, I would say that emojis, so emojis can also be used for a lot of
good positive reinforcement, which I do think is another really important
part of reviewing someone's PR is like going through and saying like, Wow,
this is super nicely organized, or, I really like how you use this pattern.
Or like yeah, like maybe they.
Not necessarily something clever, but something that might like, do
a performance optimization that was kind of like something that
they went over and above to do.
Then I'll drop like sunglasses face or like, Oh, this is cool.
Or even just like those little things that that you're calling out that
are, that are good can help, I guess.
Yeah, they, they, they help soften the blow for any critical feedback that
might come later, but also, You're giving kudos where kudos are due for
someone, you know, doing great work.
So that's a, yeah, I think that's also like an important part of review.
So at, at orbit, do you have checklists, like, do you kind of like have a team
organized checklist that's like, okay, make sure that you're looking for security
vulnerabilities and is it tested and is there observability and is there logging
and is there, does it need legal review or
Colin: Yeah, we do have a checklist in Notion we have like a PR.
Checklist especially, we have a feature that's coming out that
like has a specific QA checklist that is in the PR template now.
So like that's still more on the PR side.
Because the person opening the PR needs to go through, I guess both the reviewer
and the PR before it gets merged.
All the check boxes need to be checked just just for sanity.
Cuz some of them are more like regression type things.
We hope to catch in tests, but we don't.
There's just some strange things that could happen.
But we do have a notion like for onboarding that people read through.
I wouldn't say like, we have explicit checklists that, you know, has
to get checked off in every pr.
Especially if it's like, you know, again, a README may not affect security.
So we're gonna skip that section.
Do you guys have something.
CJ: We, I think each team does it a little bit differently.
Each team kind of like individually maintains their process and
their own code ownership.
And yeah, I mean, we do use code owners to like say which files are
owned by which teams and so people can automatically get pulled in
if there's changes to their stuff.
. But yeah, like I, I've definitely seen some teams where it's like,
there's a really explicit checklist and it's, you have to go through
and make sure there's that.
You think about security and then you think about tests and then you think
about n plus one queries and then you think about caching and then you think
about whatever, you know, like yeah.
And like another great one is migrations.
I've been bit a million times, This is more in like Django land, but
yeah, you're trying to roll out a, my data migration that's gonna add.
Maybe you're gonna add a new column that needs an index or something
and like you gotta do all the different steps that in Rails it's
less painful than it is in Django.
But like sometimes in Django you'd have to do like several prs where
they were coordinated, where like the first one does something and then the
second one does something else and you have to like deploy one and then
immediately deploy the other one.
And yeah.
So I think we kind of built a pretty strong culture around.
Reviewing data migrations and yeah, I don't know.
I think it's, it's, it's also tough to like make sure that you're
hitting all of the right things that are required for a feature.
Like you know, do we have the right metrics in place to measure the
success of this thing as it goes out?
Yeah.
Colin: Yeah.
I think other than that checklist, I'd say to kind of put a bow on this
one, like the big thing is to remember that the, the person, the the person
on the other end of the review is human and like they're gonna mix their
identity with the code a little bit.
. And so just keeping that in mind and, and having compassion and pa
compassionate code reviews is important.
And with that, have you used any of the tools on like, I think one of the things
that we've kind of talked about here is like to, for both the PR reviewer and
the person authoring it I've been seeing a lot more like tooling and processes
for like, Prs that make it easier.
And the one that comes to mind is graphite.dev.
Have you seen this before?
CJ: I have not seen graphite.
We used a tool called, I think it was called Reviewable.
Yeah.
We used reviewable.
Colin: The idea behind graphite, and I think this is similar to like fabricator
at Facebook and some of these other tools is That like we ran into this issue when
we were building some of our integrations, we would usually build them as one pr.
They were the most insanely large PRS you've ever seen.
If people just wouldn't wanna review it because they were like, I don't like,
Unless you were on the integrations team, you didn't know how to review it.
what we've moved to right now is just small prs.
Each PR being kind of like what you just said, that they're like coordinated.
This is PR one.
Once that lands, we can then merge two and three.
The problem with that is that you end up with dependencies on branches
and prs and so graph I aims to solve that and it essentially has these
like stacks of prs that all end up rolling up into like one major change.
I've been playing with it.
I'm still not sure how I feel about it.
It feels like something like your whole team kind of has to use if
you're gonna really dedicate to.
Or maybe you can use it for your own prs, but it has a really cool
integration with GitHub, whereas each PR gets added and merged.
Like there's a little running list that gets added to the GitHub
PR of like, this is one of four.
This one's been merged, this one's been merged.
And then it auto rebates all of the upstream onto it.
And this is
CJ: Ooh.
Colin: doing all the get like get does not translate well to audio at all.
CJ: Yeah,
Colin: when you start talking about merges and branches.
And Rebasing, but definitely check out graphite.dev if you have this issue.
There are other tools that are you know, available if you, if you search for like,
merging and, and code review strategies.
I'm still trying to find that, that thing that's like lightweight on
the team where you actually can have branches and prs that are dependent
on previous ones, but not block you as a developer from keeping, you know,
development, you know, working on it.
You know, doing it where you have to constantly rebase upstream or you
know, if the thing you need is not in the branch that you're in right now,
then that, that becomes an issue.
And so little bit off topic for code reviews, but Graphite's own website claims
that the biggest reason to do this is that it makes code review even easier
because maybe the migration is in a PR.
The model and the controller are and there on prs, and then you
have the business logic in its own.
And that way you can also start getting things into main faster
feature flag things, right?
You're starting to develop around feature flag development instead
of like, you know, having that big launch day where you're afraid to
see if everything's gonna land.
CJ: Mm-hmm.
. Mm-hmm.
. Yeah, I, I'm trying to remember the features of reviewable
that were killer features.
I think many of the ones that we were using are now rolled into just
like the default GitHub interface.
So yeah, I'm sure there's features, new features that we, I haven't
seen added, but yeah, tools can definitely help your flow.
I think we also talked about having like an automated system for.
Adding comments to the PR that like you kind of have a bot do a first pass
of a review and make comments about things like style or, Hey, you know,
at this company we put parentheses when we make our method calls or whatever.
Like you can kind of apply those automatically, but.
Colin: Let the, let the bot do the nitpicky stuff so you
CJ: Yeah.
. Exactly.
Yep.
Cool.
Well let's wrap it up.
We hope you enjoyed this deep dive into how to review PRS and doing code reviews.
Next time we're gonna start talking about some developer content creation
and a lot of the workflows that we use for dev content creation.
Colin: As always, you can head over to build and learn.dev to check out all the
links and resources in the show notes.
That's all for this episode.
We'll see you next time.
CJ: Bye friends.