diff --git a/src/en/general-development/codebase-info/pull-request-guidelines.md b/src/en/general-development/codebase-info/pull-request-guidelines.md index b3c45dc8a..bc75a3f40 100644 --- a/src/en/general-development/codebase-info/pull-request-guidelines.md +++ b/src/en/general-development/codebase-info/pull-request-guidelines.md @@ -25,6 +25,8 @@ Thank you for contributing to Space Station 14. When submitting pull requests (P - **Do not make multiple unrelated changes in one PR.** For example, do not make miscellaneous additional changes to a PR, e.g. changing the heat resistance of a pair of gloves alongside your PR adding a new gun. - Try to split your PR into smaller ones where it makes sense to do so. This makes it significantly easier to read and can lead to faster reviews. It's also usually easier for you, and means you will receive earlier feedback and can avoid spending time making changes that have to be reworked. + +- **Start each PR with a short summary of what your PR does in simple terms, and if it's a gameplay PR link to design documents if applicable.** Being able to quickly see what your PR intends to do and how it relates to established design documents makes for faster triages and reviews. When submitting a small change, or when there is no existing design document that is applicable, the design may instead be outlined directly in the PR description. ## Testing diff --git a/src/en/wizden-staff/maintainer/review-procedure.md b/src/en/wizden-staff/maintainer/review-procedure.md index e0b466f80..9c30b77ea 100644 --- a/src/en/wizden-staff/maintainer/review-procedure.md +++ b/src/en/wizden-staff/maintainer/review-procedure.md @@ -1,23 +1,37 @@ # PR Review Procedure -This does not apply to rule change or server config change PRs from the Head Game Admins. In an emergency these requirements may be waived with written permission from a project manager. +This does not apply to rule change or server config change PRs from the Head Game Admins. In an emergency these requirements may be waived with written permission from a Project Manager (either on Github, Discord or the SS14 forums). Not following this procedure/policy will result in disiplinary action being taken. ## Requirements -- All PRs *should* adhere to the [code conventions](../../general-development/codebase-info/conventions.md), but this is not a hard requirement since some specific usecases may require straying from convention for readability. -- PRs must be properly tagged with the relevant department/game areas. -- PRs affecting gameplay must reference a design doc/proposal. For smaller changes, the design may be outlined in the PR description. All gameplay PRs must not conflict with the core game design and should ideally reference how they fit into it. -- PRs primarily fixing bugs must be tagged with the "Bug Fix" tag. -- **2 Maintainers are required to review/sign off on merging or closing a PR**. *This requirement may be waived with written permission from a PM when the situation warrants it.* -- PRs that have more than 3 weeks of inactivity may be closed as "dormant" by a single maintainer if an attempt has been made to contact the author. +- All PRs **should** adhere to the [code conventions](../../general-development/codebase-info/conventions.md), but this is *not* a hard requirement since some specific usecases may require straying from convention for readability. +- PRs **should** be triaged, as per the [triage procedure](triage-procedure.md) guidelines. If a PR is not triaged, this can be done at the same time as the PR is initially considered for review. +- PRs affecting gameplay **should** reference a design doc/proposal if one exists. For smaller changes, or where design documentation does not exist yet, the design may instead be outlined in the PR description. All gameplay PRs must not conflict with the [core design principles](../../space-station-14/core-design/design-principles.md) and should ideally reference how they fit into it. The responsibility to include this is on the PR author. +- **2 Maintainers are required to review/sign off on merging or closing a PR**. *This requirement may be waived with written permission from a PM when the situation warrants it (either on Github, Discord or the SS14 forums).* + - For merging, this takes the form of 2 "Approval" checkmarks from Maintainers on the PR (visible in the PR's comments or under "Reviewers" in the Github sidebar). This Approval can also be given in written form during discussions on other platforms such as Discord or the SS14 forums, as long as it is also stated by another Maintainer in a comment under the PR. If the PR author is a Maintainer, this automatically counts as the PR having 1 Approval. *An "Approval" checkmark implies that you have reviewed and approved the PR's contents (code, art, mapping change etc.). If you approve of the PR conceptually without reviewing it this can be written in a PR comment, but does not count as an Approval. The label "S: Conceptual Approval" is reserved for Maintainer Discussions and may not be used in this instance.* + - For closing, this takes the form of 2 Maintainers expressing an opinion to close the PR, or a reluctance to implement the PR, in the PR's comments. This "Disapproval" can also be given in written form during discussions on other platforms such as Discord or the SS14 forums, as long as it is also stated by another Maintainer in a comment under the PR. + - In the case where there's dissent among maintainers, e.g. 2 Approvals and 1 Disapproval, a Maintainer Discussion should be held as described under the Policy section. + - You do not need to be one of the two Maintainers who have given Approval/Disapproval to merge/close the PR. Ideally a PR should be processed once it meets the requirements to be merged/closed, but if that does not happen for some reason, any other Maintainer is free to do it (even if that Maintainer is the PR author). +- PRs that have more than 3 weeks of inactivity waiting on something from the PR author should be considered "stale" and labeled as such with the `S: Stale` label, unless another reason explains the inactivity (e.g. freeze or author has informed about a longer break). You may use any means to contact the author of the PR, but at the minimum you should leave a comment on the PR and give at least 1 week to respond before closing a stale PR. A PR that is waiting on an action from Maintainers (e.g. a code review or discussion resolution) is not eligible to be marked as stale. ## Policy -- When reviewing a PR assign it to yourself to let others know that you will be "owning" the PR. If you decide to stop owning the PR, un-assign yourself so that others may take over the PR. By owning a PR you are taking responsibility for keeping track of the PR's progress and keeping the PR author informed of what is the status of the review process. Anyone may review an owned PR, however before taking action talk with the owner first. *If changes are approved by the "owner" any maintainer may merge the PR if they also approve the changes.* +- When starting a new review for a PR that has not yet been assigned to a Maintainer, you are **required** to Assign yourself to the PR. This can be done under "Assignees" in the Github sidebar. This let others know that you will be "owning" the PR. If you decide to stop owning the PR, un-assign yourself so that others may take over the PR. By owning a PR you are taking responsibility for keeping track of the PR's progress, ensuring code quality and keeping the PR author informed of what is the status of the review process. If you are the PR author yourself, you may not also be the PR owner. Anyone may review an owned PR. *If changes are Approved/Disapproved by the PR owner any Maintainer may merge the PR if they also Approve/Disapprove the changes, provided there is no other Maintainer dissent.* -- If you think a PR warrants further discussion with maintainers, you may **optionally** create a review thread in maint-reviews. *This is not a requirement*, but is recommended when you aren't sure about something. If you do this, make sure to tag the PR with the "In Discussion" tag, and summarize the result of the discussion in the PR *before* taking any action(Such as merging or closing). The discussion thread should have the PR number in the title, and a link to the PR included within. You should also be appropriately tagging the discussion thread with the relevant work groups. Any maintainer may do this, not just the PR owner but make sure to check to not make a duplicate. +- If you think a PR warrants further discussion with maintainers, or there is dissent regarding merging/closing, you may **optionally** start a Maintainer Discussion. **This is not a requirement**, but is recommended when you aren't sure about something. If you choose to do this, you are **required** to tag the PR with the "S: Under Maintainer Discussion" label, which should automatically cause a thread with the PR's name, PR number, Github link and appropriate tags/pings to be created on Discord in the #maint-review channel. If this does not happen automatically (e.g. the Discord bot is down or otherwise unavailable), you should create the thread manually. Any maintainer may do this, not just the PR owner but make sure to check to not make a duplicate. Once discussion has concluded, summarize the result of the discussion in a PR comment *before* taking any action. Then, remove the "S: Under Maintainer Discussion" label. Closing or merging should automatically apply appropriate tags and title prefixes to the discussion thread (e.g. [Approved], [Merged], [Closed] etc.), as well as close it. + - If the result of a Maintainer Discussion is positive and the PR has not undergone code review (meaning it is not yet ready to merge), the PR may be given the label "S: Conceptual Approval", to show that it has been discussed with a positive outcome. + - If the result of a Maintainer Discussion is negative (i.e. a desire to close the PR), the result of the discussion can be cited as the reason to close the PR without explicitly requiring 2 Disapprovals. -- All non-gamebreaking revert PRs *must* undergo a maintainer discussion before being created. If there is a PR that you would like to be reverted, please *create a thread in maint-reviews with the format REVERT-PRNumber and ping the relevant work group maintainers.* Discussions should reach a resolution within 48 hours, if discussions have stalled notify the Game Director or a PM. +- If a Maintainer Discussion thread has been inactive for a week with no new messages, the Discord bot will tag the PR owner and prompt for a resolution. What this resolution is depends on the discussion, but Maintainers are encouraged to arrive at a compromise to either give the PR conceptual approval, agree to close the PR, or in rare cases ask the PR to be set to Draft in case a resolution can not be reached for some unique reason related to the PR. Other actions may also be taken. In the case the Discord bot fails, Maintainers are encouraged to do the prompting. + +- All non-gamebreaking revert PRs are **required** to undergo a Maintainer Discussion *before* being created. If there is a PR that you would like to be reverted, please *manually create a thread in maint-reviews with the format REVERT-PRNumber and ping the relevant work group Maintainers.* Discussions should reach a resolution within 48 hours, if discussions have stalled notify the Game Director or a PM. **This does not apply for reverts made to the staging branch during the review period**, as there will be a discussion thread for the release. -- When closing a PR you must give a reason explaining why the PR is being closed, ideally this should include an alternative solution or approach. Dormant PRs may just be closed for the reason of being dormant, you may use any means to contact the author of the PR, but at the minimum you should leave a comment on the PR and give some time to respond before closing a dormant pr. +- When closing a PR you are **required** to give a reason explaining why the PR is being closed, and if applicable this should include an alternative solution or approach. Stale PRs may just be closed for the reason of being stale, as described under the Requirements section. - When reviewing a PR use *constructive* language and avoid referring negatively to the author's ability. Strong but fair criticism of code/decisions are fine, personal criticisms are not. -- When reviewing code, don't assume that all contributors have your knowledge of the codebase. Giving a short explaination of how something works can be far more helpful than responding with jargon and pointing to the docs. *Sometimes a newer contributor may need more help, in that case direct them to #HowDoICode or the docs. But ideally you should try to give them a short explaination on what they need to do rather than just a "change this". +- When reviewing code, always assume that the contributor may not have your knowledge of the codebase, and provide a short explanation or reason with your review comments. You shouldn't need to code for them, but pointing them in the right direction to learn more helps build a more robust contributor base. Sometimes a newer contributor may need more help, in that case direct them to #HowDoICode or the docs. But ideally you should try to give them a short explaination on what they need to do rather than just a "change this". + +- Any PR that is made for illegitimate reasons, such as abuse, spam or harrassment, may be closed without following the Policy as long as a PM has given written permission, as per the procedures set out in the Requirements section. + +## Issues & Reviews +While Issues do not have any code to review, they may still contain suggestions and feature proposals that would benefit from a Maintainer Discussion. + +- An Issue can be given the "S: Undergoing Maintainer Discussion" label to create a discussion thread as per the process described in the Policy section. If the discussion has a positive outcome, the issue can be given the "S: Conceptual Approval" label, and any PR that references the issue without major deviation can subsequently also be given the same label, indicating no new discussion is necessary.