Pull request review styles
Do you review pull requests by file changes or commit-by-commit? Do you squash and merge or always create a merge commit?
Author-reviewer mismatch
How you choose to author or review a pull request comes down to personal preference. Some people like a clean commit history, while others only care about the final diff1. The problem lies when the styles of the author and reviewer don’t match.
Context matters here. In some situations, one side will have leverage over the other. If that’s the case, adapt your style to match theirs. For example:
When contributing to open source, the reviewer has leverage.
When reviewing a pull request from another team, the author has leverage.
Teams with high levels of author-reviewer mismatch will suffer from ongoing [people] conflicts. In this case, you should define a policy and adhere to it. Shared agreement trumps personal preference.
Being a good author
The pull request review can only be as good as the pull request itself.
When you create a bad pull request (no description, unnecessary changes, etc.), you put the burden of the review on the reviewer. This can appear counterintuitive, but it’s your responsibility as the author to see the code through to production. So it makes sense to invest your time to speed up pull request approval.
Optimizing for review time ensures that you won’t go too far trying to make your pull request “good”. As such, aim for conciseness in the description and final diff.
Finally, it’s worth communicating your intentions. If you carefully crafted a cohesive commit history, you should recommend that the reviewer goes through the pull request commit-by-commit. On the other hand, if you didn’t clean up the commits (for whatever reason), you should ask them to focus on the file changes instead. Either is fine, as long as it’s explicit.
Being a good reviewer
There’s no point in reviewing a pull request commit-by-commit when the author clearly didn’t care about commit history. When you do that, you end up adding comments to early commits for something that will likely have been addressed in a later commit. Don’t try to fight the style of a pull request.
That being said, if you’re confronted with a bad pull request, it’s okay to [politely] mention that it was hard to review and that you could have missed things as a result. Chances are the author doesn’t know any better, and this can be an opportunity to start a conversation.
When you’re ready to approve the changes, it’s important to know what the merging etiquette is. Do you approve and merge straight away, or do you wait2? Do you squash and merge, or do you create a merge commit3? When in doubt, ask.
Purpose
Ultimately, you need to think about why you’re using pull requests in the first place. Is it serving a purpose, or is it just a box-ticking exercise? Pull requests can significantly slow down software delivery, so make sure everyone involved knows why they exist.
Not caring about the diff at all is just sloppy. That’s how you introduce bugs.
If you don’t merge a pull request straight after approving it, make sure there’s a valid reason for it. Too often I’ve seen approved pull requests sit there for days waiting for someone to click the merge button.
My rule of thumb is to squash and merge a pull request when the commit history doesn’t add value to the final diff.