> Teams often require code to be reviewed, so, yes, you _are_ asking for permission.
You conveniently left out:
> For one-off reviews, set the expectation: The developer is asking for feedback, and not permission.
I'm not speaking about what code reviews are, but what they should be. I've personally seen this policy work. It's not appropriate for all kinds of projects, though.
Furthermore, a nitpick: Requiring a code review does not equate to permission. It just means people are required to solicit feedback.
> If a reviewer offers feedback that you feel is unimportant, you can always ask someone else for approval.
If the team allows for this, then I find it to be totally acceptable. To me, it's the same as:
"a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus."
> Going behind someone's back like that is obviously unhealthy, so you should instead have a discussion with the reviewer about why something is/isn't important.
Agreed, and a solution is:
1. Make code review discussions transparent - at least to management (e.g. visible via a code review tool). This limits the potential for abusive teammates.
2. Don't make them go behind the reviewer's back. Simply invite additional reviewers (or escalate to the moderator). Do it openly so the reviewer is aware.
I never said "Don't have conversations". Conversations are necessary. I'm saying "Don't give the reviewer undue influence without proper checks and balances". If he is engaging in the tactics in the submission, ensure your team culture is such that this is open and can be handled. You can see in the other comments how common these problems are. Expecting most teams to be high trust and well functioning is unrealistic. Simple rules, however, can shift the team closer to that goal.
> If you feel like you can't have these conversations or the reviewer isn't taking your thoughts into account, then that's a sign that you aren't aligned.
Or (and more common), it's a sign that the reviewer is a poor reviewer and/or abusing his power.
Trust is a continuum. After all, the existence of code reviews is merely a formalization of distrust :-)
I think the policies and culture around code review will vary based on the situation (new hire vs seasoned developer, large vs small team size, minor vs major code change, etc). However, I don't think I advocated anything that would be a bad idea even for high trust teams.
The possible exception would be that a code review "not be a request for permission". If you have structured it as a gatekeeping mechanism, simply ensure a "referee" or "tie breaker" to smooth/hasten the process. Even with high trust/functioning teams you'll occasionally get pointless ratholing in the middle of a review, and both the reviewer and the developer should have a way to cross that divide. And no, a "be mature and discuss it through" will not work every time.
As for stating concerns - when is that ever a bad idea? I didn't come up with it, BTW. I learned it from reading books on effective communications. It's a good rule to live for everyday conversation.
I get annoyed when someone ignores my feedback and requests a review from someone else who is known to approve anything. I think doing this is fairly disrespectful and circumvents the point of code review.
Your comment is a good example of what I'm saying:
The problem is not the developer, but the guy who approves everything.
A team that requires code reviews, and has a guy who approves everything, is an unhealthy team.
The developer is asking for a code review not because he wants feedback, but because it is a ritual he has to go through because of team policy. If the team policy changed to "code reviews are for feedback, not permission", then he wouldn't waste your time. He's not the problem - the team's culture/policies is. If the focus was feedback, then you would only get requests from people who want your feedback.
> I get annoyed when someone ignores my feedback
Are you annoyed that he circumvented the purpose of the code review, or that he did not act on your feedback? If the latter, I strongly urge you to change. Acting on feedback should always be optional.
Of course, making code reviews completely optional is a luxury most teams do not have. But one of the reasons they don't have that luxury is that you'll rarely get a high trust team.
Despite this, one can put mitigating protocols with mandatory code reviews to solve some of the problems you're highlighting. If all the code reviews are via Github, and someone else approves the merge when you've left strong feedback suggesting otherwise, the team really needs to have a protocol for handling such scenarios.
I see, I think I'm better understanding what you're getting at, and I think I agree.
> Are you annoyed that he circumvented the purpose of the code review, or that he did not act on your feedback? If the latter, I strongly urge you to change. Acting on feedback should always be optional.
This has only happened to me with a couple of individuals. Usually in these cases I'll leave a comment and it will just never be addressed; the author will request another reviewer and my comment is ignored.
I don't hold PRs hostage and if things go more than one or two rounds of back-and-forth then I'll almost always yield to the author.
You conveniently left out:
> For one-off reviews, set the expectation: The developer is asking for feedback, and not permission.
I'm not speaking about what code reviews are, but what they should be. I've personally seen this policy work. It's not appropriate for all kinds of projects, though.
Furthermore, a nitpick: Requiring a code review does not equate to permission. It just means people are required to solicit feedback.
> If a reviewer offers feedback that you feel is unimportant, you can always ask someone else for approval.
If the team allows for this, then I find it to be totally acceptable. To me, it's the same as:
"a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus."
> Going behind someone's back like that is obviously unhealthy, so you should instead have a discussion with the reviewer about why something is/isn't important.
Agreed, and a solution is:
1. Make code review discussions transparent - at least to management (e.g. visible via a code review tool). This limits the potential for abusive teammates.
2. Don't make them go behind the reviewer's back. Simply invite additional reviewers (or escalate to the moderator). Do it openly so the reviewer is aware.
I never said "Don't have conversations". Conversations are necessary. I'm saying "Don't give the reviewer undue influence without proper checks and balances". If he is engaging in the tactics in the submission, ensure your team culture is such that this is open and can be handled. You can see in the other comments how common these problems are. Expecting most teams to be high trust and well functioning is unrealistic. Simple rules, however, can shift the team closer to that goal.
> If you feel like you can't have these conversations or the reviewer isn't taking your thoughts into account, then that's a sign that you aren't aligned.
Or (and more common), it's a sign that the reviewer is a poor reviewer and/or abusing his power.