add Reject as an option to pull request reviews

[pabigot]

Currently the only negative review option is "Request Changes". Some PRs take a technical approach that is simply wrong and no changes less than a completely different solution would be acceptable.

#15545 is one of those. So was #11880, which if it had been firmly rejected as a solution path, rather than leaving open the possibility it could be adjusted to an acceptable state, would have reduced the amount of time wasted trying to reach consensus.

Adding a "Reject" option that requires an explanation would provide an opportunity for area maintainers to make a decision on technical merits and either overrule the rejection or accept it and close the PR, rather than leaving it in limbo for an arbitrary duration.

Rejection with "Contributor has not made requested changes and nobody else cares" could also be useful in cleanup. (410 open PRs at the moment; there's no way they're all going to be eventually updated and merged.)

[aescolar]

@pabigot The Approve/Comment/"Request changes" review choices are given by GitHub. It's not something this project can change.

Anyhow, you can always give a "Request changes" review explaining that it is indeed simply the wrong way, and no marginal change will be enough. Quite a few PRs have been closed after people disagreed on the overall principle.
It seem to me what you are after is reviewers being more direct in their way of communicating when they want to reject something, that would not really require a separate review choice button.
But, be aware that some PRs target areas with too weak ownership, were no reviewer feels they have the authority to fully reject (or approve) it. In these cases, PRs which are seen as contentious, may never receive an approving review.

"Contributor has not made requested changes and nobody else cares" is today marked with "stale PR", and there is a few in that state, which are eventually closed if there is no reaction by the author.

[pabigot]

@aescolar

The Approve/Comment/"Request changes" review choices are given by GitHub. It's not something this project can change.

Well, that's annoying. OK then.

It seem to me what you are after is reviewers being more direct in their way of communicating when they want to reject something, that would not really require a separate review choice button.
But, be aware that some PRs target areas with too weak ownership, were no reviewer feels they have the authority to fully reject (or approve) it. In these cases, PRs which are seen as contentious, may never receive an approving review.

I'm really looking for ways of decreasing the amount of time a contributor is left hanging when reviewers don't reach consensus. Even if there are several approving reviews, one "Changes Requested" can block progress. The absence of oversight, though, is indeed hard to work around.

I'll leave this open in case somebody can suggest a viable process improvement approach, but if nobody does then somebody can close it.

[Laczen]

@pabigot, I agree with your concerns, I stopped contributing to zephyr exactly because of this.

I have been thinking about possible improvements to the review process and I have come up with the following proposal:

Replace the code-owner idea with a code-sponsor: The code sponsor is assigned with the task of
getting a PR merged. When a new PR is submitted the code sponsor decides (possibly with help of
others, if required) if the idea is good to be merged. Possibly this will take some communication between the submitter and the sponsor.

If the idea is not good the code sponsor convinces the submitter to remove the PR (#15545 is a good example of how to do that).

If the idea is good the code review starts and the code sponsor helps in getting the PR approved as fast as possible taking into account the expected code quality. The code sponsor is responsible for removing any reviews that leave the PR in a "stall" state. The code sponsor follows the idea: "Whenever an idea is worth doing it is worth doing fast".

Working this way should fasten the approval/merging of PR's and would not leave a PR in some undetermined state where no one approves the PR because someone else has put a "request changes".

[ulfalizer]

Couldn't you just say "I'm sorry, but I think the basic approach here is wrong, so I won't approve it"?

Feels like people fall back on templates and stuff instead of just talking to each other too often in IT. :P

[mbolivar]

Couldn't you just say "I'm sorry, but I think the basic approach here is wrong, so I won't approve it"?

@ulfalizer:

This leaves open the problem that the only way to surely block a change from being merged is with the Request Changes state. Which we can't improve upon.

Edit: We could try to resort to things like a bot that warns-about-then-closes PRs in that state if they don't see any activity from their authors. But this can in turn be annoying if the author is actively submitting other PRs that pave the way for the main one in response to changes requested by reviewers. I suppose they could always resubmit again once those others were in, but it's nice to have an open PR to point to and say "this ultimate goal is why these changes are being made". So the devil would be in the details here.

The code sponsor follows the idea: "Whenever an idea is worth doing it is worth doing fast".

@Laczen:

I don't think this is part of Zephyr's philosophy. Some things that are worth doing take time. This is often the case in open source projects that support a wide variety of hardware architectures, configurations, host operating systems, etc.

So I think this proposal is founded on unsound premises for the Zephyr project.

[mbolivar]

@pabigot I think this could use TSC attention. It's a symptom of the ubiquitous problems of divergent priorities and more people writing code than are able to review it, but "process for NAKing PRs" seems like low hanging fruit.

• I've suggested that the TSC consider this at the next meeting by adding a request for an open item in the next agenda in the minutes doc, https://docs.google.com/document/d/12p8Q4USdExsP3a8lIdhyqG-4vxb44kisccZ9TvlxWBA/edit#
• I've also added the TSC label to this issue.

[aescolar]

This issue is digressing into a container of all kind of somehow related frustrations and proposals.
I think this is not going to help anybody, so I'd request people to stop commenting/answering here. It is simply not the right place or format for this. Also this may give people the impression that commenting here is going to reach the right ears. (Long threads with all kind of unrelated topics and discussions tend to be just ignored)

I'm really looking for ways of decreasing the amount of time a contributor is left hanging when reviewers don't reach consensus.

@pabigot : This is something quite a few of us are very aware of, and something we would like to fix. @Thea-Aldrich between others.

@Laczen Your proposal misses a few points.
CODEOWNERS today would be a tool to ensure the owners of an area get notified when a PR targets that area, and to ensure it is clearly indicated in the review who is responsible for that area. A code owner would be responsible to ensure that code quality and coherence is maintained in that area, and that in general the overall project guidelines and processes are followed. So codeowners does fulfill a purpose, and it is not to be replaced by something unrelated.

@Laczen What you propose about a "code sponsor" has been discussed some time ago, and has not been implemented yet, between other things, because it ignores some realities of the project. The most important one being that in some areas there is more than enough engaged maintainers and activity for this to be a non issue, while in others there is not enough interest to man that role. And this is a role that takes a lot of time.

The code sponsor follows the idea: "Whenever an idea is worth doing it is worth doing fast".

@Laczen as @mbolivar said, it is certainly not the priority of the project to get code in as fast as possible. I can certainly understand that as a contributor, any of us, would like attention to our PRs by others who are knowledgeable about the area, who would provide quick, constructive feedback and would help us to mature and merge our PRs as soon as humanly possible. But the reality is that the main contributors who can provide useful feedback tend to be very busy with a multitude of other issues, many PRs take a horribly long amount of time and iterations from those maintainers. And some of those PRs, even if of great importance for the author who wants them merged, may be considered less of a priority by the maintainers compared with all that is going on.
In some areas, PRs are unfortunately going to be "one PR more" in a multitude of PRs with not enough maintainers.

but "process for NAKing PRs" seems like low hanging fruit.

@mbolivar, @pabigot , That is a fruit that has already been picked.
The "request changes" review option name is just an euphemism. It is for all practical purposes a "Rejected" review outcome. The part where that option title may communicate badly to the author can be solved by the reviewer stating it clearly, and if wanting to be more (too) direct also by the maintainer closing the PR. (Though contributors may easily take offense if their PR is summarily closed)
Or, what else would you think is needed as a process for how to "reject PRs"?

[mbolivar]

I'd request people to stop commenting/answering here. It is simply not the right place or format for this.

Are you saying you don't think this particular issue is the right place for a discussion, or the issues page in general? Do you have a counterproposal location? Also since you've specifically asked me a question here, I'm going to reply despite this request that nobody comment here ;).

The "request changes" review option name is just an euphemism. It is for all practical purposes a "Rejected" review outcome.

I'm sorry, but this is simply not true.

There is a big difference between "I have some issues that, if addressed, would make me willing to merge this PR" (which is how we normally use "Request Changes" -- it's even in the name of the state) and "as subsystem maintainer, this approach is fundamentally flawed and I will never approve it for merge" (which is what NAKing is).

The part where that option title may communicate badly to the author can be solved by the reviewer stating it clearly, and if wanting to be more (too) direct also by the maintainer closing the PR. (Though contributors may easily take offense if their PR is summarily closed)
Or, what else would you think is needed as a process for how to "reject PRs"?

The original request was to add a NAK state, which you've already said we cannot do because GitHub owns this.

I do think that "comment and close" from the level 1 code owner (first person in the list of owners in the CODEOWNERS file's entry for a given area) would be a good way to signal NAK rather than "not quite". We generally speaking don't do this. And it seems like low hanging fruit to discuss adding this to our process.

That would not have "already been picked" -- I've been maintaining minor things for a while in Zephyr and I know for a fact this is not a common policy which has been discussed and agreed upon.

[pabigot]

TSC re-check: not of interest, use existing process.