[AGREEMENT] Added first version agreement for PR moderation #129

Gusted commented 2023年02月02日 10:47:55 +01:00

They can be asked to explain and fix a problem that arise in the future as a consequence of the PR being merged.

This is quite a statement; I'm not so sure about this. This would mean that approving is equal to writing, "I couldn't have done this PR myself better, and this looks fantastic in my eyes." With complex pull requests or very careful pieces of code(cryptographic bits for example), I wouldn't assume the reviewers know every detail that went in to it.

In my opinion, the main responsibility lies with the one who made the pull request to maintain it, or the one who "maintains" the feature branch (this is currently undefined).

> They can be asked to explain and fix a problem that arise in the future as a consequence of the PR being merged. This is quite a statement; I'm not so sure about this. This would mean that approving is equal to writing, "I couldn't have done this PR myself better, and this looks fantastic in my eyes." With complex pull requests or very careful pieces of code(cryptographic bits for example), I wouldn't assume the reviewers know every detail that went in to it. In my opinion, the main responsibility lies with the one who made the pull request to maintain it, or the one who "maintains" the feature branch (this is currently undefined).

Ghost commented 2023年02月02日 11:23:28 +01:00

I strongly believe code must not be merged unless there are at least two people who fully understand every details.

There may be rare exceptions where the complexity is such that details are bound to elude the human reviewers. But if there is a suspicion of that level of complexity, it will need to be associated with tests that compensate for that and methodically explore the many border cases that would escape the human brain.

In the most common case though the situation will be that someone submits a piece of code but has not commitment to maintain it, they are the occasional contributor. Whoever reviews their contribution and is a regular contributor to Forgejo is going to be ultimately responsible for maintaining it. They need to understand every aspect of what they merge better than the author.

I think the case you are referring to is when two regular contributors to Forgejo are involved and trust each other. In that case the level of attention of the review does not need to be as high as with an external contributor. But it needs to be at least able to be sure there is nothing in the pull request that escapes their understanding.

I strongly believe code must **not** be merged unless there are at least two people who fully understand every details. There may be rare exceptions where the complexity is such that details are bound to elude the human reviewers. But if there is a suspicion of that level of complexity, it will need to be associated with tests that compensate for that and methodically explore the many border cases that would escape the human brain. In the most common case though the situation will be that someone submits a piece of code but has not commitment to maintain it, they are the occasional contributor. Whoever reviews their contribution and is a regular contributor to Forgejo is going to be ultimately responsible for maintaining it. They need to understand every aspect of what they merge better than the author. I think the case you are referring to is when two regular contributors to Forgejo are involved and trust each other. In that case the level of attention of the review does not need to be as high as with an external contributor. But it needs to be at least able to be sure there is nothing in the pull request that escapes their understanding.

fsologureng commented 2023年02月02日 22:33:20 +01:00

I agree with @dachary here, is like the value behind pair programming, both proponent and reviewer have to share knowledge about the new code. That shares is always good for maintenance.

I agree with @dachary here, is like the value behind pair programming, both proponent and reviewer have to share knowledge about the new code. That shares is always good for maintenance.

Gusted commented 2023年02月03日 14:02:59 +01:00

I was indeed referring to the scenario you described, as that will be 80% of the case (likely) in which this rule is a bit inefficient when you're suddenly hold accountable for such things.

I was indeed referring to the scenario you described, as that will be 80% of the case (likely) in which this rule is a bit _inefficient_ when you're suddenly hold accountable for such things.

Ghost commented 2023年02月03日 15:58:07 +01:00

@Gusted what kind of accountability would you find more efficient as a reviewer of a PR?

@Gusted what kind of accountability would you find more efficient as a reviewer of a PR?

Gusted commented 2023年02月04日 00:08:09 +01:00

Well, there's no general rule of thumb in our case. We either go with this rule and each contributor is accountable for the code that they approve or we could go with each feature branch has a contributor that oversees and approves code for their feature branch.

I think the latter is more effective, as I happily approve a PR for the sake of the Go/Javascript code being correct, efficient, safe and idiomatic -- but not necessary for the behavior change, that's up to the "experts". In the first case, I also have to understand why the behavior change is necessary (and potentially read a backlog of discussions on a topic I have little knowledge in). In the latter case, I can approve a PR to say that the code is good and does what it says it does without having to be accountable why that change was necessary (and representing a opinion that was formed after a discussion).

We currently don't really have a setup such that the latter case can exist and I think it would be quite hard to achieve it currently.

Well, there's no general rule of thumb in our case. We either go with this rule and each contributor is accountable for the code that they approve or we could go with each feature branch has a contributor that oversees and approves code for their feature branch. I think the latter is more effective, as I happily approve a PR for the sake of the Go/Javascript code being correct, efficient, safe and idiomatic -- but not necessary for the behavior change, that's up to the "experts". In the first case, I also have to understand **why** the behavior change is necessary (and potentially read a backlog of discussions on a topic I have little knowledge in). In the latter case, I can approve a PR to say that the code is good and does what it says it does without having to be accountable why that change was necessary (and representing a opinion that was formed after a discussion). We currently don't really have a setup such that the latter case can exist and I think it would be quite hard to achieve it currently.

Gusted commented 2023年02月05日 20:39:13 +01:00

Going to mark this resolved, the current setup wouldn't allow for any other situation than currently is described.

Going to mark this resolved, the current setup wouldn't allow for any other situation than currently is described.