From f3c2ffb8ab969a97fe39df6b1ab735868e60b219 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sat, 18 Sep 2021 10:43:38 +0100 Subject: [PATCH 1/4] Add review process guidelines This is mostly a reflection of GitHub's terms, though with an explicit bias to action. --- CONTRIBUTING.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index db25d083..4fe9da38 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,3 +9,40 @@ 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 +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. 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. + +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. From 21c50485d339d963e6b3dd628a4758a917f119b1 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sat, 18 Sep 2021 11:02:17 +0100 Subject: [PATCH 2/4] Expand on the practicalities --- CONTRIBUTING.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4fe9da38..9ff76a8c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -43,6 +43,11 @@ 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. -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. +### Practicalities of Review + +* Please do choose reviewers when opening a PR, ideally by subject matter but if + in doubt pick one of the maintainers and they can redirect +* Please do use GitHub's tooling to ask for re-review after changes +* 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. From 9c6da4ed5dc6a01cded496efbbf78750ba69de20 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sat, 18 Sep 2021 12:21:57 +0100 Subject: [PATCH 3/4] Encourage clarity around what's blocking --- CONTRIBUTING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9ff76a8c..9193bb64 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,9 @@ therefore proposed for review: 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. Such responses should be rare. + 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 From db4bbcc0f4f62d47f11121d4420230022d42ffd3 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Sun, 3 Oct 2021 13:57:28 +0100 Subject: [PATCH 4/4] Clarify who the maintainers are Naming Jake & I here is hopefully temporary, at least until we can get something better set up (Code Owners is a likely candidate). --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9193bb64..352c2a41 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,7 +48,8 @@ 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 and they can redirect + 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 * After review the contributor is typically responsible for performing the merge themselves. New contributors (who may not have write access) should ping one