-
-
Notifications
You must be signed in to change notification settings - Fork 4
[Handbook] Reviewing Pull Request
This document outlines the goals and process for reviewing pull requests (PRs) in our GitHub repository. The aim is to ensure code quality, maintainability, and project stability.
-
Functionality and Correctness: The PR should successfully address the problem it intends to solve. This includes verifying that the changes do not introduce new bugs or regressions. The PR message should be related to the linked GitHub issue and the code changes are related too.
-
Passing CI Tests: All continuous integration (CI) tests must pass successfully. This provides a baseline level of confidence in the code's correctness and stability. By passing CI tests we assume it does not break anything, then merge is allowed (even though it may introduce new bug, we will leave it until someone really triggers it).
While thorough review is crucial, efficiency is also important. In cases where there's high trust in the submitting developer and the reviewer understands the changes, a simple "looks good to me" might suffice. However, this should only be done when the reviewer is confident in their understanding of the code and its implications.
To enhance the reliability of our CI-based review process, we should strive to improve our end-to-end (e2e) test coverage and add unit tests where necessary. This will help catch potential issues earlier in the development cycle.
Generally, we trust the PR author to act in the best interest of the project. However, significant code changes require thorough explanation and justification. A good rule of thumb is to consider changes exceeding a couple hundred lines of diff as "significant" and requiring extra scrutiny. The definition of "significant" may need further refinement based on project context.
This document is a living document. If you encounter a situation not covered here, please raise an issue or suggest an update with your proposed changes. We follow a "no fix unless needed" approach; if this document functions adequately, we won't change it unless there's a compelling reason to do so.