-
Notifications
You must be signed in to change notification settings - Fork 0
Design and Code Review Policy
This document lays out the basic philosophy and steps behind the LINCC-Frameworks design review and code review processes.
The purpose of design reviews is to share expertise and provide feedback on 1) new projects, 2) major changes to existing projects, or 3) large new features before the majority of the implementation is started.
- Always treat others with respect. Design reviews should focus on the technical aspects of the project and the tradeoffs.
- Ask questions. If you do not understand something, don’t assume it just works. Design reviews are a chance to explore tradeoffs and search for potential problems. How will the code scale? What are the SLAs? How is data backed up? What happens in case of a failure?
- Share expertise. Do not be afraid to add your opinions or thoughts. One of the major values of a design review is sharing expertise and providing an opportunity for others to learn.
- Be open to arguments. Design reviews are an opportunity for everyone to learn. Listen to team member’s advice and technical arguments.
- Be prepared to disagree and commit. Many times there will be multiple valid approaches and we will not reach 100% consensus on each proposal. If there isn’t a clear winning approach in the discussion, be willing to disagree with the approach but still commit to driving it forward.
- The designer(s) writes a design proposal (for early stage work) or design document (for a full design review) using the LINCC-Frameworks templates.
- The document is circulated to the team ahead of a review for the other team members to pre-read and comment on.
- Design documents should be distributed approximately one week before the review in order to give reviewers time to read the document.
- Required reviewers should include:
- For all designs: Someone on that subproject and knowledgeable about the technical details of the specific project.
- For new features or project directions: A research scientist knowledgeable about the domain who can comment on the scientific use cases.
- For major changes or foundational designs: At least one engineer who is not on the immediate project and can provide a fresh perspective.
- Depending on complexity of the review and the initial comments, there are two potential next steps:
- There are no major concerns and the review is completed within the document.
- Further discussion is required and a design review is scheduled for the team members to discuss the design and tradeoffs.
- Concerns or open questions should be captured in notes. Each question should be marked as blocking or non-blocking. All blocking items must be addressed before the design is approved.
We hope the majority of design reviews will lead to consensus. In the case where there is disagreement between two equally valid approaches, the team’s proposed approach will be selected. We will use a disagree and commit approach.
In the case where there is a disagreement between approaches / tradeoffs that team members do not agree are equally valid, the leads for that project will make the decision. In case of a disagreement between leads, the framework leads (Jeremy, Rachel, Andy) will be the final deciders.
The purpose of code reviews is to check new code for errors, assure that new code meets the team’s quality standards and styles, and to share knowledge throughout the team. All new code should be reviewed by either a LINCC-Frameworks team member or experienced owner from the corresponding project.
The principles for code reviews are similar to those for design reviews.
- Always treat others with respect. Code reviews should focus on the technical aspects of the code.
- Share expertise and add comments. Code reviews are only useful if the reviewer shares their suggestions or concerns. This is an opportunity for the author to collect feedback.
- Ask questions. If you do not understand something, don’t assume it just works.
- Be open to arguments. Code reviews are an opportunity for people to learn.
- Be prepared to disagree and commit. Many times there will be multiple valid approaches and we will not reach 100% consensus on each proposal. This is especially the case for some stylistic approaches.
- Check for tests. New code should be tested and those tests should ensure the behavior of the code (and not just the coverage).
- The author(s) writes new code and submits a pull request via github. The author suggests reviewers.
- The author also posts the PR link to the #lincc-codereviews slack channel. This allows other team members to see what changes are being made, comment (if they want), and learn.
- Reviewers add comments to the review. Blocking comments must be resolved before a PR is submitted.
- Each PR must be approved by either a team member or (in cases of external projects) an experienced project contributor.
- The pull request description should include both a description of the changes made and the context for making these changes. Bug fixes should refer back to the corresponding bug. New features should reference the scientific use cases. Link to github issues whenever possible.
- New code should include relevant comments and unit tests in the pull request. These are not items to be added later.
- Run all tests, including unit tests and regression tests, before sending out the pull request.
- Documentation should be updated in the pull request or in parallel (depending on where the documentation is kept).
- Changes should be broken up into the smallest functional sizes possible. Ten thousand line code reviews are both time consuming and difficult for the reviewer. Break code reviews into smaller changes where possible. Consider pre-factorings to set up major changes.
- New code should conform to the style of the current package. Run applicable auto-formaters on the code before sending out the pull request.
- Choose reviewers who can provide meaningful review of the code. Consider adding more than one reviewer depending on the scope of the change:
- For all changes: A reviewer knowledgeable about the technical details of the specific project.
- For new features or project directions: A research scientist knowledgeable about the domain who can comment on the scientific use cases and code usability.
- For major changes or foundational designs: A knowledgeable technical reviewer from outside the immediate project who can provide a fresh perspective.
To effectively review code it is necessary to not only understand what the code is doing (how the bits are moving around), but the problem it is solving and the context of the rest of the program. Is the code actually solving the problem it is setting out to solve? Is the change even necessary? Does the approach work in the context of the full system? The first step of a code review is to understand these factors. Sometimes the current PR will just be the first step of a larger set of changes, so the end goal might not be obvious. Ask questions if needed.
Now perform the code review:
- Check that the code is correct in its intended change. Do you see any errors in logic? Are all cases covered? These should be blocking changes.
- Check that the changes are tested. Tests should confirm the behavior of the code, not just that it is able to compile and run. If someone later broke the code, would the tests pick that up? Suggest specific test cases or scenarios that are missing.
- It’s okay to suggest adding test cases that cover logic closely adjacent to the current change. If the code branches over 4 different cases and only 3 are tested (including the one added by the author), suggest covering the 4th.
- It’s also okay to ask “How did you test this?” Large changes need to go beyond the unit tests and might require running a diff test or regression test.
- Comment on the organization of the code. Is it maintainable (good)? Is it a single huge function with intertwined if statements and global variables (bad)? Suggest improvements, such as breaking commonly used code into helper functions or breaking apart large files.
- Check that the new code contains appropriate comments. If there is a portion that you don’t understand, it likely needs a comment.
- Confirm that relevant documentation has been updated. Changes in behavior, parameters, etc. should include changes to the documentation at the time the code is written.
- Check that the new/updated code conforms to the style of the package. LINCC-Framework generally uses LSST DM’s styles (Python and C++). These are both pretty extensive, so we shouldn’t expect people to memorize all the rules. Instead ensure that the code looks consistent with the surrounding code and check whether you see something you know violates the rules. Refer to the style guide when needed, but we don’t expect the code reviewer to run through the entire style guide as a checklist. It is okay to miss small stylistic things (and if you see style problems in existing code, please do submit a PR to fix it).
- Add all comments to the pull request. Note which comments (if any) are non blocking.
- Submit the review by marking the PR as either approved, comments, or needs more work. Do not approve until all blocking comments have been addressed.
We hope the majority of code reviews will lead to consensus. In the case where there is disagreement between two equally valid approaches, the author’s initial proposed approach will be selected. We will use a disagree and commit approach.
In the case where there is a disagreement between approaches / tradeoffs that team members do not agree are equally valid, the owner of the codebase will make the final decision.