Create PR-Review process

[marchermans]

Introduction

After internal discussions on the 22nd of December, 2023, it was clear that we should have a semi-formalized process for creating and reviewing PRs. This pull request provides such a procedure.

Design goals:

• Address the concern of existing members that new members are overriding their opinion
• Address the concern of new members that existing members can not review pull requests in time due to their availability
• Address the concern of members that pull requests are not documented properly
• Address the scope of reviews and when they can be dismissed.

None goals:

• Create a pull request pattern, this should be done on a project level.

[TelepathicGrunt]

The doc here seem a bit too strict. Imo, I think a better balance is:

• PRs should sit for 24 hours since initial creation before they can be merged. If it is a critical issue such as a crash fix, approvals from two other maintainers that didn't make PR can override the time limit. This will be handled on a case-by-case basis. Use maintainer ping if needing approvals. There should be enough people that can hop on phone, read up on what is happening, and approve. 24 hours allows anyone in any time zone to have an opportunity to read PR and say their concerns or approve it. This is respectful of people and their free time.

• Reviews left on PR that as not prefixed with "Passing remark" will initiate a 24 hour waiting period on PR to give time for PR maker to answer the comment/fix the remark. This gives time to the reviewer to actually read the response/new work and say if their concerns are resolved or not. Generally, people are going to reply earlier than 24 hours but in the case it does exceed the limit, PR maker can ask another maintainer to read the original review and the responses/fixes to see if it did resolve issue. And if that maintainer approves PR, then PR can be merged despite the review not closed by original reviewer within 24 hours.

• PR titles and descriptions should be clear and easy for us to look back on in months and know what the PR was for and why. Description should be at least 1 sentence minimum. Larger PRs would be expected to have larger descriptions detailing what is going on and why. This would be handled case-by-case basis. If one feels a PR description is too short for a gigantic PR that has a huge number of changes, they can leave a comment asking for PR maker to expand the description and that's it. No issue. Basically, maintainer PRs should be held to the same standard we would hold non-maintainer PRs to. We wouldn’t merge a random modder’s PR that has no description and no reasoning for being made. Therefore, maintainers should face same scrutiny in order to keep quality PRs rather than degrading to code churn lousy and obtuse PRs.

• Breaking change PRs all must list what the breaking change is, why it is decided to be broken, and what the workaround is. Modders will be looking for the PR to know what the workaround is. They won't be combing the changelog nor Discord. They are going to do a Blame in GitHub or a keyword search for the PR and will be reading the PR body for the workaround. Lack of info in a breaking change PR body is a jerk move. Full stop. NeoForge can do better and not only make changes that help modders but also document the changes in ways that makes it easier for modders.