Skip to content

Commit

Permalink
Update docs with new label definitions (#350)
Browse files Browse the repository at this point in the history
Updates the docs with the small label adjustments, as well as clarifies
what `S: Approved` is intended for. Fairly small fix!
  • Loading branch information
SlamBamActionman authored Nov 15, 2024
1 parent 2c0588a commit 1e3310f
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 24 deletions.
8 changes: 4 additions & 4 deletions src/en/wizden-staff/maintainer/review-procedure.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@ Not following this procedure/policy will result in disiplinary action being take
- 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 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 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.
Expand All @@ -34,4 +34,4 @@ Not following this procedure/policy will result in disiplinary action being take
## 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.
- 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.
40 changes: 20 additions & 20 deletions src/en/wizden-staff/maintainer/triage-procedure.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,24 @@ Abuse of the Triage permissions (intentionally assigning incorrect labels, closi

When performing a triage, the goal is to add all relevant labels to the item. Some labels have concrete definitions to follow (such as the `Size` or `Branch` label categories), while others are subjective and need to be evaluated based on the item's contents. Triagers may close Issues at their own discretion should they no longer be relevant to keep open. *PRs may* ***not*** *be closed via triage*, as that is an action reserved for Maintainers.

When closing an issue, ideally either link to the PR that resolved it, post an in-game screenshot showing the issue is no longer relevant or mention why the issue should not be addressed and is an intended mechanic (in this last case, mark the Issue with `Bug: Intended Feature`).
When closing an issue, ideally either link to the PR that resolved it, post an in-game screenshot showing the issue is no longer relevant or mention why the issue should not be addressed and is an intended mechanic (in this last case, mark the Issue with `Issue: Intended Feature`).

### New Issue / PR Triage

Github automatically assigns the `S: Untriaged` label to any Issue or PR that is submitted. Triage should be performed on any such new item. Read through the item and assign appropriate labels based on its content.

For a triage to be completed, the item must have *at least one* label from the following label categories:
- Area
- Category
- Difficulty
- Priority
- Status
For a triage to be completed, the item must have *at least one* label from the following label categories. This is signified by these categories having a 1-letter category name:
- Area / A
- Difficulty / D
- Priority / P
- Status / S
- Type / T

While not mandatory, it's likely that the following label categories will be relevant for most items:
- Size (Only applicable to PRs, is assigned automatically)
- Changes (Applicable to both Issues and PRs, some Changes labels are assigned automatically)
The following categories are assigned automatically by Github:
- Size
- Changes

Once a triage is completed, remove the `S: Untriaged` label and attach 'S: Needs Review' for PRs, or 'S: Requires Content PR' for Issues (alternative labels may be more appropriate, such as when closing an issue).
Once a triage is completed, remove the `S: Untriaged` label and attach `S: Needs Review` for PRs, or `S: Requires Content PR` for Issues (alternative labels may be more appropriate, such as when closing an issue).

### Old Issue / PR Re-Triage

Expand All @@ -50,15 +50,15 @@ Each label has a description that explains what is to be used for in its categor
|---|---|---|
| Area | A | Describes which area of the project an item is related to. An item may be related to multiple areas, however it should always have at least one. An example would be a guidebook entry on an undocumented Science feature, which would fit within both the Guidebook and Science area. If an item doesn't seem to fit in any area, report it to a maintainer to see if a new label needs to be made. |
| Branch | Branch | If an item is intended for a non-master branch. Most commonly used for hotfixes. |
| Bug | B | Reserved for Issues, where a complex bug should be replicated to ensure it's accurately reported. Not necessary for all bug reports. |
| Category | C | What the item is attempting/suggesting to do. Multiple categories may be applicable. |
| Changes | Changes | Indicates an item should be handled by someone with knowledge in a certain area. |
| Difficulty | D | An estimate of how complex the item would be to review or create a PR for. Fairly subjective and should be based on the code.
| Fun | F | For the silly little labels. Should be used sparingly. |
| Intent | I | If the item is intended to be processed using an alternate review/merge policy. Used for hotfixes or test merges.
| Priority | P | How important the item is to the project. Contributors may tackle any item as they see fit, but this category should help guide what is important. |
| Size | size | How large the PR is codewise. Will ideally be automatically generated by Github. |
| Changes | Changes | Indicates an item should be handled by someone with knowledge in a certain area. Automatically generated by Github. |
| Difficulty | D# | 0-3, with 0 the hardest. An estimate of how complex the item would be to review or create a PR for. <br>`DB: Beginner Friendly` should include clear steps towards a solution for the issue. |
| Fun | Fun | For the silly little labels. Should be used sparingly. |
| Intent | Intent | If the item is intended to be processed using an alternate review/merge policy. Used for hotfixes or test merges.
| Issue | Issue | Reserved for Issues, where a complex bug should be replicated to ensure it's accurately reported. Not necessary for all bug reports. |
| Priority | P# | 0-3, with 0 the highest. How important the item is to the project. <br>`Critical` is only for emergencies. <br>`High` is for things that are important, but not emergencies. <br>`Raised` are things that should be brought to a maintainer's attention, due to being a particularly neat or useful feature. <br>`Standard` is the most common priority, and should be for everything else. |
| Size | size | How large the PR is codewise. Automatically generated by Github. |
| Status | S | The current status for the item. There should be at least one label in this category, though at times multiple may be applicable. |
| Type | T | What the item is attempting/suggesting to do. Multiple types may be applicable. |

## Non-Triager Labels

Expand All @@ -69,7 +69,7 @@ Some labels should not be used/removed by Triagers, as they are either automatic
| Changes category | Applied automatically.* |
| Fun category | Only at Maintainer/PM discretion. |
| Size category | Applied automatically.* |
| S: Approved | Only Maintainers can approve items. |
| S: Approved | Only Maintainers can approve items. Applied automatically. |
| S: Awaiting Changes | Applied automatically. |
| S: Conceptual Approval | Can only be applied by a Maintainer, following Review Procedure. |
| S: Undergoing Maintainer Discussion | Maintainer Discussions are at Maintainers' discretion. |
Expand Down

0 comments on commit 1e3310f

Please sign in to comment.