Adjust and relax review process#9
Merged
sciwhiz12 merged 2 commits intoParchmentMC:mainfrom Jun 19, 2022
Merged
Conversation
Notable changes include: - Abolishment of any specific time delays between PR approval and its merging, or for dismissing a stale change requesting review. - Reduction of required reviews to a single approving review from a member of the reviewers sub-team. - Moving the burden of delays from required time delays between PR approval and merging to be within the discretion of the mappings team.
There will likely never be a reason for the invocation of this section. Even so, the point on delaying the merging of the PR may be applied in such situations.
alcatrazEscapee
approved these changes
Jun 18, 2022
ChampionAsh5357
approved these changes
Jun 18, 2022
Lanse505
approved these changes
Jun 18, 2022
SizableShrimp
approved these changes
Jun 18, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following the recent internal meeting and discussion with the @ParchmentMC/mapping-standards team, the PR review process has been relaxed considerably for the benefit of contributors and the mappings team.
Any references to specific time delays for PR approval and merging and for stale review dismissal have been removed. It is now up to the discretion of the individual mappings team members on the timeframe to merge approved PRs or dismiss stale reviews.
Notably, this means that PRs may now be merged immediately after its approval (provided that the merging mappings team member does not decide to delay the PR's merging) -- the previous restriction of the 48/24 hour mandatory waiting time has been abolished.
Most if not all PRs have never made use of the mandatory waiting time between PR approval and merging, leaving it as an ineffectual waste of time. This removes that waiting period, reducing the bureaucratic overhead of counting down the clock on PRs.1
The requirements for PR approval has been relaxed from two approving reviews, one of which must be from a member of the reviewers sub-team, and stricly zero change-requesting reivews to one approving review from the reviewers sub-team and a broad stipulation that there are "no pending changes from reviews".
Most PRs are small in size and do not necessarily need the counterapproval of two reviewers. Additionally, due to the small size of the pool of contributors who review PRs, it is often the case that the two reviewers are members of the reviewers sub-team.
The section on fixed discussion of PRs has been removed entirely. It has been effectively superseded by a new clause which grants the discretion of delaying a PR's merge to the mappings team.
There will likely never be a case in which a PR gains so much discussion and/or controversy that the fixed discussion section will be invoked. Removing this section and replacing it with a broader discretionary power to delay PR merging grants us greater flexibility and more closely matches the actual flow of review and merging in practice.
Footnotes
This also means that the PR Review project in the mappings repository will be closed and retired. ↩