Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a proposal for review process guidelines #120

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,48 @@ Contributions to Student Robotics' Runbook should:
[code-of conduct]: https://opsmanual.studentrobotics.org/about-the-charity/code-of-conduct
[license]: ./LICENSE
[contribution-guide]: docs/contributing.md

## Review Process

We should aim to keep review as lightweight as possible, with a bias towards
getting content live over making it perfect.

We do however want to review the content which is contributed for two reasons:

* content that is substantially unclear or actually wrong will do more harm than
a lack of documentation

* some of the content here covers sensitive topics such as safety measures at
events or safeguarding; it is especially important that documentation of these
areas is correct & clear, warranting more scrutiny

The following protocol (in line with how GitHub describe these terms) is
PeterJCLaw marked this conversation as resolved.
Show resolved Hide resolved
therefore proposed for review:

* _Approval_ responses mean "this change can merge as is" but may include
additional suggestions. Contributors may reject these suggestions, but are
expected to respond to them before merging if that is the case. Discussion of
these suggestions may continue after the PR is merged.

* _Comment_ responses are non-blocking, but also not approval, typically to ask
questions about the content or suggest improvements. Such responses may
indicate a review only of some part of the content.

* _Request changes_: responses are blocking and indicate that changes are needed
before content can be merged. The review should indicate, either in the
summary comment or in the inline comments, which of the issues raised are
blocking. Such responses should be rare.

In general reviewers are encouraged to bias in favour of merging unless they
feel that the change is particularly sensitive or would be a net negative to the
runbook.

### Practicalities of Review

* Please do choose reviewers when opening a PR, ideally by subject matter but if
in doubt pick one of the maintainers (@PeterJCLaw or @RealOrangeOne) and they
can redirect
* Please do use GitHub's tooling to ask for re-review after changes
PeterJCLaw marked this conversation as resolved.
Show resolved Hide resolved
* After review the contributor is typically responsible for performing the merge
themselves. New contributors (who may not have write access) should ping one
of the maintainers or reviewers to do this instead.