Pull Request Review Guidelines
This document outlines our team's expectations and workflow for reviewing pull requests (PRs). It is designed to ensure consistency, quality, and collaboration during the review process. By adhering to these guidelines, we aim to maintain high-quality code, foster collaboration, and continuously improve our system and team practices.
Goals of PRs
Pull requests serve several key purposes:
- Automated Validation: Ensure basic correctness through automated checks
- Code Review: Facilitate human oversight to catch potential issues and share knowledge
- Team Collaboration: Promote shared understanding and cross-training across the team
Automated Tasks
To reduce manual effort and ensure consistency, the following automated tasks must run on every PR:
1. Check Code Style
Ensure adherence to the agreed-upon code style and formatting guidelines.
2. Execute Automated Tests
- Run unit tests to validate individual components
- Execute integration tests (where applicable) to verify the behaviour of combined components
Note: Any PR failing these automated checks should be corrected before proceeding to human review.
Code Review Guidelines
When reviewing a pull request, team members should focus on the following aspects:
1. Potential Side-Effects
Are there any potential side-effects or unintended consequences of these code changes that might not be immediately apparent?
2. Correct Usage of Classes/Components
- Are any classes, components, or libraries being used incorrectly?
- Does the code adhere to our agreed-upon guidelines and architectural decision records (ADRs)?
3. System Understanding and Cross-Training
- Use the review process as an opportunity to share knowledge and enhance the team's collective understanding of the system
- Encourage discussions that lead to cross-training and knowledge transfer
4. Enhancements and Improvements
Identify possible enhancements or optimizations to improve the system's functionality, performance, or maintainability.
5. Test Coverage
- Verify that tests provide adequate coverage for the changes introduced
- Suggest additional tests if there are gaps in coverage
Submitter Responsibilities
- Ensure all automated checks pass before requesting a review
- Provide context for the changes, including the "why" behind them, in the PR description
- Be responsive to feedback and promptly address requested changes
Reviewer Responsibilities
- Be thorough and constructive in your feedback
- Focus on actionable improvements and avoid nit-picking minor issues unless they are critical
- Respond promptly to PRs to maintain workflow efficiency