Pull request etiquette and why it matters…

As a developer or engineer part of your everyday work is likely to involve submitting code and reviewing other people’s code in the form of pull requests.

A pull request is a request for changing some code in a repository, normally created via a platform like GitHub. Once you have implemented the desired changes in the code, you submit a PR. Interested parties can then review the code changes and provide the author with any feedback or request changes. The idea behind PRs is to ensure that as a team you are writing high-quality, bug-free code.

Reviewing PRs can be trickier than it sounds so here are some things to think about and consider the next time you are looking at code written by another engineer.

Be empathetic and polite

As an engineer it’s really important to provide feedback in a polite and courteous tone. You need to remember that you are reviewing code written by your teammates’, the people you like to talk to, go for coffee walks with and go out to lunch with. All engineers are human, they have feelings, and feelings can easily be hurt. It’s a good idea to try and be empathetic during your reviews.

Provide examples for code improvements

Rather than just saying “this code is bad” or “this code could be better”, it’s generally far more helpful to provide actual suggestions or examples on how it could be improved, for example:

“I can see that this code works, but after reading through it again I thought of something I was keen to run by you.

function is_user_admin()
{
return current_user_can('manage_options');
}

What are your thoughts? I’m happy to pair on this if it would be helpful.”

Use emoji’s

Often online criticism can be easily misinterpreted, and this can lead to unwarranted tension and bad vibes. Emoji’s are a good tool for conveying a tone of voice textually. For example, we have a thing at Finder where if you’re reviewing a PR and you spot something minor that you don’t consider to be a blocker we include the tropical drink emoji. This way the code author knows we are still being friendly and not annoying and picky. This could be used for things like grammar corrections, spacing and indentation formatting, a minor code refactor, etc.

“[minor]: these properties should be in alphabetical order 🍹

Don’t be scared of taking it offline

If there is a lot of back and forth in conversations with a particular teammate, and things are getting lengthy and hard to follow, consider taking the conversation offline. There isn’t much point spending a lot of time typing when you could quickly hop on a call or roll over to their desk, or hop onto a call and chat through things. When people meet face to face you automatically develop more empathy. When you come to a conclusion it’s a good idea to post a comment on the PR summarising the discussion so other readers following along are aware of what happened.

Sometimes it can be hard to know how to respond to the feedback you’ve received on a pull request, so here are some things that might help.

When someone suggests a change

Writing code tends to be a very collaborative process. If another engineer has made a suggestion then always treat it as a potential way of improving your code. If you don’t understand something, it’s completely okay to ask for further clarification, and it’s not something to be at all embarrassed about.

“Would you mind providing me with an example, or clarifying this further? I’m not sure I understand it completely…”

If you like the feedback and think it’s valid, try implementing it, see if it works. Respond to the person that suggested the change and let them know how you feel about it.

“Thanks for that suggestion, I’ve implemented and tested it and pushed up the change, would you mind taking another look?”

If you disagree with a suggestion

There will be times when disagreements happen, different engineers have different ways of writing code. Always try and voice your opinion in a warm, friendly manner and stay professional. If you partially agree then let them know, but also provide a reason for disagreeing if that’s the case, it could provide them with more context or teach them something.

“That’s a great point. I agree with your first suggestion and I’ll work to implement that now. I don’t agree with the second point, however. I had considered that option and we decided not to do it like that because the PM has said we don’t want to display that value on the front-end.”

I believe that taking a little care with code review and pull request comments can lead to a happier and more productive team 🙂

Software Engineer, a11y and user advocate, experienced remote worker, creative coder, lover of all things front-end.