What are PR’s, how to effectively create a PR, how to give feedback on PR’s and how to respond to feedback
What is a Pull Request (PR)?
It is a request for changing code in a repository. Once you make changes you need in your code, you submit a PR. Once submitted, interested parties will perform a code review and provide you with any feedback/changes needed.
Pull Requests are typically used by teams for shared collaboration and feature work or bug fixes. The idea is to make sure well written and bug-free code gets pushed to the repository. It is a way to develop high-quality code.
How to Create a Pull Request
Breakdown your story/feature
Before you start working on a story/feature, make a mental/written note on how you want to break it down into several smaller PR’s. This doesn’t just help your reviewer but also YOU as you can keep track of your progress and get frequent feedback. Another benefit is, as the codebase is shared with other developers, reverting a change gets a lot easier. Hence, try to keep the scope of your PR to a single issue while breaking it down.
For example, let’s say you are implementing a todo list and have the following story:
As a user, I can add and delete items to/from a list respectively
I would break it down by:
PR #1: Add a text box and an add button on the page.
PR #2: Clicking on add button adds the item to the list
PR #3: Clicking on delete button on the list item deletes that item from the list
You will get reviewer blessings 🙏 for doing so. Imagine if your co-worker blasted a HUGE PR, you wouldn’t want to review, would you? 🙈
Add comments on your PR
Wherever you have done something different, usually, it’s a good idea to 🔗 add a comment on the line number explaining why you did what you did. If you are not completely sure of your code, you should definitely tag “@” at those who have the most context on that code to get some feedback. For example,
You used a new function from the library Lodash called
isNanwhich might not be used in the codebase a lot, you might want to add a comment there saying
this function basically checks if the value is Nan and returns a boolean.
This helps your reviewer a lot as that saves their time (i.e. if they don’t already know what the function does).
Screenshots and gifs are proof that the code that you submitted works! This increases the likelihood of acceptance and also gives your reviewer a visual of what you have implemented. A screenshot definitely works although when your code is doing more than what a screenshot can capture, create a gif. By using a gif, you can click around and show everything that your PR covers. My favourite one is: Licecap 🔗
Tests are documentation of your code
Whenever you have worked on something which makes sense to you and you want to explain this complex code to future developers your intention for that particular change. Don’t litter the code with code comments and ask yourself these questions, if it’s so difficult to understand,
- Can I split this code into smaller functions?
- Can I rename this function to something more obvious? ask your team for function name suggestions if you like.
- How can I simplify my code further so I don’t call it complex?
Irrespective of what you pick, you should write a TEST. 🔥
Your tests are there to test your logic but also, are the documentation of your code. Try to add a test explaining that part of your code.
How to review Pull Requests?
As a reviewer, it’s extremely important to provide feedback in a courteous manner. You need to remember that you are reviewing your teammates’ code who you like to talk to and go for team outings, lunches etc. They are human, they have feelings and feelings can get hurt easily! 💥 Hence, be more empathetic during your reviews.
Make yourself more familiar with the code
Always get in the habit of teaching and it doesn’t matter if you consider yourself a junior or a senior developer. Instead of telling them what the problem is, ask them questions, make them think and use a friendly tone. Following are a few examples:
- Do you think maybe we could assign this to a variable and re-use it on line 9?
- Could we possibly use this helpful utility that already does that for us which our dear teammate Sarah built?
- Can we move this code in its own function so we can write more tests 😃?
- What do you think about trying this option 🤔?
- I’m not sure if I understand the whole picture but could you explain what this function is doing? This will make them possibly think that the function name could be renamed. After they explain what it does, then follow it up with a suggestion such as:
“I get it now, sorry for being slow! I’m not sure but do you think we could rename this function to option1, option2 or something along those lines, what are your thoughts”?
Always ask them their thoughts as remember you are a reviewer and they wrote the code so its possible they have more context than you do.
Imagine if your co-worker used words such as “Don’t do this, just rename the function”. That sounds so much direct, less friendly and sounds like you are giving them a command rather than taking their input on it.
Always provide suggestions for code improvements
Rather than just telling them, “this code could be improved”. Provide them suggestions/examples on how it could be improved further for example,
This code works perfectly but after I started reading it further, I thought of another idea that I wanted to run it by you. I’m not sure but what about?
sample code #1,
sample code #2
What are your thoughts?
Online criticism is misinterpreted easily. Hence, emoji’s are one of the best tools for communicating online. Using them automatically adds a friendly tone to your sentences. Use it when you start feeling that the conversation is getting too serious . But if that’s not you then don’t force it, keep it real.
Take it offline
If you find yourself going back and forth in conversations with your teammate, the conversations are getting too long and its leading nowhere, take it offline. There is no point spending time typing when you can quickly hop on a call 🤙or just walk 🚶 to their desk.
When you meet face to face, you automatically develop more empathy and understand their point of view even further and alternatively, they understand yours. When you come to a conclusion, post a comment on the PR summarizing your discussion so other readers following along are aware of it and your future self will thank you for it.
While reviewing code, you may find something that is basically a nitpick i.e. it may not necessarily block the approval of the PR but something to consider. Nitpicks are not important but technically correct. They can be related to grammar corrections, un-intentional new lines, any aesthetics, minor code refactor etc. For example,
nit: extra white space
nit: can we assign this value to a
constantfor better readability purposes?
How to respond to feedback
The rules that apply while submitting a PR equally apply while responding to it as well.
When suggestions are provided
Always remember that you are collaborating and writing code. If a suggestion is provided, always treat it like a possibility of improving your code. If you don’t understand something, ask for clarification for example,
“Can you please provide an example or Can you please clarify further? I don’t understand it completely.”
If you think the feedback is valid, implement it! However big or small the change is, if you think the feedback is valid, always reply and let the reviewer know that you took their feedback. For example,
That’s an excellent point, change coming right away!
Good point, thanks for catching that.
If you disagree 👈
Disagreements are bound to happen as different developers have different ways of writing code. Although, try to voice your opinion in a very friendly manner. If you agree to them partially then let them know but also provide your reasoning at where you disagree with them. For example,
That’s a great point. I do agree with your first suggestion, will make a change right away. Totally missed that one. Although for the 2nd one, I did consider that option and decided to not go that route as for the following scenario, we don’t want to display that. //or whatever your reasoning was, be honest. 👈
Make your commit message obvious
Now that you spent time on your feedback, write a good commit message which lets the reviewer know that you did consider their feedback but don’t make the message too long. A very handy tool is 🔗 Gitmoji. 😍 They provide an easy way of identifying the purpose or intention of a commit by looking at the emojis used.
Take it a step further
You have made some changes in your code based on the feedback you got. Now your code handles an additional scenario which you didn’t think of or wasn’t aware or may have missed but there’s your chance to “Write a test”.
Once your code is merged in, a defect gets filled. Once you fix that defect, add a test to make sure that the defect is actually fixed and there’s proof 🕵️. Now, its documented!! 😃 You get brownie 🍫 points if you take it a step further. You will automatically gain the trust of your team.
I hope you enjoyed reading this article and learned something. If you did, please give it some ❤️ and also, would love to hear your personal experiences on pull requests in comments. If you have any questions, please feel free to reach out on my Twitter.
Oldest comments (24)
Love the nitpick idea. I always clarify with something like “not a big deal but” so I’m definitely going to start prefixing with nit instead of doing that.
Yeah, that's awesome that you loved it. nitpicks are minor but technically relevant. I love them. It removes the awkwardness of being picky (sometimes) 😅
Someone wrote "nit" in a code review at my company once, and someone who wasn't familiar with that term thought that was the preferred name of the author 😂
Haha this is hilarious. I was going to suggest that we should share some common practices with new devs or new clients although we wouldn't get to giggle on this one then 😁
Only thing is I've had people say "nit" to me on things that were pretty subjective (like it deviated from their preferred style but was in no way a standard for our team). In those cases using "nit" almost seems insulting, like they've assumed that their suggestion is correct. I think "nit" is fine when it's something obvious (like extra whitespace or a typo).
We are using prefixes for close to every comment:
Minor:means the commitees can chose whether to improve that one
Major:this one needs to be adressed
works quite well for us
Agree completely with the need to be empathetic as a reviewer.
Fear and power struggles are a chronic problem in our industry during code reviews.
This alone can do a lot to reduce it!
Yeah I have been through both. Honestly we all want to feel inclusive right? So if we do our bit, we would get to write better code collectively.
I make it a point to only ask questions on PRs. Even if you know you’re right, when you couch it as a question, you offer the author the chance to discover the knowledge on their own (and that way they can own the solution) rather than telling them what to do (and they rent it).
As the old sales adage goes: telling ain’t selling. Guiding people to the right answer with questions produces much better results.
Wow, I wish I knew about the old sales adage before I published this post. This is awesome. Thanks for sharing this.
I also love to do the same. Always ask! I think of 5 other things when a teammate raises a question.
Loved this post!
Glad you liked it :)
Wow. I loved the blog. Especially where you explained, "How to review Pull Requests?". What an explanation. You have covered almost everything. Right from the documenting the pull request to merging.
Also, I am willing to discuss/add a few more points about creating a pull request. Did you forget to talk more about the points following image has? Or you don't want this to be part of the blog? I don't know. :D
Thanks much! And I appreciate your efforts for adding more knowledge about writing code reviews. Cheers!
Hey glad you liked it. Haha yeah I intentionally left that bit out as the post was already getting too long, also GitHub has so many cool features for PR's that one post won't fit everything. Although I'm glad you added it as most readers would go through comments.
Also, there is so much more I want to share, probably a part 2 article should be written. Hmmm time for more weekend work 💃
Haha. Okay. COOL. B)
Yesss!!! I am a big supporter of the nitpick idea. Makes people accept me for being picky hahaha
haha that is true and you don't have to apologize by saying
sorry for being picky
Thank you so much. That was so much fun to read. It is easy to misinterpret the tone in PRs. Humanizing and being empathetic to our reviewers is an important still.
Im definitly gonna start using gitmoji moving forward.
Another great tool for creating GIFs is Kap. I used LICEcap for a while before trying Kap. They're similar in a lot of ways, but Kap has, in my opinion, a nicer interface with some additional helpful features.
Yeah, that's a great tool. Thanks for the recommendation ✨. I will start using it now and you are right, Kap has a much better interface than LiceCap. ✨
"Take it offline" can't be emphasized enough, imho. Expand that advice to client communication, or cross-team communication, etc. Learning to sense when it's time for a call is a really good skill to have.
An imperfect signal that it's time to walk over for a face-to-face or give a call is if someone's kind of stressing out and the email you're writing to try to assuage a concern is hitting 2-4 paragraphs in length. And you're spending 30 minutes trying to be very specific and clear in your language.
Very clear, useful and a real post made for humans. Thanks Ankita
Glad you liked it 🙌
Great article Ankita!
I have couple of comments 🐿
I think it's not the best example. First, if it was really “basically”, you wouldn't need to write a comment. Second, the comment merely repeats the function name itself.
Better example would be something like: “I'm using the
isNaNfunction from Lodash, which I've never used before, and I'm not sure it's the right way to use it”.
Also if it's something really tricky or unusual, better to write a comment in the code, not in the pull request.
As a reviewer I don't completely agree with this. Receiving 20 comments with “fixed” for a particular pull request isn't fun. As a reviews I expect that all comments will be addressed in some way: by changing the code or explaining why it won't be done. Saying something like “thank you, great idea” occasionally is a great thing though ;-)
I think this depends on a workflow and a particular team. Other all commits get squashed into one before merging, so only one commit message per pull request matters. Which should be very good ;-) Also as a reviewer I don't really care about particular commits, but maybe that's just me. For me a comment by the author, saying that the pull request is ready for another review iteration, is much more useful.