A guide to Pull Requests

Authoring and reviewing pull requests with quality and efficiency.

PublishedFebruary 22, 2022

•4 min read•View as Markdown

Tiger Abrodi

Just a guy who loves to write code and watch anime.

Comments (2)

Join the discussion

Devdatta Khoche4y ago

Well written! I loved the way you explained it, really really well for beginners to understand. Keep it up, mate !! ✌️✌️✌️

Curtis Einsmann4y ago

Thanks for the shout! Nice article. Happy to see more code review content out there in the world.

I liked how thorough your approach is — making sure to run the tests and linters, and also double checking diffs before opening the pull request.

I also like how as a reviewer, you focus on the code itself, and not the person. This is key for building relationships with empathy.

You also touched on the soft skills of humility, open-mindedness amd willingness to share knowledge. These will be key for growth in code review, as well as other aspects of your software engineer career.

A few pieces of feedback:

Take a look at Slack/GitHub integration. It'll automatically post in a Slack channel when a PR is opened.
Looks like all review comment examples are questions. Questions are OK when the deadline is far away, but remember that they also introduce back-and-forth — this adds cycles and blocks the PR. Know when to use a question, and when to make a direct suggestion.
The problem with pair reviews is that the reviewer has a person explaining the code, as they reviewing it. This is not a realistic situation. In reality, a developer will be reading the codebase on their own, as they're adding features. They need to be able to understand it on their own, and if they can't, it needs adjustment. Individual, async reviews realistically simulate this.
If PRs are forgotten or ignored, there's a problem with the team's code review process that needs adjustment.

Overall, awesome article! Thanks for sharing your thoughts, and for the shoutout! Good to be connected with you here on Hashnode.

Niranjan Borawake4y ago

Hello Curtis Einsmann and Tiger Abrodi,

Interesting topic and discussion. I think the code review is indeed a very important topic along with unit tests.

IMO, code review has more todo with people than actual code 😆 and hence some of the examples shared above become crucial.

I had put my thoughts on similar lines a while back -

https://blog.niranjanborawake.in/make-yourself-proud-by-reviewing-pull-requests

More from this blog

Become a cracked product engineer today

Qualities that make outstanding builders.

Jun 1, 20264 min read

Bipedal, humanoid, and the words for creature shapes in games

Bipedal vs quadrupedal: how many legs This is about the number of legs the creature walks on. Bipedal. Two legs. Humans, ostriches, T-rex, kangaroos, most fantasy humanoids. Quadrupedal. Four legs. Do

May 18, 20263 min read

Bipedal, humanoid, and the words for creature shapes in games

Lerp and smoothstep, what they actually do

Intro You hear "lerp" and "smoothstep" everywhere in game dev. They sound like math jargon. They're not. Both are small tools that do the same job: smoothly move from one value to another. The problem

May 18, 20264 min read

Lerp and smoothstep, what they actually do

IK and FK, what they actually are

What is Kinematics Kinematics is the math field. It's the study of how things move without worrying about forces (which would be dynamics). FK and IK are the two branches: forward kinematics and inver

May 10, 20267 min read

How to make your textures fast on the GPU

Intro Textures are usually the biggest cost in a 3D scene. Memory, bandwidth, and load time all get eaten by them. Resizing them is the obvious lever. There's more. This post is about the less obvious

May 10, 20268 min read

How to make your textures fast on the GPU

Tiger's Place

497 posts

Curtis is working on a Code Review course.

It made me wanna write my own piece, a guide to pull requests. I'm no genius on this, but I think I can surely share from my experience what has worked well, and what I ideally enjoy, so yeah, this piece is quite subjective I must admit.

The ideal goal in software development is to be agile, ship fast with quality.

I'll be covering the process from a pull request being created all the way to merged I guess.

Author

Alright, let's start with authoring a pull request.

How I like to do it:

Before committing, I will run the tests, linter, etc.
When committing, I'll check my own diffs in VS Code.
After having submitted a pull request, I will again in Github/GitLab check my own diffs as well as check the ticket's requirements and make sure I implemented the stuff I'm supposed to.
I will then run related E2E Tests for the feature I was working on (running all of them would take a long time).

When it comes to committing, I try to make my commits small and follow Conventional Commits.

Now usually after this, I would share the pull request with co-workers in Slack.

Reviewer

Reviewing, let's go.

First thing I usually do is to check what it is about, the description, the commits, and then open the related Ticket for the pull request, to see the requirements, so I can later make sure they all have been implemented.
Review the code itself. When it comes to reviewing code, make sure to focus on the code and not the author:

Instead of for example saying:

Why did you implement this here?

...you may say:

I'm curious, why was this part implemented?

Don't forget, in the end, you are a team, you and your teammate have the same mission, be humble, open-minded, and also willing to share your knowledge as well as learn.

I enjoy asking questions when reviewing, and not making assumptions, I feel like that makes me come across as more humble, understanding that I don't know the intent of the author.

Examples:

I'm curious, why is optional chaining necessary here?

I'm wondering if this is necessary, considering we have a similar function in util.ts?

I have seen this being repeated several times in our codebase, perhaps we could have a look and see if it can be abstracted into a single component?

I also enjoy making suggestions when it fits, of how something may look like that I had in mind. If I'm for example proposing an abstraction, I would then post some code in the comment of how it could look like and perhaps the file name I had in mind.

If tests haven't been implemented, I ask for them to be implemented if it makes sense. If for example a new feature with some of its own edge cases has been implemented, then it does make sense, but if it is a minor refactoring or CSS change, then obviously that's different.
Check out the branch and check the code locally, if something is UI-related, make sure it works properly and looks nice matching the design.

Pair Reviews

To be honest, doing code reviews in an async flow, that has most of the times from my experience been quite slow, at times confusions rising, and I feel like pair reviews are perfect, and never have I regretted doing them.

Author

If you author a pull request, when sharing it, ping one of your teammates Slack/Discord and ask them to do a pair review with you, instead of waiting for someone to review your code.

If they can't do it right away, it's best to talk about when to do the pair review.

Example from your teammate:

I can't do it right now, let's review it together in 45 mins.

Reviewer

For the reviewer, quite the same, just ping the author and ask them to do a pair review with you, or communicate when it is possible for you to do it.

My Experience

Pair reviews have been amazing. I love easily you can talk through things, ask questions, and exchange ideas right away if something is supposed to have been implemented differently.

It still blows my mind how much smoother and faster the flow is compared to an async code review, where you'd be writing comments back and forth with your teammate.

I have also noticed as the reviewer, that the author better understands the Ticket and its requirements, leading to fewer initial questions being asked from me as a reviewer at times.

Doing an async code review flow, I've noticed, sometimes the pull request either gets forgotten or ignored, leading to it being reviewed quite late after the author reminds the team to review their pull request.

Conclusion

I love pair reviews, and for now, will always advocate for them, my experience doing pair reviews has been great.

Obviously depending on whom you are working with, ideally, you'd wanna work with teammates who are humble and open-minded, and willing to embrace change.

Command Palette

Comments (2)

More from this blog

Author

Reviewer

Pair Reviews

Author

Reviewer

My Experience

Conclusion