From 8ad3792aa023f52eb4f96e246b0adbabba3991f2 Mon Sep 17 00:00:00 2001 From: Jonathan Cave Date: Tue, 9 Jan 2024 17:29:25 +0000 Subject: [PATCH 1/4] Update CONTRIBUTING.md Modified the English of the code review section for readability, but no intent to change meaning. --- CONTRIBUTING.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f3e479304c..13ddd184b0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -304,15 +304,15 @@ about: ### Code review criteria & workflow -As a general rule there is one reviewer per PR. Other reviewers may pop in to smooth up the process (where they feel confident they can _approve and merge_ changes after some were requested by another review). When more reviewers are needed an approval of, comments explaining that are made in the comment thread. Chiefly this is done to work around cases of low test coverage or when the changes affect something known to be of low quality (i.e. something significantly complex and hard to reason about, brittle, dated, known to have broken in the past, etc). +As a general rule there should be one reviewer per PR. A review that requests changes should provide sufficient depth that the proposer is able to bring the code to an acceptable standard without input from other reviewers. In certain infrequent situations it may be necessary for a reviewwer to request that another reviewer provide additional guidance, this must be explicitly communicated in the PR comments. Examples of when this might be done are to work around cases of low test coverage or when the changes affect something known to be of low quality (i.e. something significantly complex and hard to reason about, brittle, dated, known to have broken in the past, etc). -On accepting a change without any change requests, the approving reviewer will merge the pull request. If they choose not to do that, they leave a comment explaining the rationale (mostly this exception is to cover situations when significant changes need to be staged across multiple releases). +On approving a change without any change requests the reviewer will merge the pull request. If they choose not to perform the merge, they must leave a comment explaining the rationale (mostly this exception is to cover situations when significant changes need to be staged across multiple releases). The reviewer is encouraged to use suggestions to communicate exact intended solutions, and to make it easy to apply them. The reviewer _must_ do this when making trivial style related suggestions. The reviewer might also post code into the PR, or to a branch branched off from the feature branch, to communicate more complex suggestions. -At all cases when a Checkbox maintainer picks up a PR for review, they are prepared to go back to the same PR if needed (if changes were requested). +Whenver a Checkbox maintainer provides a review for a PR they are accepting responsibility to follow the PR to conclusion. -Checkbox maintainers will follow the following process to make the determination between "accept", "request changes" and "comment". +The following sections describe the criteria on which Checkbox maintainers will make the determination between "Approve", "Request changes" and "Comment". #### Reviewer will approve the pull request when... From edbc7840e09033667cf08508de512f1b1c7108ae Mon Sep 17 00:00:00 2001 From: Jonathan Cave Date: Wed, 10 Jan 2024 11:30:07 +0000 Subject: [PATCH 2/4] first para Co-authored-by: kissiel --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 13ddd184b0..3da1b0d56f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -304,7 +304,7 @@ about: ### Code review criteria & workflow -As a general rule there should be one reviewer per PR. A review that requests changes should provide sufficient depth that the proposer is able to bring the code to an acceptable standard without input from other reviewers. In certain infrequent situations it may be necessary for a reviewwer to request that another reviewer provide additional guidance, this must be explicitly communicated in the PR comments. Examples of when this might be done are to work around cases of low test coverage or when the changes affect something known to be of low quality (i.e. something significantly complex and hard to reason about, brittle, dated, known to have broken in the past, etc). +As a general rule, there should be one reviewer per PR. A review that requests changes should provide sufficient depth so that the proposer can bring the code to an acceptable standard without input from other reviewers. In certain infrequent situations, it may be necessary for a reviewer to request that another reviewer provide additional guidance. This must be explicitly communicated in the PR comments. Examples of when this might be done include working around cases of low test coverage or when the changes affect something known to be of low quality (e.g., something significantly complex and hard to reason about, brittle, dated, known to have broken in the past, etc.). On approving a change without any change requests the reviewer will merge the pull request. If they choose not to perform the merge, they must leave a comment explaining the rationale (mostly this exception is to cover situations when significant changes need to be staged across multiple releases). From 6df73f21e8085753965d417afecb9f1af8e65a19 Mon Sep 17 00:00:00 2001 From: Jonathan Cave Date: Wed, 10 Jan 2024 11:32:32 +0000 Subject: [PATCH 3/4] suggestion 2 Co-authored-by: kissiel --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3da1b0d56f..7e7ca48fb4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -310,7 +310,7 @@ On approving a change without any change requests the reviewer will merge the pu The reviewer is encouraged to use suggestions to communicate exact intended solutions, and to make it easy to apply them. The reviewer _must_ do this when making trivial style related suggestions. The reviewer might also post code into the PR, or to a branch branched off from the feature branch, to communicate more complex suggestions. -Whenver a Checkbox maintainer provides a review for a PR they are accepting responsibility to follow the PR to conclusion. +Whenever a Checkbox maintainer provides a review for a PR, they accept responsibility to follow the PR through to its conclusion. The following sections describe the criteria on which Checkbox maintainers will make the determination between "Approve", "Request changes" and "Comment". From e83bcf8f559f5f359d1c6848bf881f237ee89e71 Mon Sep 17 00:00:00 2001 From: Jonathan Cave Date: Wed, 10 Jan 2024 11:33:49 +0000 Subject: [PATCH 4/4] flowery but ok Co-authored-by: kissiel --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e7ca48fb4..17ef8b267a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -312,7 +312,7 @@ The reviewer is encouraged to use suggestions to communicate exact intended solu Whenever a Checkbox maintainer provides a review for a PR, they accept responsibility to follow the PR through to its conclusion. -The following sections describe the criteria on which Checkbox maintainers will make the determination between "Approve", "Request changes" and "Comment". +The following sections describe the criteria upon which Checkbox maintainers will base their determination among "Approve", "Request Changes", and "Comment". #### Reviewer will approve the pull request when...