-
Notifications
You must be signed in to change notification settings - Fork 170
Code Review
Code reviews are key to maintaining a consistent, high-quality codebase. Every person on the team needs to be involved in the process. All team members should actively be reviewing PRs that they have knowledge of, and being very thorough in their feedback to reviews received.
This document covers
- Code Owners Setup
- Review Process
- Review Sessions
Intended audience: Developers
Reviews are both one of the most important parts of our code process, and often one of the biggest chokepoints for development.
While we reach out frequently on slack to request reviews, discussion that's directly relevant to the PR should stay within GitHub to reduce information fragmentation. Tangentially related discussion can stay in slack without concern.
GitHub documentation on Code Reviews can be found here.
- CI should be run to validate there’s no tests that fail unexpectedly
- Reviewers comment frequently so the PR author doesn’t need to reach out for clarification unless there’s no clear path
- Reviewers should make note of issues around:
- Breaking changes*
- Style/documentation
- Unclear logic
- Possible tech-debt being built in
- Security vulnerabilities
If changes are requested/required, the engineer will correct and the process repeats until all concerns are resolved. The PR is then ready to be merged.
Before a PR with breaking changes is merged, we must make sure we've communicated them clearly and with generous for users to prepare.
TODO: Do we have a communication platform for this like RAPIDS does? We can use GH Discussions.
- Make sure that the breaking changes are intentional and necessary
- Ensure the PR has the
Breakinglabel - Make sure to clearly document all breaking changes in the PR description + the code comments. Explain what has changed, why it has changed, and how it will affect existing functionality. This helps reviewers understand the changes and find any potential issues
- Test the breaking changes thoroughly and be sure CI is updated for the changes
- After the changes have been deployed, monitor the impact of the breaking changes carefully. Keep an eye out for any unexpected issues or negative feedback, and be prepared to make further adjustments if necessary.
Codeowners automatically define who should be reviewing PRs. We set up our codeowners by creating Teams in GitHub that cover areas CCCL repo such as Thrust-Owners. This way it is easy to request a review from someone who is knowledgeable in the area.
Review sessions are a dedicated time block where all team members are invited to join a call to discuss open reviews. There is no expectation of camera usage, nor talking when you're not involved in a review.
Traditionally, the lead of the code-area being reviewed (ie C++/Python/etc) shares their screen and leads the review session.
Doing this both makes sure PRs don’t get stale waiting on reviews, and provides a place where team members know that everyone will be in one place so changes that affect a wide audience are especially beneficial for discussion here.
The goal is that open questions can be resolved, and if there are no open questions then it provides a blocked out time window where reviews are prioritized. Letting PRs go stale hurts velocity and increases risk of blockers and merging conflicts, so timely resolution is critical.
If a review session has no applicability for you, there's no need to attend. If you'd like to discuss a separate review with team members who aren't involved with the "main" review going on at any given time, use a Breakout Room function to pull relevant members in.
Any final thoughts/references go here.