From d5c203b51e545f6fe3a64c427cb6738a8744e69c Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Thu, 9 Nov 2017 11:08:04 -0800
Subject: [PATCH] standard/review/template - Consistent order. Use comments for
instructions. Emphasize r-run prose.
Three changes here:
* Sort criteria in the same order across all three templates.
* For meta-instructions, use the Markdown/HTML comment notation.
* For `r-run`, we really don't want boilerplate. Remove the boilerplate.
---
docs/standards/review/template-del-1.0.md | 43 +++++++++++-----------
docs/standards/review/template-mc-1.0.md | 38 +++++++++----------
docs/standards/review/template-word-1.0.md | 6 +--
3 files changed, 43 insertions(+), 44 deletions(-)
diff --git a/docs/standards/review/template-del-1.0.md b/docs/standards/review/template-del-1.0.md
index 56a6c0df..da5b5323 100644
--- a/docs/standards/review/template-del-1.0.md
+++ b/docs/standards/review/template-del-1.0.md
@@ -6,47 +6,46 @@
* __UNREVIEWED__
* __PASS__ : The PR has a JIRA reference. (Or: it does not need one.)
* __ISSUE__: Please file a ticket in [JIRA](http://issues.civicrm.org/) and place it in the subject
- * __COMMENTS__: *(optional)*
+ * __COMMENTS__: <!-- optional -->
* Test results ([`r-test`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-test))
* __UNREVIEWED__
* __PASS__: The test results are all-clear.
* __PASS__: The test results have failures, but these have been individually inspected and found to be irrelevant.
* __ISSUE__: The test failures need to be resolved.
- * __COMMENTS__: *(optional)*
+ * __COMMENTS__: <!-- optional -->
* Code quality ([`r-code`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-code))
* __UNREVIEWED__
* __PASS__: The functionality, purpose, and style of the code seems clear+sensible.
* __ISSUE__: Something was unclear to me.
* __ISSUE__: The approach should be different.
- * __COMMENTS__: *(optional)*
+ * __COMMENTS__: <!-- optional -->
+* Documentation ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc))
+ * __UNREVIEWED__
+ * __PASS__: There are relevant updates for the documentation.
+ * __PASS__: The changes do not require documentation.
+ * __ISSUE__: The user documentation should be updated.
+ * __ISSUE__: The administrator documentation should be updated.
+ * __ISSUE__: The developer documentation should be updated.
+ * __COMMENTS__: <!-- optional -->
+* Maintainability ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint))
+ * __UNREVIEWED__
+ * __PASS__: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
+ * __PASS__: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
+ * __ISSUE__: The change does not sufficiently improve test coverage.
+ * __COMMENTS__: <!-- optional -->
* Run it ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run))
* __UNREVIEWED__
- * __PASS__: I executed the code in a few plausible ways, and it behaved as expected.
- * __ISSUE__: I executed the code in a few plausible ways, and it had a problem.
- * __COMMENTS__: *(optional)*
+ * __PASS__: <!-- describe how you ran it -->
+ * __ISSUE__: <!-- describe how you ran it -->
* User impact ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user))
* __UNREVIEWED__
* __PASS__: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
* __ISSUE__: The change would noticeably impact the user-experience (eg requiring retraining).
* __PASS__: The change would noticeably impact the user-experience (eg requiring retraining), but this has been addressed with a suitable transition/communication plan.
- * __COMMENTS__: *(optional)*
+ * __COMMENTS__: <!-- optional -->
* Technical impact ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech))
* __UNREVIEWED__
* __PASS__: The change preserves compatibility with existing callers/code/downstream.
* __PASS__: The change potentially affects compatibility, but the risks have been sufficiently managed.
* __ISSUE__: The change potentially affects compatibility, and the risks have **not** been sufficiently managed.
- * __COMMENTS__: *(optional)*
-* Maintainability ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint))
- * __UNREVIEWED__
- * __PASS__: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
- * __PASS__: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
- * __ISSUE__: The change does not sufficiently improve test coverage.
- * __COMMENTS__: *(optional)*
-* Documentation ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc))
- * __UNREVIEWED__
- * __PASS__: There are relevant updates for the documentation.
- * __PASS__: The changes do not require documentation.
- * __ISSUE__: The user documentation should be updated.
- * __ISSUE__: The administrator documentation should be updated.
- * __ISSUE__: The developer documentation should be updated.
- * __COMMENTS__: *(optional)*
+ * __COMMENTS__: <!-- optional -->
diff --git a/docs/standards/review/template-mc-1.0.md b/docs/standards/review/template-mc-1.0.md
index 21c345ba..b1900294 100644
--- a/docs/standards/review/template-mc-1.0.md
+++ b/docs/standards/review/template-mc-1.0.md
@@ -5,39 +5,39 @@
* JIRA ([`r-jira`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-jira))
* [ ] __PASS__ : The PR has a JIRA reference. (Or: it does not need one.)
* [ ] __ISSUE__: Please file a ticket in [JIRA](http://issues.civicrm.org/) and place it in the subject
- * [ ] __COMMENTS__: *(optional)*
+ * [ ] __COMMENTS__: <!-- optional -->
* Test results ([`r-test`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-test))
* [ ] __PASS__: The test results are all-clear.
* [ ] __PASS__: The test results have failures, but these have been individually inspected and found to be irrelevant.
* [ ] __ISSUE__: The test failures need to be resolved.
- * [ ] __COMMENTS__: *(optional)*
+ * [ ] __COMMENTS__: <!-- optional -->
* Code quality ([`r-code`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-code))
* [ ] __PASS__: The functionality, purpose, and style of the code seems clear+sensible.
* [ ] __ISSUE__: Something was unclear to me.
* [ ] __ISSUE__: The approach should be different.
- * [ ] __COMMENTS__: *(optional)*
+ * [ ] __COMMENTS__: <!-- optional -->
+* Documentation ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc))
+ * [ ] __PASS__: There are relevant updates for the documentation, or the changes do not require documentation.
+ * [ ] __ISSUE__: The user documentation should be updated.
+ * [ ] __ISSUE__: The administrator documentation should be updated.
+ * [ ] __ISSUE__: The developer documentation should be updated.
+ * [ ] __COMMENTS__: <!-- optional -->
+* Maintainability ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint))
+ * [ ] __PASS__: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
+ * [ ] __PASS__: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
+ * [ ] __ISSUE__: The change does not sufficiently improve test coverage.
+ * [ ] __COMMENTS__: <!-- optional -->
* Run it ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run))
- * [ ] __PASS__: I executed the code in a few plausible ways, and it behaved as expected.
- * [ ] __ISSUE__: I executed the code in a few plausible ways, and it had a problem.
- * [ ] __COMMENTS__: *(optional)*
+ * [ ] __PASS__: <!-- describe how you ran it -->
+ * [ ] __ISSUE__: <!-- describe how you ran it -->
+ * [ ] __COMMENTS__: <!-- optional -->
* User impact ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user))
* [ ] __PASS__: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
* [ ] __ISSUE__: The change would noticeably impact the user-experience (eg requiring retraining).
* [ ] __PASS__: The change would noticeably impact the user-experience (eg requiring retraining), but this has been addressed with a suitable transition/communication plan.
- * [ ] __COMMENTS__: *(optional)*
+ * [ ] __COMMENTS__: <!-- optional -->
* Technical impact ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech))
* [ ] __PASS__: The change preserves compatibility with existing callers/code/downstream.
* [ ] __PASS__: The change potentially affects compatibility, but the risks have been sufficiently managed.
* [ ] __ISSUE__: The change potentially affects compatibility, and the risks have **not** been sufficiently managed.
- * [ ] __COMMENTS__: *(optional)*
-* Maintainability ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint))
- * [ ] __PASS__: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
- * [ ] __PASS__: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
- * [ ] __ISSUE__: The change does not sufficiently improve test coverage.
- * [ ] __COMMENTS__: *(optional)*
-* Documentation ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc))
- * [ ] __PASS__: There are relevant updates for the documentation, or the changes do not require documentation.
- * [ ] __ISSUE__: The user documentation should be updated.
- * [ ] __ISSUE__: The administrator documentation should be updated.
- * [ ] __ISSUE__: The developer documentation should be updated.
- * [ ] __COMMENTS__: *(optional)*
+ * [ ] __COMMENTS__: <!-- optional -->
diff --git a/docs/standards/review/template-word-1.0.md b/docs/standards/review/template-word-1.0.md
index f36711cf..45e3adfa 100644
--- a/docs/standards/review/template-word-1.0.md
+++ b/docs/standards/review/template-word-1.0.md
@@ -7,6 +7,6 @@
* ([`r-code`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-code)) __Undecided__
* ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc)) __Undecided__
* ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint)) __Undecided__
-* ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run)) __Undecided__: (*Describe...*)
-* ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user)) __Undecided__: (*Describe...*)
-* ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech)) __Undecided__: (*Describe...*)
+* ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run)) __Undecided__: <!-- Describe -->
+* ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user)) __Undecided__: <!-- Describe -->
+* ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech)) __Undecided__: <!-- Describe -->
--
GitLab