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 (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, 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 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 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. Github will automatically assign a PR with 1 Maintainer approval the S: Approved label. 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 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, 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.

• 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 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, 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.