Skip to content

Commit b8e1024

Browse files
josecelanojosecelano
committed
Merge #104: Add PR Review Guide for systematic code reviews
533a700 docs: [#80] add comprehensive PR review guide for contributors (copilot-swe-agent[bot]) 06f3e0a Initial plan (copilot-swe-agent[bot]) Pull request description: Code reviews lacked systematic guidance, allowing architectural violations like #75 (config module in wrong DDD layer) to merge undetected. Reviewers needed a consolidated checklist of quality standards and common red flags. ## Changes ### New PR Review Guide (`docs/contributing/pr-review-guide.md`) **Quality Standards Checklist** - Systematic verification of: - Branching/commits (naming, conventional format) - Code quality (DDD layer placement, error handling, module organization) - Testing (coverage, conventions, isolation) - Documentation (ADRs, rustdoc) - Templates (Tera syntax, Ansible registration) **Quick Red Flags** - Fast-scanning checklist for common violations: ```markdown Architecture Violations: - ❌ File I/O in domain/application layers - ❌ Business logic in infrastructure/presentation - ❌ Presentation directly calling infrastructure Error Handling Issues: - ❌ Generic string errors instead of typed enums - ❌ Errors without context or actionable guidance ``` **Feedback Guidance** - When to request changes vs. comment vs. approve, with examples showing how to reference documentation: ```markdown > This error handling doesn't follow [error-handling.md](./error-handling.md). > See `src/domain/config/error.rs` for the typed error enum pattern. ``` **Living Document** - Framework for continuous improvement as patterns emerge ### Issue Template Updates Added contributor note to `SPECIFICATION-TEMPLATE.md` and `GITHUB-ISSUE-TEMPLATE.md`: > These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting. ### Documentation Integration - Added "Acceptance Criteria as PR Review Checklist" section to `roadmap-issues.md` explaining dual purpose (for authors and contributors) - Linked from `docs/contributing/README.md` for discoverability ## Cross-References Guide links to: DDD layer placement, error handling, module organization, branching, commit process, testing conventions, templates, known issues, and development principles. <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Add PR Review Guide for Contributors</issue_title> > <issue_description># Add PR Review Guide for Contributors > > ## 📋 Issue Information > > - **Type**: Documentation Enhancement > - **Priority**: High > - **Related Issue**: #75 - Move config module to correct DDD layer > - **Depends On**: #79 - Add DDD Layer Placement Guide > - **Parent Epic**: None (standalone improvement) > > ## 🎯 Problem Statement > > Code reviews currently lack a systematic guide for reviewers. This has allowed issues like architectural violations (#75 - config module in wrong layer) to be merged without detection. > > Reviewers need: > > - A quick reference guide for important review aspects > - Clear red flags to watch for > - Consistent review criteria across all PRs > > Without a standardized guide, we risk: > > - Merging code that violates architectural principles > - Inconsistent review quality > - Missing important quality aspects > - Technical debt accumulation > > ## 💡 Proposed Solution > > Create a concise PR review guide at `docs/contributing/pr-review-guide.md` that consolidates project quality standards for reviewers, including a comprehensive checklist, quick red flags section, and guidance on providing constructive feedback. > > ## 📝 Implementation Plan > > See detailed specification: [docs/issues/add-pr-review-guide.md](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/issues/add-pr-review-guide.md) > > ## ✅ Acceptance Criteria > > > **Note for Contributors**: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations. > > - [ ] Document created at `docs/contributing/pr-review-guide.md` > - [ ] Comprehensive quality standards checklist included > - [ ] Checklist covers: branching, commits, code quality, testing, documentation > - [ ] Quick red flags section for common violations > - [ ] Example feedback showing how to reference docs > - [ ] Guidance on when to request changes vs. comment vs. approve > - [ ] Issue templates updated with contributor note about acceptance criteria > - [ ] `docs/issues/SPECIFICATION-TEMPLATE.md` updated with pre-review checklist note > - [ ] `docs/issues/GITHUB-ISSUE-TEMPLATE.md` updated with pre-review checklist note > - [ ] `docs/contributing/roadmap-issues.md` updated with "Acceptance Criteria as PR Review Checklist" section > - [ ] Linked from `docs/contributing/README.md` > - [ ] All linters pass (markdownlint, cspell) > - [ ] Document follows project markdown conventions > - [ ] Clearly states it's a living document that will evolve > > ## 🏷️ Labels > > documentation, enhancement, code-review, contributing-guide</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> - Fixes #80 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/torrust/torrust-tracker-deployer/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. ACKs for top commit: josecelano: ACK 533a700 Tree-SHA512: ca3804548b344975de28ac9218240483d761adebafc686825b6e2c56bb5b114fa5f92d58755298796c2232c933885f001d2762b870d3bc5a916c6099d9992dad
2 parents 7a5afc1 + 533a700 commit b8e1024

File tree

5 files changed

+219
-0
lines changed

5 files changed

+219
-0
lines changed

‎docs/contributing/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ This guide will help you understand our development practices and contribution w
77
| Topic | File |
88
| ------------------------------------ | ---------------------------------------------------- |
99
| DDD layer placement (architecture) | [ddd-layer-placement.md](./ddd-layer-placement.md) |
10+
| PR review guide for reviewers | [pr-review-guide.md](./pr-review-guide.md) |
1011
| Creating roadmap issues | [roadmap-issues.md](./roadmap-issues.md) |
1112
| Branching conventions | [branching.md](./branching.md) |
1213
| Commit process and pre-commit checks | [commit-process.md](./commit-process.md) |

‎docs/contributing/pr-review-guide.md‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
# PR Review Guide
2+
3+
This guide helps reviewers verify that pull requests meet our quality standards. Use this as a systematic checklist when reviewing code to ensure consistency and catch issues early.
4+
5+
> **Note**: This is a living document that will evolve as we learn from reviews. All automated checks (linters, tests, builds) must pass before review begins.
6+
7+
## 📋 Quality Standards Checklist
8+
9+
### Branching & Commits
10+
11+
Verify contributors followed these project conventions:
12+
13+
- [ ] **Branch name** follows `{issue-number}-{description}` format (see [branching.md](./branching.md))
14+
- [ ] **Commit messages** use conventional format:
15+
- With issue branch: `{type}: [#{issue}] {description}`
16+
- Without issue branch: `{type}: {description}`
17+
- See [commit-process.md](./commit-process.md) for details
18+
- [ ] **Pre-commit checks** passed (contributor should have run `./scripts/pre-commit.sh`)
19+
20+
### Code Quality
21+
22+
- [ ] **DDD layer placement** is correct - business logic in domain, orchestration in application, external integrations in infrastructure (see [ddd-layer-placement.md](./ddd-layer-placement.md))
23+
- [ ] **Error handling** uses explicit enum errors with context, not generic strings (see [error-handling.md](./error-handling.md))
24+
- [ ] **Errors are actionable** - include clear guidance on how to fix the issue
25+
- [ ] **Module organization** follows conventions: public before private, top-down structure (see [module-organization.md](./module-organization.md))
26+
- [ ] **Code is clean and maintainable** - clear naming, minimal complexity, well-structured
27+
- [ ] **No obvious security vulnerabilities** introduced
28+
29+
### Testing
30+
31+
- [ ] **New logic has appropriate test coverage** (see [testing/](./testing/))
32+
- [ ] **Tests follow naming conventions** (`it_should_...` pattern)
33+
- [ ] **Tests are isolated** - use temporary resources, don't depend on external state
34+
- [ ] **Tests are readable** - clear intent and easy to understand what's being tested
35+
- [ ] **Both production and test code** meet quality standards (clean, maintainable, sustainable)
36+
37+
### Documentation
38+
39+
- [ ] **Significant architectural decisions** documented as ADRs in `docs/decisions/` (see [decisions/README.md](../decisions/README.md))
40+
- [ ] **Public APIs have rustdoc comments** with clear descriptions
41+
- [ ] **Complex logic is explained** with comments where necessary
42+
- [ ] **User-facing documentation updated** if behavior changes affect users
43+
44+
### Templates (if applicable)
45+
46+
- [ ] **Tera templates use correct syntax**: `{{ variable }}` not `{ { variable } }` (see [templates.md](./templates.md))
47+
- [ ] **Static Ansible playbooks** (without `.tera` extension) are registered in `src/infrastructure/external_tools/ansible/template/renderer/mod.rs`
48+
49+
## 🚩 Quick Red Flags
50+
51+
Watch for these common issues that indicate quality problems:
52+
53+
### Architecture Violations
54+
55+
- ❌ **File I/O in domain or application layers** - Should be in infrastructure
56+
- ❌ **Business logic in infrastructure or presentation layers** - Should be in domain
57+
- ❌ **Presentation layer directly calling infrastructure** - Should go through application layer
58+
- ❌ **Domain layer depending on infrastructure** - Violates dependency flow rules
59+
- ❌ **Mixing concerns across layers** - Each layer should have clear responsibilities
60+
61+
### Error Handling Issues
62+
63+
- ❌ **Generic string errors** instead of typed enums - Use explicit error types
64+
- ❌ **Error messages without context** - Include what, where, when, why
65+
- ❌ **Error messages without actionable guidance** - Tell user how to fix it
66+
- ❌ **Lost error chains** - Missing source preservation, can't trace root cause
67+
- ❌ **Using `anyhow` where explicit enums would be better** - Prefer pattern matching
68+
69+
### Code Organization Problems
70+
71+
- ❌ **Private items before public items** in modules - Public should come first
72+
- ❌ **Low-level details before high-level abstractions** - Use top-down organization
73+
- ❌ **Error types mixed with main implementation logic** - Separate concerns
74+
- ❌ **Inconsistent naming** - Follow Rust conventions and project patterns
75+
76+
### Testing Issues
77+
78+
- ❌ **Tests using `unwrap()` without explanation** - Use proper error handling
79+
- ❌ **Tests creating real directories** instead of temp directories - Use `TempDir`
80+
- ❌ **Missing tests for new error paths** - Error handling should be tested
81+
- ❌ **Tests that depend on external state** - Tests should be isolated
82+
- ❌ **Test code that doesn't meet production quality standards** - Both should be clean
83+
84+
### Known Issues vs. Real Problems
85+
86+
Be aware of expected behaviors documented in [known-issues.md](./known-issues.md):
87+
88+
- ✅ **SSH host key warnings** in E2E test output are normal and expected
89+
- ✅ **Red error messages** during setup don't always indicate failure
90+
- ❌ **New unexpected failures** should be investigated and resolved
91+
92+
## 🗣️ Providing Feedback
93+
94+
### Be Constructive and Specific
95+
96+
When providing feedback:
97+
98+
1. **Point to documentation** - Reference specific contributing guides
99+
2. **Be specific** - Link to exact lines and explain the concern clearly
100+
3. **Suggest alternatives** - Provide examples or point to similar code in the codebase
101+
4. **Explain why** - Help contributors understand the reasoning behind standards
102+
5. **Distinguish severity** - Make clear whether something is blocking or a suggestion
103+
104+
### Example Feedback
105+
106+
**Good feedback** references our documentation and is constructive:
107+
108+
> This error handling doesn't follow our guidelines from [error-handling.md](./error-handling.md). Errors should use explicit enums with context rather than generic strings.
109+
>
110+
> Please see the example in `src/domain/config/error.rs` which shows how to create a typed error enum with proper context fields. This will make the errors more actionable for users and easier to handle in calling code.
111+
112+
**Better feedback** is specific and actionable:
113+
114+
> In `src/application/commands/provision.rs` line 42, using `.unwrap()` will panic if the file doesn't exist. Instead:
115+
>
116+
> 1. Define an error variant in the command's error enum for file not found
117+
> 2. Use `.map_err()` to convert the I/O error to your domain error
118+
> 3. Include the file path in the error context
119+
>
120+
> See [error-handling.md](./error-handling.md) section 2 for the pattern.
121+
122+
### When to Request Changes vs. Comment vs. Approve
123+
124+
**Request Changes** - Blocking issues that violate documented standards:
125+
126+
- Architectural violations (wrong DDD layer placement)
127+
- Security vulnerabilities introduced
128+
- Breaking existing functionality
129+
- Missing required tests for new code
130+
- Error handling that doesn't meet standards
131+
- Pre-commit checks not passing
132+
133+
**Comment** - Suggestions or questions (non-blocking improvements):
134+
135+
- Minor style inconsistencies not caught by linters
136+
- Optional refactoring opportunities
137+
- Questions about approach or implementation
138+
- Nice-to-have documentation improvements
139+
- Suggestions for future enhancements
140+
141+
**Approve** - All standards met:
142+
143+
- All quality checklist items verified
144+
- No blocking issues found
145+
- Changes are minimal and focused
146+
- Tests pass and provide good coverage
147+
- Documentation is adequate
148+
- Code follows project conventions
149+
150+
## 🔄 Evolving This Guide
151+
152+
This is a living document. As we identify new patterns during reviews, we can:
153+
154+
1. **Update this guide** with new checklist items or red flags
155+
2. **Update specific contributing guides** for detailed guidance on particular topics
156+
3. **Add to "Quick Red Flags"** section for common review issues we encounter
157+
4. **Document new architectural decisions** as ADRs in `docs/decisions/`
158+
5. **Update issue templates** if acceptance criteria patterns emerge
159+
160+
If you notice patterns that should be added to this guide, please:
161+
162+
- Create an issue to discuss the addition
163+
- Reference examples from recent PRs that show the pattern
164+
- Propose specific checklist items or red flag entries
165+
- Update relevant contributing guides with detailed guidance
166+
167+
## 📚 Related Resources
168+
169+
### Contributing Guides
170+
171+
- [DDD Layer Placement Guide](./ddd-layer-placement.md) - Architectural guidance
172+
- [Error Handling Guide](./error-handling.md) - Error handling patterns
173+
- [Module Organization](./module-organization.md) - Code organization within files
174+
- [Branching Conventions](./branching.md) - Branch naming rules
175+
- [Commit Process](./commit-process.md) - Commit message format
176+
- [Testing Guide](./testing/) - Testing conventions and patterns
177+
- [Templates Guide](./templates.md) - Working with Tera templates
178+
- [Known Issues](./known-issues.md) - Expected behaviors and workarounds
179+
180+
### Development Principles
181+
182+
- [Development Principles](../development-principles.md) - Core principles guiding all development
183+
- [Codebase Architecture](../codebase-architecture.md) - Overall architecture overview
184+
- [Architectural Decisions](../decisions/) - Decision records and rationale
185+
186+
### Tools and Automation
187+
188+
- [Linting Guide](./linting.md) - Running and configuring linters
189+
- [Pre-commit Script](../../scripts/pre-commit.sh) - Automated quality checks
190+
191+
---
192+
193+
**Remember**: Reviews are about maintaining quality and helping contributors improve. Focus on being helpful, constructive, and educational. Point to documentation, provide examples, and explain the "why" behind standards. Together we build better software! 🎉

‎docs/contributing/roadmap-issues.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,27 @@ This verification ensures:
111111

112112
The pre-commit script is the single source of truth for all quality checks. By including it in acceptance criteria, we make it clear that passing these checks is **required**, not optional.
113113

114+
#### Acceptance Criteria as PR Review Checklist
115+
116+
Acceptance criteria serve a dual purpose in our workflow:
117+
118+
1. **For Issue Authors**: They define what "done" means for the task
119+
2. **For Contributors**: They provide a pre-review checklist before submitting PRs
120+
121+
Both issue templates ([SPECIFICATION-TEMPLATE.md](../issues/SPECIFICATION-TEMPLATE.md) and [GITHUB-ISSUE-TEMPLATE.md](../issues/GITHUB-ISSUE-TEMPLATE.md)) include a note that reminds contributors:
122+
123+
> **Note for Contributors**: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations.
124+
125+
**Benefits of this approach**:
126+
127+
- **Clear expectations**: Contributors know exactly what reviewers will check
128+
- **Self-review**: Encourages contributors to verify their work before submission
129+
- **Faster merges**: Reduces back-and-forth in PR reviews by catching issues early
130+
- **Quality consistency**: Ensures all PRs meet the same standards
131+
- **Learning opportunity**: New contributors learn project standards through acceptance criteria
132+
133+
When creating issues, think about what you would check when reviewing the PR. Make acceptance criteria specific, measurable, and aligned with the project's quality standards as documented in the [PR Review Guide](./pr-review-guide.md).
134+
114135
### Step 2: Create GitHub Epic Issue (if needed)
115136

116137
If this is the first task in a roadmap section, create an epic issue first:

‎docs/issues/GITHUB-ISSUE-TEMPLATE.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ See detailed specification: [docs/issues/{number}-{name}.md](../docs/issues/{num
4747

4848
## Acceptance Criteria
4949

50+
> **Note for Contributors**: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations.
51+
5052
**Quality Checks**:
5153

5254
- [ ] Pre-commit checks pass: `./scripts/pre-commit.sh`

‎docs/issues/SPECIFICATION-TEMPLATE.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@
6767

6868
## Acceptance Criteria
6969

70+
> **Note for Contributors**: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations.
71+
7072
**Quality Checks**:
7173

7274
- [ ] Pre-commit checks pass: `./scripts/pre-commit.sh`

0 commit comments

Comments
 (0)