Submit approval as “commented” instead of “requested changes”

[jotaen4tinypilot]

When approving a PR from CodeApprove, it appears as “requested changes” on Github. That looks a bit odd to me, because both statements contradict themselves.

Would it be possible to submit the CodeApproval review as “commented” instead? I think Github allows that as review category as well.

[samatcodeapprove]

@jotaen4tinypilot yeah this definitely needs improvement. I do think "Request Changes" is the correct model here because the idea of CodeApprove is for any unresolved conversations to block submission. CodeApprove automatically handles approving the review when that condition changes though.

However the email for "approved with comments" definitely shouldn't look like that! It should be much clearer to all parties what is going on.

I'll try and improve the communication. If you still find it confusing I am open to making "commented vs request changes" a team-level setting but I am hesitant to go straight there.

[jotaen4tinypilot]

I do think "Request Changes" is the correct model here because the idea of CodeApprove is for any unresolved conversations to block submission.

Wouldn’t a mere comment also block submission? I always thought unless you explicitly approve, merging continues to be blocked on Github’s end.

I often have the use case that I just have a question that I want the PR author to address, e.g. to make sure that they have considered something, but I don’t necessarily want to require changes in the code. On Github, that shows up as “requested changes” then, which doesn’t really feel correct in that situation. I agree, however, that the semantics might be tricky to get right in general.

[samatcodeapprove]

@jotaen4tinypilot sorry for the late response here but this matters a lot more with multi-reviewer code reviews. In that situation, you need "request changes" to allow one reviewer to block the PR. Otherwise GitHub will show a PR with one approval and one approved-but-unresolved reviewer as ready to merge.

[samatcodeapprove]

@jotaen4tinypilot I am updating the messaging here to be clearer:

I hope you agree that it's an improvement! I am going to continue to use "Request Changes" for unresolved comments for now but if I continue to hear this feedback from other users I will reconsider.

[jotaen4tinypilot]

Thanks for addressing this. I agree with your reasoning, and I think it’s tricky to find the perfect solution here. I think your improved message will already help first-time users of CodeApprove to understand the intended flow.