“Pull requests let you tell others about changes you've pushed to a branch in a repository on GitHub. Once a pull request is opened, you can discuss and review the potential changes with collaborators...” - Github
In this article, I’ll delve deep into the mystical art of submitting and reviewing Pull Requests (PRs), with a focus on the following three key areas:
- Preparing your PR.
- Receiving feedback on your PR, and
- Giving feedback on someone else’s PR.
I’ll also include some useful resources that have inadvertently helped me along my Pull Request journey thus far. These principles apply to all software developers, regardless of experience level.
My Love Hate Relationship With PRs
I recently reached the 100 PR milestone in one of the main Ruby on Rails repositories we have at Zappi, the company I have been plying my trade at for the past two years.
Despite the fact that submitting PRs is such a regular occurrence in my life as a software developer, this seemingly trivial act can still be quite a daunting exercise for me. Especially when I am working on a complex feature or navigating a particular area of the codebase that I am not completely comfortable with.
Opening yourself up for criticism and judgement is an extremely vulnerable activity that has the potential to impact you either positively or negatively. And when you are an HSP (highly sensitive person) like me, just a hint of destructive criticism not only has the potential to derail your day but also your self-worth as a human being, your entire life’s purpose, and your career as a software developer as a whole.
Ok... slight exaggeration, but you get my point.
In order to counteract my heightened sensitivity to judgment and critique, I have come to understand that there is a pattern that defines a good PR and I have realized that following a core set of principles when handling PRs - regardless of whether you are the submitter or the reviewer - will result in a much smoother process for all parties involved.
Why are PRs important?
There are a number of reasons why PRs are important, with the most important being that you need another set of eyes to critically analyze the changes that you are about to introduce.
When you are developing software, whether it is a new feature, a refactor, or a bug fix, what usually happens is that you have been looking at your code for so long, and over this period of time, you have gradually allowed yourself to see the world from a very specific viewpoint.
While this is expected, it is slightly dangerous because this viewpoint of yours is often riddled with blind spots and confirmation biases, that are often reinforced by the Dunning-Kruger effect. Getting feedback from others via a PR, allows you to break away from this pattern of thinking.
The act of having other developers, regardless of whether they are more or less experienced than you, review your code, provides you with an opportunity to receive well-rounded feedback that you can either act upon or use to further confirm your approach and rationale.
Step 1: Preparing your PR
“Indeed, the ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. ...[Therefore,] making it easy to read makes it easier to write.” ― Robert C. Martin, Clean Code
1. Write presentable code
This should be one of the golden rules of coding in general: write presentable, concise code that makes sense. This simple act will make everybody’s life easier.
Over the course of your career you will read much more code than you will write, so it is absolutely imperative that you ensure others can read and understand the code that you yourself write.
Your code needs to be clean. It doesn’t need to look complicated. It doesn’t need to be verbose or superfluous. It doesn’t need to be composed of fancy, extremely obscure, complex, hard-to-read one-liners. You are not participating in a coding challenge, you are contributing to a shared repository. Thus, your code needs to get the job done, nothing more, nothing less. It needs to have meaningful method/variable names. It needs to follow the conventions of that particular repository. It needs to be readable, and all in all, it just needs to make sense.
2. Add detail to your PR description
Your code should be concise but your PR description doesn’t have to follow that same principle. I prefer to write and read PR descriptions that offer as much detail as possible, obviously, in a readable, formatted, presentable manner. Your description should tell me what you are doing, why you are doing it, and in certain situations, it should also provide an outline of the how as well. After I read your description, I shouldn’t have any more questions, doubts, or uncertainty.
- If it is a new feature, add a link to the ticket that describes the user story and briefly touch on the technical implications of the changes you made.
- If you are fixing a bug then detail why the bug was happening in the first place and what you have done to fix it.
- If you are making a change to anything visual, then include a screenshot where possible. If there are many visual changes as part of a new/changed workflow, then drop a link to a pre-recorded video either in the PR or on the ticket.
- If you have done a refactor to the database or added some new tables, then throw in a UML class diagram.
When someone reviews a PR, before they look at a single line of your code, they should have the full context of what they are looking at and why these changes were made. Your PR description is your opportunity to provide that context.
3. Keep a clean commit history
As a developer, you should cultivate a habit of committing regularly in logical chunks, with commit messages that make sense. It will help you out in the future when you are looking back and trying to figure out what happened and when. Even if you squash all commits before merging in your branch, you still shouldn’t code as if you are participating in some arbitrary hackathon.
4. Get feedback as soon as possible
There is no rule in software development that states that you should only submit a PR when the work is complete. No, not at all. In fact, one of the joys of PRs is that you can submit one whenever you want. It is never too early to submit a PR.
So if you are working on a complex change, if you are not too confident about your approach, or if you want to get approval of your thought process and rationale from your peers, then you should submit a PR early, maybe even throw in a to-do list for transparency. Just make sure you make it clear in your title or description. You can also use a custom label like “WIP” (work in progress) or “DO NOT MERGE”.
The benefit of the early PR approach is that your reviewers will show even more empathy towards you because you are basically saying “hey guys, I’m not too sure about this approach, but this is what I am currently thinking, please let me know your thoughts.”
All the points I have mentioned above will still apply to your draft PR. Even if it is not yet complete, it should still be clean, concise and presentable.
5. Notifying people about your PR
You need to have an effective way of informing the rest of your team that your PR is ready. On top of this, you need to ensure that your team has incorporated PRs into it’s day to day workflow so that no PR can be easily overlooked. I’ve encountered many instances where a PR has been waiting to be reviewed for days on end to no avail.
In our team (The Oilers), we manage PR transparency and communication with a combination of a “Ready for Code Review” Jira Swimlane that we talk about during standup every day, as well as a minimal-noise slack channel dedicated solely to PRs.
Step 2: Receiving Feedback
6. Leave your ego at the door
The first piece of advice I would give when it comes to receiving feedback is to leave your ego at the door. Try your best not to bring emotions into the process, and don’t be so sensitive about the code just because you wrote it. The more objective you are throughout, the easier it will be.
I realized that I was dragging my ego into my PRs the day I caught myself being super defensive over code I had copied and pasted from another file. It wasn’t even my code, yet I was still being so protective of it. #smh
7. It is code that you wrote, it is not who you are
You’ve spent hours, days, weeks, maybe even months working on this code, and naturally, you’ve probably formed some sort of emotional attachment to it. This is understandable, we are software developers, this is what we do. But in spite of this, you should fight the urge to identify yourself and your self-worth with the code you have written.
You need to understand that the feedback you receive is for the benefit of the codebase, the product, the team, and the company as a whole and it is not a direct critique of your personal self-worth or your overall value in the world. So don’t take things personally. Take it on the chin and move on.
“I don’t believe people wake up in the morning saying ‘who can I treat poorly today?’. Always assume positive intent.” - Mary Frances-Winters
8. Always assume the best intentions
One thing I have repeatedly noticed when it comes to interpersonal relaying of information is that text-based communication can be extremely ambiguous.
When people are interacting in person there are a lot of nuances that one can easily pick up on, which are not equally transferrable when interacting via text, and this makes it very easy for any plain text comment to be taken the wrong way. Because of this, I would advise that, as a premise, you should always assume the best intentions of others, unless given an explicit reason to do otherwise.
9. Don’t leave people hanging
Contrary to popular belief, developers are people too, and we would greatly appreciate it if our efforts and opinions are respected and acknowledged.
So when you receive feedback on your PR, be ready to act upon it in a timely manner. Either implement the feedback or engage the reviewer in a respectful conversation explaining why you chose to do what you did and why you think it’s the better approach. But don’t let people’s reviews go to waste. If you don’t think something is important/relevant, that is fine but don’t silently disregard it.
10. Don’t hesitate to reach out
If you see a comment that you don’t fully understand, don’t hesitate to ask for a more detailed explanation. Sometimes, we software developers tend to treat our interactions with other people as if they have an all-year-round VIP pass to the inner workings of our prefrontal cortex.
So we will drop a vague comment that makes complete sense to us, without taking into consideration the fact that other people might not be on the same wavelength as us. When this happens, just reach out and ask for clarity.
11. Ensure you preserve the context
A PR lives on forever, so it is important that you keep all the context in there when possible. If you start a conversation in a PR, do your best to keep that conversation there so that it is easy for other people to follow.
This is important because at any point in time there could be multiple people reviewing the same code both in the present moment and in the future, long after it is merged in. So you should do your best to ensure everyone is still in the loop.
I can recall a number of occasions where a thread from a PR has escalated into more synchronous forms of communication like a Slack conversation or a Zoom call. And when everything is said and done I will be sure to drop a comment on the PR, either summing up what was discussed or just simply stating that it was discussed, before resolving the thread.
Step 3: Giving Feedback
12. Avoid ad hominem criticism
Your task is to provide feedback on the code/design/approach, not the person. You should never confuse the two. If you do happen to have a personal qualm, then that should be handled elsewhere, not on a PR.
13. Be aware of the context
Make it a point to take the context of the situation into consideration. Yes, you should always be consistent in your reviewing, but how you present your feedback shouldn’t be entirely agnostic of the person that requested the feedback.
For example, how you respond to a developer who is very experienced in that codebase might not be exactly the same as how you would respond to a junior who just started programming in Ruby a week ago.
A general rule of thumb that applies to all forms of communication is that you should always empathize with your target audience as much as possible.
14. Don’t be afraid to ask “why?”
Within the realm of problem-solving, “Why” is one of the most important questions that one can ask. You should never think that it will make you look ignorant, or that it will undermine the person who wrote the code.
PRs should be seen both as an opportunity to critique and an opportunity to learn. So if you see something you don’t understand, then ask why that is happening. Simple as that. To this day there are parts of our repositories I am very active in, that I still haven’t seen. My exploration of our code is akin to humanity’s understanding of the deep-sea, where there are literally new species being discovered every year.
On the other hand, asking why also encourages people to question their beliefs, their rationale, and their approach. Often we developers (AKA hackers) do things simply because “this is just how it’s done”, “this is what was there before”, “without this it doesn’t work”, or simply because “StackOverflow said ...”. And as a result of this inertia, sometimes you will find that developers don’t have a valid answer to the “why” question that you have posed. When this happens it means there is a much more pressing issue at hand.
15. Don’t feel pressure to approve
If you don’t feel comfortable approving a PR then don’t. It’s ok to disagree with an approach and to withhold your approval. Every opinion is valid and you shouldn’t feel any pressure or obligation to push something through if you are not fully convinced. Just be sure to communicate this effectively.
16. Don’t be afraid to come across as pedantic
When reviewing PRs, you should feel free to leave comments and suggestions despite how minor you think they are. Don’t ever be afraid of being regarded as pedantic.
As the proud Grammar Nazi that I am, I will not hesitate to correct your typos, fix your grammar or sort out your punctuation. I’ll even add spaces, and remove unnecessary new lines in all the places that either you or your linter overlooked. Yes, I am aware that it’s a code review and not an English essay submission, but as a writer, I take great pride in all forms of communication, regardless of whether I am creating or consuming. I don’t discriminate, and neither should you.
One day, years from now, the repository will be in a much better place because of you and your uncompromising attention to detail.
17. Leave room for preference
When it comes to giving PR feedback there is a fine line between subjectivity and objectivity. And as a software developer, you must be cognizant of where that line is.
Just because you would do something in a certain way doesn’t necessarily mean that is how it should be done. Different people tend to have different opinions, and in some situations there might not be a ‘right’ way to do something, and that’s ok.
18. Beware of the bystander effect
I have seen some big PRs in my life and I have been intimidated by them. But I have come to realize that even the big PRs need some loving. Big PRs are PRs too. So you should not procrastinate by continuously putting it off, or thinking that “another developer will do it”. Just bite the bullet/eat the frog. The world will be a better place after you view all 347 of those files.
19. Review as many PRs as you can
Reviewing PRs is a win-win for both parties, so it is important to review as many as you can. By reviewing a PR you gain more context about what is happening in the codebase, and the developer who submitted the PR has the benefit of having an extra pair of eyes view their work.
It is vital that you incorporate regular PR-reviewing-time into your weekly schedule, because those PRs will not review themselves.
20. Don’t be so serious!
At the end of the day, it’s just a PR, so don’t be so serious. You should always make room for banter because good quality banter makes the world a better place.
So don’t think that you need to always be super serious in the comments section of a PR. Yes, you should be professional and yes, you should always keep it #SFW, but there is always room for a joke, some sarcasm, and some kick-ass emojis.
Alright, there you have it, my core PR principles summed up into one article. If you have any interesting PR learnings that you would like to share with me, then drop a comment below. To read more of my articles you can check out my blog. Otherwise, happy coding my fellow developers. The world is your oyster, go forth and prosper.
Resources:
Preparing your PR:
- Clean Code: A Handbook of Agile Software Craftsmanship - Robert C. Martin
- The Pragmatic Programmer: Your journey to mastery - David Thomas, Andrew Hunt
Receiving feedback:
- The Power of Vulnerability - Brene Brown
- Why incompetent people think they're amazing - TedEx
Giving feedback:
- How to Win Friends & Influence People - Dale Carnegie
- Crucial Conversations: Tools for Talking When Stakes Are High - Kerry Patterson, Joseph Grenny, Ron McMillan, Al Switzler, Laura Roppe