2

Should the author of a changeset be required to obtain sign off from all reviewers who have reviewed the code or should code be allowed to check in as long as there is one sign off? I think giving veto power to reviewers does more harm than good as then they use it to blackmail (asking for unnecessary changes) and block checkins but would be curious to hear people's opinion on this matter as to what is the best practice to follow.

EDIT: Being more specific and to the point, Should a checkin be blocked if:

  1. there are some people (one or more) who have signed off
  2. but there are other(s) who have marked the diff as needing revision
  3. and the author has made a good faith effort addressing comments of reviewers who have marked the diff as needing revision but cannot come to agreement with them?

what do people think?

Improve this question
8
  • 3
    This is primarily an opinion poll. It may not be on topic here.
    – Adam Zuckerman
    Mar 29, 2016 at 23:35
  • 5
    I expect this to be closed as an opinion poll. If you are at the point, though, where you are worried about reviewers "blackmailing" other developers rather than being useful collaborators (i.e. "I would prefer you do X but I see that the other two reviewers disagree with me and have a reasonable argument so I'll live with it."), you've got deeper problems.
    – Justin Cave
    Mar 29, 2016 at 23:40
  • 2
    I think this should be left open. It's not asking if each of us has such a veto power in our teams, but rather whether it's a good idea, and although that depends on the environment to some degree, there is a useful "correct answer" to this which is basically what Telastyn already wrote.
    – Ixrec
    Mar 29, 2016 at 23:45
  • 1
    Your question reminds me of the warning on JSLint, if I may rephrase. Warning: code reviews may hurt your feelings.
    – Karl Bielefeldt
    Mar 30, 2016 at 5:33
  • 2
    @morpheus "We already tried supporting those questions, we even gave them their own site. Sadly, it didn't work out..."
    – gnat
    Mar 31, 2016 at 18:49

2 Answers 2

Reset to default
6

It depends on your environment. In a few environments, I've seen reviewers have veto power, and that was important since the skill level of the developers (and the impact if a bug made it through) necessitated it. For most environments, I don't believe that code reviews are necessary at all for 80%+ of check-ins. For some environments, I've seen team leads have veto power since in those environments, they were held unusually accountable for issues on their team.

But mostly, we're all professionals. If someone sees something in the code review that they think is a problem, then the team should be able to come to some consensus about what should be done. If the team can't come to some consensus, then I wouldn't consider the problem so severe as to block the check-in. Stop bikeshedding and focus on what matters.

Improve this answer
2
  • 1
    I think when there are issues, the issues are generally "reviewer confusing perfection with acceptability" not the review process itself. I think a lot of the negative behavior comes from people failing to see the code as a team creation and see it as a personal creation.
    – Gort the Robot
    Mar 29, 2016 at 23:58
  • @StevenBurnap - sure, I've seen a lot of that, but I've also seen a lot of disagreements simply because reasonable people put different levels of priority on different trade offs. As much as I hate process, it can help here derail some of those disagreements.
    – Telastyn
    Mar 30, 2016 at 0:15
6

It doesn't really matter. If a colleague were "blackmailing" you out of the proper scope of their reviewing job, you'd take it up with your manager or team lead and get it sorted out that way. No process is going to prevent someone with that sort of attitude to life from trying to get one over on you.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.