(no title)
rucamzu | 3 years ago
First and foremost, I like to think that as a reviewer I'm there not to point out issues but to provide useful feedback to the author. I'm not a compiler, therefore I shouldn't behave like one and dump error messages in the PR. Even if there are errors indeed.
Any and all feedback I provide is up for discussion. This is something I always like to highlight when I'm about to review someone else's code for the first time, no matter their level or experience. I'd rather have my comments discussed and challenged than taken for granted as issues that automatically need to be addressed. I might even throw in the occassional curve ball that I know needs challenging :)
I firmly believe the way I word my comments matters, and matters a lot - and I like to think I've managed to improve a bit over the years. PR comments are not commit messages. I avoid imperatives and quite often phrase comments as questions instead. Also, I favor using "we" as opposed to "you" ; in the end, what gets merged is the result not only of the author's but the whole team's work.
Also, I try to leverage PRs to educate when possible. I frequently include FYI comments illustrating e.g. more idiomatic implementations, potential refactorings or applications of design patterns, not as something I expect the author to fix but to learn from. Code snippets and links to e.g. documentation pages or articles are great ways of enriching a comment and providing guidance beyond the PR itself.
Finally, all these are things that I love to see when someone is reviewing my own PRs.
No comments yet.