| ▲ | ahartmetz 6 hours ago |
| Gerrit 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 |
|
| ▲ | thayne 33 minutes ago | parent | next [-] |
| 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. |
|
| ▲ | wccrawford 4 hours ago | parent | prev | next [-] |
| 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. |
| |
| ▲ | hnfong 3 hours ago | parent [-] | | Either the code gets merged or it does not. That's the inherent boolean part. Given that, what's wrong with simply commenting on the PR to document the concerns, issues, lack of knowledge, etc? Unless you're using those +/-2 to achieve some sort of goal... but you can also do that with labels, tags, etc. on the PR. | | |
| ▲ | kps 3 hours ago | parent [-] | | > Either the code gets merged or it does not. That's the inherent boolean part. In many environments that depends on more than just code review, e.g. CI. | | |
| ▲ | hnfong 2 hours ago | parent [-] | | Sure, it depends more than code review, but the code review is still a boolean flag, i.e. BOOLEAN getsMerged = codeReview && passesCI && passesTests.... Unless you're implying codeReview is a score and a low code review score can be offset by higher scores elsewhere eg. passes more tests????? |
|
|
|
|
| ▲ | chii 5 hours ago | parent | prev | next [-] |
| 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). |
| |
| ▲ | newshackr 4 hours ago | parent | next [-] | | What if you want someone to look at a portion of it but they don't know enough to approve the whole thing. They give +1 Someone else knows the other portion well and sees the +1 and decides to +2. In practice this ends the stalemate where partial owners don't feel confident to approve the whole thing | | |
| ▲ | chii 4 hours ago | parent [-] | | The PR needs to have someone who knows the whole thing. Having several people review each separate parts but not understanding the others' can cause interaction bugs. If such bugs cannot happen (say, due to modularity, or type safety guarantees etc), then it won't be the case where you need to have a partial approve. I am not a fan of partial approve. Either you think the code is approvable, or it isn't. | | |
| ▲ | benlivengood 2 hours ago | parent [-] | | Domains of expertise are a thing. E.g. Google had "readability" which was the code style and opinioned language expertise that one person might have even without the deep system knowledge for a PR. You can require approvals from N domains from (potentially) different people. |
|
| |
| ▲ | unethical_ban 3 hours ago | parent | prev [-] | | To be clear, that is an opinion, not an objective truth. Some people think that PR status can also communicate rationales and partial approvals. Some think that should be done with tags and comments. Lots of request systems have multiple stages between "open" and "resolved". |
|
|
| ▲ | thcipriani 5 hours ago | parent | prev | next [-] |
| 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-...> |
|
| ▲ | sandeepkd 2 hours ago | parent | prev | next [-] |
| 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. |
|
| ▲ | rprwhite 3 hours ago | parent | prev [-] |
| 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. |
| |
| ▲ | zephen 2 hours ago | parent [-] | | I like the idea of being able to merge a PR that is a partial solution, while keeping the issue open to reflect that it is only partially done. It kinda makes sense to do this in a single action. Also: > If [a person is not suitable to make the decision of whether the PR should be approved] then the person should remove themselves from the list of reviewers. This doesn't reflect what sometimes happens in real life. Someone could have sufficient specialized knowledge to be able to veto a PR, without having sufficient broader knowledge to approve a PR. That person should definitely be left on the reviewer list, with the ability to veto, the necessity to remark if he has vetoed or not, and the inability to definitively approve. It is necessary for this specialist to notate "I have finished examining this PR, and there is nothing within my expertise that would cause me to veto it" before the PR is advanced. Unfortunately, in a binary system, that often equates to him having to say "I approve" even though this does not truly capture the intent. Then you wind up with hacky work-arounds, like requiring a minimum number of approvals. |
|
