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.

Sign up for GitHub

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

Update expectations of code review #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update expectations of code review #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fe5af6
Update expectations of code review to contain a statement that provides
Nov 14, 2017
449a605
Add links to expectations of code review and clean up sentence structure
Nov 14, 2017
bbdb8a3
Fix markdown error
Nov 14, 2017
c3c9748
Add modification to definition of done to reflect some basic software
Nov 27, 2017
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
2 changes: 2 additions & 0 deletions Definition_of_Done_for_Feature_Branches.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
Expand Down
18 changes: 12 additions & 6 deletions Expectations_of_Code_Review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 review the following items and provide any needed feedback to the contributor(s):
- 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
- 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)
- 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)