Pull requests are one of the most essential collaboration tools for software development teams of this day and age. In this article I wanted to explore what worked for me in the past and what to avoid while doing pull requests.
What to avoid?
First of all you should avoid commenting on style. There are dozens of automated solutions that you can use to make sure the style of the pull request matches company standards. For example there is a lovely tool called Hound that you can use to achieve that. Another problem with commenting on style is the fact that you can become lazy and not try to dig deeper in to the logic and just skim over the most important details like: Business logic, design of the code and typos.
I think one of the most crutial parts of the PR culture is to educate people on your team. Make sure that when you spot a potential problem in the code you explain why it matters to change it. Those could badly named methods or variables, places where you need to extract method, extract class, apply strategy pattern instead of case statement, give a conditional a name e.t.c.
Don't try to fall in to the trap of pointing out mistakes and saying: "Gocha sucker!".
Another essential part of education by PR is that you should point out things that people did great. You want to give recognition for awesome parts of the code that people have made. As a bonus, it gives your team ability to learn form each other's good techniques and not only the bad ones.
Try to be non assuming
This is more related to the open source projects than to a team that has worked together for a while. It is easy to sound like an asshole on the interweb even if you don't intend it to be that way. That happens to the best of us :) So, when someone comments on your PR, don't try to be defensive about it. If there is not enough information, you should ask for it. If you want to point out some ugly part of the code and want to do that gently as possible, try to phrase it like this: "I think this part of code should... What do you think about...?". Question is a form of communication that is really hard to interpret as being aggressive. Use questions more.
PR is a great tool if you utilise it right. In fact, Github recently added a lot of modifications to the PR functionality and I encourage you to check it out. Make sure that it is first and foremost an education tool and not a tool to point out peoples mistakes. In the carrot and stick approach, carrot works the best most of the time :)