[dust-jacket]

> PR approval is too boolean. The PR is approved or it's not approved. Real code review, like real life, lives in the middle

This is have-your-cake-and-eat-it. PR approval is a permission so is a boolean. Of course it is. Either the code can be merged or it can't.

What's being described really here is just something to make you feel slightly better about yourself whilst approving code you hate ("we should revisit this..."). Just open a new ticket.

[ahartmetz]

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

[wccrawford]

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]

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.

[m463]

I haven't used this kind of +/-2 but I think it might be good communication between reviewers.

sometimes I review something and say "approved", but sometimes I can only review part of it, and really need someone else to check what's out of my wheelhouse.

sort of "partially approved".

I have used systems that can set things like "requires 2 reviewers" or "bob, fred are required reviewers, elon and sam are optional reviewers".

also we had "thumbs up, thumbs down, and some comments might have a "task" associated with them as a required fix before approval"

optionally, maybe before you say "approved" you have an overall comment, and see the comments of other reviewers.

[kps]

> 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]

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?????

[thcipriani]

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-...>

[chii]

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]

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]

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]

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]

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".

[thayne]

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.

[sandeepkd]

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]

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]

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.

[xmodem]

I just want a button that says "approve and merge these 3 commits now but these two need re-work"

[jftuga]

I like to rebase/squash before pushing because it keeps the commit history cleaner. However, I do like your idea so I guess I could also do a squash/merge after approval (which I already do, anyway).

[zzo38computer]

There is another case, which is that the code is approved but not merged (in which case the maintainer may apply it manually (with changes), and might list the author of the PR as a co-author of the commit that applies those changes).

[cpan22]

Well I think the bigger issue here is that GitHub is too decoupled from an issue tracking system - its a lot of manual overhead to constantly have to keep in sync. Linear does an ok job but far from ideal

[BigTTYGothGF]

> Either the code can be merged or it can't.

Not an intuitionist, I see.

[pdpi]

If you are an intuitionist, excluded middle isn't an axiom, but is still provable/assertable on a case-by-case basis. This is a scenario where asserting it is entirely reasonable.

[kps]

The whole point of a VCS is that your code exists in a superposition of merged and unmerged.

[tanh]

This exists in Azure DevOps as Approve with Suggestions

[jftuga]

I think this is a good idea as it happens anyways even in GitHub.

[hmokiguess]

I mean, that’s fair no? If the UX creates an impasse for the user then this leads to friction in the process. There’s more than one way to address it. One is the user overcomes his own internal dilemma, the other is the UX helps him get there. For example, would be cool if there was a way to do a conditional approval with an issue tied to a stacked PR or something similar (just throwing ideas, point is to surface up the friction as a UX take not a protocol or API issue with git)

[ferngodfather]

There is also "this code is functionally fine but I absolutely hate it"