[Governance] Define a criteria in order to define when a PR should be aproved and when should be disaproved #124

fsologureng commented 2023年01月23日 16:23:40 +01:00

Considering that pull requests have started to increase in frequency and that proposals have started to be written by different people, ambiguity about the criteria for their moderation could be interpreted as arbitrary decision-making by the people involved for the longest time in the Forgejo initiative.

To solve this problem, I propose to define minimum criteria to be established in order to provide governance:

• For approval

1. All review comments were addressed and answered.
2. The person approving the PR agrees to be accountable for what it contains. They can be asked to explain and fix a problem that arise in the future as a consequence of the PR being merged.

• For disapproval

1. Unanswered arguments against or questioning the relevance of the proposal.
2. At least one review comment unresolved without arguments or changes in code.

Feel free to comment, edit, extend or reject this proposal.

Considering that pull requests have started to increase in frequency and that proposals have started to be written by different people, ambiguity about the criteria for their moderation could be interpreted as arbitrary decision-making by the people involved for the longest time in the Forgejo initiative. To solve this problem, I propose to define minimum criteria to be established in order to provide [governance](https://codeberg.org/forgejo/meta/issues/19): * For approval 1. All review comments were addressed and answered. 1. The person approving the PR agrees to be accountable for what it contains. They can be asked to explain and fix a problem that arise in the future as a consequence of the PR being merged. * For disapproval 1. Unanswered arguments against or questioning the relevance of the proposal. 1. At least one review comment unresolved without arguments or changes in code. Feel free to comment, edit, extend or reject this proposal.

Ghost commented 2023年01月23日 16:58:37 +01:00

A minimum number of passes.

Could you please explain what you mean by this?

Regarding this:

No disapproved that has not been solved with more arguments and/or commits.

I propose this is written as:

All review comments were addressed and answered.

> A minimum number of passes. Could you please explain what you mean by this? Regarding this: > No disapproved that has not been solved with more arguments and/or commits. I propose this is written as: > All review comments were addressed and answered.

fsologureng commented 2023年01月23日 17:04:22 +01:00

A minimum number of passes.

Could you please explain what you mean by this?

Regarding this:

No disapproved that has not been solved with more arguments and/or commits.

I propose this is written as:

All review comments were addressed and answered.

Issue edited to incorporate that suggestions.

> > A minimum number of passes. > > Could you please explain what you mean by this? > > Regarding this: > > > No disapproved that has not been solved with more arguments and/or commits. > > I propose this is written as: > > > All review comments were addressed and answered. > > Issue edited to incorporate that suggestions.

Ghost commented 2023年01月23日 17:33:38 +01:00

A minimum number of reviewer votes in favor of the proposal.

There are not that many reviewers so I would not be shocked if there is no minimum number of reviewers. In my experience the most important thing is to get quality reviews rather than a fixed minimum number of reviews. To that end I propose instead:

The person approving the PR agrees to be accountable for what it contains. They can be asked to explain and fix a problem that arise in the future as a consequence of the PR being merged.

Which creates a healthy situation where every PR has at least two person accountable for it. And there cannot be a situation where someone just blindly approves because they risk facing questions they cannot answer.

> A minimum number of reviewer votes in favor of the proposal. There are not that many reviewers so I would not be shocked if there is no minimum number of reviewers. In my experience the most important thing is to get quality reviews rather than a fixed minimum number of reviews. To that end I propose instead: > The person approving the PR agrees to be accountable for what it contains. They can be asked to explain and fix a problem that arise in the future as a consequence of the PR being merged. Which creates a healthy situation where every PR has at least **two** person accountable for it. And there cannot be a situation where someone just blindly approves because they risk facing questions they cannot answer.

Ghost commented 2023年01月23日 17:57:38 +01:00

Is there a need to explain explicitly why a PR is disapproved? Or is it enough to explain under which conditions it will be approved?

What happens usually is that a PR receives review comments suggesting changes. There can be also requests for changes that suggest this is not the right approach. In every case, if they addressed the PR will be merged. But if they are not addressed, the criterion for being approved will not be met and it will not be merged.

Do you see what I mean?

Is there a need to explain explicitly why a PR is disapproved? Or is it enough to explain under which conditions it will be approved? What happens usually is that a PR receives review comments suggesting changes. There can be also requests for changes that suggest this is not the right approach. In every case, if they addressed the PR will be merged. But if they are not addressed, the criterion for being approved will not be met and it will not be merged. Do you see what I mean?

fsologureng commented 2023年01月23日 19:49:34 +01:00

Is there a need to explain explicitly why a PR is disapproved? Or is it enough to explain under which conditions it will be approved?

What happens usually is that a PR receives review comments suggesting changes. There can be also requests for changes that suggest this is not the right approach. In every case, if they addressed the PR will be merged. But if they are not addressed, the criterion for being approved will not be met and it will not be merged.

Do you see what I mean?

Yes. The need I see is that insufficient approval votes are not reason enough to merge neither to deny the request because the community could be without available task force to do additional reviews. But in case a proponent didn't response a review or explicitly refuse to do it, should be sufficient to disapproval.

> Is there a need to explain explicitly why a PR is disapproved? Or is it enough to explain under which conditions it will be approved? > > What happens usually is that a PR receives review comments suggesting changes. There can be also requests for changes that suggest this is not the right approach. In every case, if they addressed the PR will be merged. But if they are not addressed, the criterion for being approved will not be met and it will not be merged. > > Do you see what I mean? Yes. The need I see is that insufficient approval votes are not reason enough to merge neither to deny the request because the community could be without available task force to do additional reviews. But in case a proponent didn't response a review or explicitly refuse to do it, should be sufficient to disapproval.

KaKi87 commented 2023年01月23日 23:24:24 +01:00

Actually I would like to suggest something much simpler than that : a PR is approved only if it implements an already approved issue, i.e. complies with its voted specifications.

And of course, complies with other eventual requirements e.g. code style, security practices, etc.

Actually I would like to suggest something much simpler than that : a PR is approved only **if it implements an already approved issue**, i.e. complies with its voted specifications. And of course, complies with other eventual requirements e.g. code style, security practices, etc.

crystal commented 2023年01月24日 05:25:43 +01:00

I think that's fine for large changes, but it adds unnecessary complications when attempting to submit fixes for minor issues that only require changing one or two lines of code.

Often when I encounter a bug, I will find the code that causes it and have it fixed before I even check if there's an existing issue.

I think that's fine for large changes, but it adds unnecessary complications when attempting to submit fixes for minor issues that only require changing one or two lines of code. Often when I encounter a bug, I will find the code that causes it and have it fixed before I even check if there's an existing issue.

Ghost commented 2023年01月24日 10:01:39 +01:00

I agree with @crystal on this.

I'd rather have minimal and solid criteria for merging a PR. This is something I think we can all agree is missing right now and creating some kind of uncertainty that needs to be addressed.

Improvements to this very minimal, very consensual basis can be discussed in a followup proposal. The more complicated the process for approving or disproving a PR, the longer it will take to reach consensus.

I'm not disputing the reasons for the improvements @KaKi87 and @fsologureng propose, they make sense to me. I'm suggesting to split this discussion into bite size issues that are easier to discuss individually.

Does that sound sensible?

I agree with @crystal on this. I'd rather have **minimal and solid criteria for merging a PR**. This is something I think we can all agree is missing right now and creating some kind of uncertainty that needs to be addressed. Improvements to this very minimal, very consensual basis can be discussed in a followup proposal. The more complicated the process for approving or disproving a PR, the longer it will take to reach consensus. I'm not disputing the reasons for the improvements @KaKi87 and @fsologureng propose, they make sense to me. I'm suggesting to split this discussion into bite size issues that are easier to discuss individually. Does that sound sensible?

KaKi87 commented 2023年01月24日 11:44:22 +01:00

Often when I encounter a bug, I will find the code that causes it and have it fixed before I even check if there's an existing issue.

If someone else has reported the issue you're trying to fix, then :

• you may miss details that the user spotted ;
• the user may never be notified that you tried to fix the issue.

I'm suggesting to split this discussion into bite size issues that are easier to discuss individually.

Alright.

> Often when I encounter a bug, I will find the code that causes it and have it fixed before I even check if there's an existing issue. If someone else has reported the issue you're trying to fix, then : - you may miss details that the user spotted ; - the user may never be notified that you tried to fix the issue. > I'm suggesting to split this discussion into bite size issues that are easier to discuss individually. Alright.

crystal commented 2023年01月24日 12:06:36 +01:00

If someone else has reported the issue you're trying to fix, then :

• you may miss details that the user spotted ;
• the user may never be notified that you tried to fix the issue.

I always search the issue tracker at some point to make sure I'm not missing anything, that's not the point here. The point is that if there isn't already an issue and I already have a fix, I shouldn't need to waste my time opening an issue and discussing it before I open my pull request.

> If someone else has reported the issue you're trying to fix, then : > - you may miss details that the user spotted ; > - the user may never be notified that you tried to fix the issue. I always search the issue tracker at some point to make sure I'm not missing anything, that's not the point here. The point is that if there isn't already an issue and I already have a fix, I shouldn't need to waste my time opening an issue and discussing it before I open my pull request.

KaKi87 commented 2023年01月24日 12:47:24 +01:00

if there isn't already an issue and I already have a fix, I shouldn't need to waste my time opening an issue and discussing it before I open my pull request

I understand.

Well, I think this is appropriate for maintainers because they have extensive understanding of a project, but not for contributors because they only know pieces here and there (until they do it so many times that they become qualified for the maintainer position of course).

> if there isn't already an issue and I already have a fix, I shouldn't need to waste my time opening an issue and discussing it before I open my pull request I understand. Well, I think this is appropriate for maintainers because they have extensive understanding of a project, but not for contributors because they only know pieces here and there (until they do it so many times that they become qualified for the maintainer position of course).

crystal commented 2023年01月24日 12:57:52 +01:00

I don't think there's a meaningful distinction in that case, as far as I'm concerned. It would obviously be inconvenient for a project maintainer to open both an issue and a pull request for every minor bug they discover and fix on their own, but especially in the case of something like forge software, I think any average user may also want to make such small changes to the codebase. As long as those changes can be quickly reviewed by people who do have more knowledge of the codebase, I think they should be allowed without an existing issue.

I don't think there's a meaningful distinction in that case, as far as I'm concerned. It would obviously be inconvenient for a project maintainer to open both an issue and a pull request for every minor bug they discover and fix on their own, but especially in the case of something like forge software, I think any average user may also want to make such small changes to the codebase. As long as those changes can be quickly reviewed by people who do have more knowledge of the codebase, I think they should be allowed without an existing issue.

fsologureng commented 2023年01月24日 13:51:20 +01:00

Hi,

I edited the issue incorporating proposal of @dachary in order to set accountability for the reviewer removing minimum number of reviewers.

@dachary I tend to think that we are pretty close to a minimal agreement, so I suggest to iterate once more and if we don't converge, open additional dependent issues.

I agree with @crystal and @dachary wrt unnecessary bureaucracy of always link a PR with an existing issue, but I think the concern of @KaKi87 (maybe I misinterpreting) is about keeping the big picture of the code. Forcing incorporation of a maintainer is useless because the merger should be one of them, so I assume reviewers always contain at least one maintainer with that big picture. Maybe adding some documentation wrt to modified code and not just comments in the PR?

How to prevent stalling PR I suggest to addressed in another issue.

Hi, I edited the issue incorporating proposal of @dachary in order to set accountability for the reviewer removing minimum number of reviewers. @dachary I tend to think that we are pretty close to a minimal agreement, so I suggest to iterate once more and if we don't converge, open additional dependent issues. I agree with @crystal and @dachary wrt unnecessary bureaucracy of always link a PR with an existing issue, but I think the concern of @KaKi87 (maybe I misinterpreting) is about keeping the big picture of the code. Forcing incorporation of a maintainer is useless because the merger should be one of them, so I assume reviewers always contain at least one maintainer with that big picture. Maybe adding some documentation wrt to modified code and not just comments in the PR? How to prevent stalling PR I suggest to addressed in another issue.

KaKi87 commented 2023年01月24日 19:16:06 +01:00

Actually, my concern is that a PR author makes a decision at implementation time that may be improper and therefore would require them to rewrite those, thus having wasted time.

Meanwhile, mandatory issue discussion would have avoided that because decisions would have been voted prior to PR.

This could indeed happen less frequently for fixes than features, but it still can.

Actually, my concern is that a PR author makes a decision at implementation time that may be improper and therefore would require them to rewrite those, thus having wasted time. Meanwhile, mandatory issue discussion would have avoided that because decisions would have been voted prior to PR. This could indeed happen less frequently for fixes than features, but it still can.

crystal commented 2023年01月24日 21:46:10 +01:00

Actually, my concern is that a PR author makes a decision at implementation time that may be improper and therefore would require them to rewrite those, thus having wasted time.

I don't understand how this relates to small changes that only change a couple lines of code. I'm totally fine with there being more stringent requirements for large additions or refactorings.

Meanwhile, mandatory issue discussion would have avoided that because decisions would have been voted prior to PR.

I think these issues can be discussed in the comments of a pull request if needed. Even if a portion of the code needs to be reworked and discussed, I still think having the original code there to use as a springboard for discussion and improvements will allow for more collaborative and creative solutions to problems. This process could even be helpful for newer contributors, allowing them to quickly get a feeling for the preferred coding practices of the project during these discussions. Even if 70% of what they wrote ends up getting refactored away, I would still not necessarily consider this "wasted time" (unless their idea of "code" is dozens of nested if statements or some other horrible thing like that)

> Actually, my concern is that a PR author makes a decision at implementation time that may be improper and therefore would require them to rewrite those, thus having wasted time. I don't understand how this relates to small changes that only change a couple lines of code. I'm totally fine with there being more stringent requirements for large additions or refactorings. > Meanwhile, mandatory issue discussion would have avoided that because decisions would have been voted prior to PR. I think these issues can be discussed in the comments of a pull request if needed. Even if a portion of the code needs to be reworked and discussed, I still think having the original code there to use as a springboard for discussion and improvements will allow for more collaborative and creative solutions to problems. This process could even be helpful for newer contributors, allowing them to quickly get a feeling for the preferred coding practices of the project during these discussions. Even if 70% of what they wrote ends up getting refactored away, I would still not necessarily consider this "wasted time" (unless their idea of "code" is dozens of nested if statements or some other horrible thing like that)

Ghost commented 2023年01月24日 21:50:48 +01:00

@fsologureng I think For approval is good as it stands 👍

I'm still unsure about the necessity of For disapproval and the need for an issue associated with a PR. There are pros and cons that deserve more discussions. Open followup issues for each of them would be useful, IMHO.

@fsologureng I think **For approval** is good as it stands 👍 I'm still unsure about the necessity of **For disapproval** and the need for an issue associated with a PR. There are pros and cons that deserve more discussions. Open followup issues for each of them would be useful, IMHO.

fsologureng commented 2023年01月25日 14:03:54 +01:00

Ok. I will close and do a PR for this agreement, and will open up two new issues:

1. About relations between PR and issues, to address and ease little contributions (as I understand is @crystal concern), and to avoid large feature PR out with improper design (which I understand is @KaKi87 concern). I think we need a balance between the two. I would add large feature PR which are not part of the Forgejo bootstrap.

2. Disapproval rule need

If there are corrections to do to my interpretation, please do it in the new issues.

All of this I will do as soon as I can.

Ok. I will close and do a PR for this agreement, and will open up two new issues: 1) About relations between PR and issues, to address and ease little contributions (as I understand is @crystal concern), and to avoid large feature PR out with improper design (which I understand is @KaKi87 concern). I think we need a balance between the two. I would add large feature PR which are not part of the Forgejo bootstrap. 2) Disapproval rule need If there are corrections to do to my interpretation, please do it in the new issues. All of this I will do as soon as I can.