alt Hacker NewsGerrit has -2...+2.
-2: This is a bad idea, don't do that
-1: This is a good idea but needs improvement
+1: LGTM but I don't have enough knowledge or authority to approve
+2: Approved
Replies
I was in camp 'boolean', but I think this has convinced me. I always had a problem that there were developers who didn't really understand the code, but would click 'approve' anyhow because they didn't see any problems in the parts they understood.
This meant that they were completely unable to actually 'approve' a review, but were only able to reject it. They were juniors, so they'd eventually get to that point, but by then, everyone would be used to just ignoring their approvals.
This provides that middle ground.
everything except +2 is unapprove.
The nuance is comments on the PR itself, rather than the state of the approval, which is binary (or ternary, if you want to count leaving it in an unknown state for extended periods of time).
And Gerrit has multiple review label that can be customized[0].
So you could require `Verified+2` (CI), `Code-Review+2`, and `Design+2`, for example.
[0]: <https://gerrit-review.googlesource.com/Documentation/config-...>
I think we are normalizing the PR process here, in reality its more convoluted and a good reflection of the team/organization itself. The relationship between the author and reviewer can have negative impact on the rationale and desired outcome of the PR itself.
To run the process smoothly, one can just hope that the team/tech lead is an ideal developer. Otherwise they are in a position where no one senior than them is available for the code review and any one junior would just rubber stamp their PR's.
This seems like it’s conflating problems. It’s actually two different problems:
1. Is the PR suitable, and therefore should be approved, and
2. Is this person suitable to make that decision.
If 2 is false then the person should remove themselves from the list of reviewers. Then 1 can follow its normal process.
This could also solve the problem Github has where anyone with an account an "approve" a PR, but if you aren't a maintainer for the project your approval doesn't mean anything as far as actually getting the PR merged, but can be a signal to the original author that it is probably good, and to the actual maintainer that the PR is worth considering.
But with this, a non-maintainer could review be allowed to give a +1 or -1, but not a -2 or +2, and it is more clear that a "+1" isn't sufficient for actually merging the PR.