I have a few personal guidelines I try to follow when reviewing pull requests.
Do not comment on style matters that we have not agreed upon as a team. Yeah, I like single quotation marks and they have used double quotation marks here, but we haven't agreed on either so let it go. If it really bothers you, start a discussion with the team about code style instead, but pull requests are not the place for that.
Try to understand the reasoning behind decisions that I don't agree with. For instance, instead of saying it's bogus to create a class for this logic when it could easily have been just a function, I'd say is there any reason to use a class here instead of a simple function?. If I don't find the response satisfactory I can always start a discussion later, but more often than not either I missed something that justifies the design, or the author will be like oh yeah, hadn't thought of that, I'll change it.
When asking for changes, offer code suggestions. I'd say something like this logic is repeated in these three places, we could create this higher order functioncode exampleand then compose it in these places like thatanother code example. Then the author will have a clearer idea of what I mean. Plus, if they are lazy they can just copy paste my suggestions.
If something is not critical but I think can be done better, offer it as a suggestion for the future without requesting a change in the current pull request.
Make positive comments. I can't stress this enough, but if you only take one thing from my post, take this. Especially if you are just starting with pull requests or as a team. If you learn something you didn't know, see a smart trick, the author applied something you taught them previously, or simply think a piece is particularly elegant, write a comment to tell them. You can hardly show too much appreciation to your colleagues. Showing genuine appreciation goes a long way to create trust within the team, and it's easier to take criticism in a part of the code if you're shown appreciation in another part.
The bottom line is try to make sure that the code is good enough, and that you all are learning in the process, not try to make it as if you would have written it.
I have a few personal guidelines I try to follow when reviewing pull requests.
code exampleand then compose it in these places like thatanother code example. Then the author will have a clearer idea of what I mean. Plus, if they are lazy they can just copy paste my suggestions.The bottom line is try to make sure that the code is good enough, and that you all are learning in the process, not try to make it as if you would have written it.
Hi Avalander,
Thanks a lot for the suggestions. These are very helpful. I'll follow them all.