From 2fe5af66fa037efcec57f3124af519fc79c62206 Mon Sep 17 00:00:00 2001 From: Kevin Reed Date: Tue, 14 Nov 2017 17:02:59 -0600 Subject: [PATCH 1/4] Update expectations of code review to contain a statement that provides guidance to how strict code reviews should be --- Expectations_of_Code_Review.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Expectations_of_Code_Review.md b/Expectations_of_Code_Review.md index b29c911..8b0228b 100644 --- a/Expectations_of_Code_Review.md +++ b/Expectations_of_Code_Review.md @@ -1,8 +1,14 @@ # Reviewer Expectations for Code Review -- Have verified that Definition of Done has been completed -- Visual inspection of code to see that it does what it needs to and doesn't do what is not reported -- Confirmed that all comments and questions raised in discussion of the pull request have been resolved +Code reviews have to support the competing goals of rapid code development, architectural integrity and quality code. To this end reviewers are asked to take a pragmatic approach to their review and they should accept code that may not be feature complete so long as it does not do any of the following: +- Contains known bugs that will occur under likely scenarios +- Prevent the code from building +- Prevent the code from working correctly under current use cases +- Interfere with the development activities of other contributors + +In addition, the reviewer should do the following and provide feedback about the items to the contributors: +- Verify that Definition of Done has been completed +- Visual inspect code to see that it does what it needs to and doesn't do what is not reported +- Confirm that all comments and questions raised in discussion of the pull request have been resolved - If possible, a reviewer should build and test the code changes -- Merging a pull request is a statement of "I believe this code is of good quality" - Check that consensus voting has resulted in approval of each issue resolved with the pull request (see governance document for details) - Check that consensus voting has resulted in approval of the pull request itself (see governance document for details) From 449a605dd0b3148b88eb3a33a5e66988f84ab20e Mon Sep 17 00:00:00 2001 From: Kevin Reed Date: Tue, 14 Nov 2017 17:10:34 -0600 Subject: [PATCH 2/4] Add links to expectations of code review and clean up sentence structure --- Expectations_of_Code_Review.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Expectations_of_Code_Review.md b/Expectations_of_Code_Review.md index 8b0228b..01f6a7c 100644 --- a/Expectations_of_Code_Review.md +++ b/Expectations_of_Code_Review.md @@ -5,10 +5,10 @@ Code reviews have to support the competing goals of rapid code development, arch - Prevent the code from working correctly under current use cases - Interfere with the development activities of other contributors -In addition, the reviewer should do the following and provide feedback about the items to the contributors: -- Verify that Definition of Done has been completed +In addition, the reviewer should review the following items and provide any needed feedback to the contributor(s): +- Verify that [Definition of Done[(https://github.com/BOINC/boinc-policy/blob/master/Definition_of_Done_for_Feature_Branches.md) has been completed - Visual inspect code to see that it does what it needs to and doesn't do what is not reported - Confirm that all comments and questions raised in discussion of the pull request have been resolved - If possible, a reviewer should build and test the code changes -- Check that consensus voting has resulted in approval of each issue resolved with the pull request (see governance document for details) -- Check that consensus voting has resulted in approval of the pull request itself (see governance document for details) +- Check that consensus voting has resulted in approval of each issue resolved with the pull request (see [governance document](https://github.com/BOINC/boinc-policy/blob/master/Governance.md#511-consensus-voting) for details) +- Check that consensus voting has resulted in approval of the pull request itself (see [governance document](https://github.com/BOINC/boinc-policy/blob/master/Governance.md#511-consensus-voting) for details) From bbdb8a3fea664312a7fbebaf885625e9cbce3810 Mon Sep 17 00:00:00 2001 From: Kevin Reed Date: Tue, 14 Nov 2017 17:11:54 -0600 Subject: [PATCH 3/4] Fix markdown error --- Expectations_of_Code_Review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Expectations_of_Code_Review.md b/Expectations_of_Code_Review.md index 01f6a7c..0ddaa6c 100644 --- a/Expectations_of_Code_Review.md +++ b/Expectations_of_Code_Review.md @@ -6,7 +6,7 @@ Code reviews have to support the competing goals of rapid code development, arch - Interfere with the development activities of other contributors In addition, the reviewer should review the following items and provide any needed feedback to the contributor(s): -- Verify that [Definition of Done[(https://github.com/BOINC/boinc-policy/blob/master/Definition_of_Done_for_Feature_Branches.md) has been completed +- Verify that the [Definition of Done](https://github.com/BOINC/boinc-policy/blob/master/Definition_of_Done_for_Feature_Branches.md) has been completed - Visual inspect code to see that it does what it needs to and doesn't do what is not reported - Confirm that all comments and questions raised in discussion of the pull request have been resolved - If possible, a reviewer should build and test the code changes From c3c97485056ff53c0af742c63b037a97d4d5448d Mon Sep 17 00:00:00 2001 From: Kevin Reed Date: Mon, 27 Nov 2017 16:11:58 -0600 Subject: [PATCH 4/4] Add modification to definition of done to reflect some basic software engineer principles about simplicity and minimal code. Changes made based on feedback from Bruce Allen. --- Definition_of_Done_for_Feature_Branches.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Definition_of_Done_for_Feature_Branches.md b/Definition_of_Done_for_Feature_Branches.md index e5b6ea1..b9cf5e6 100644 --- a/Definition_of_Done_for_Feature_Branches.md +++ b/Definition_of_Done_for_Feature_Branches.md @@ -2,6 +2,8 @@ The definition of done is an agile document that clarifies what it means for a set of development work to be completed. It is both a check list and a set of guidelines that should be followed. ## Development Practices +- Code should only contain the features necessary to solve the stated problem +- Code and data structures should be as simple and easy to understand as the stated problem allows - Code has followed the style guide for BOINC (http://boinc.berkeley.edu/trac/wiki/CodingStyle) - Does not contain unrelated code changes - Uses atomic commits (https://www.freshconsulting.com/atomic-commits/)