Skip to content

Commit 533a700

Browse files
Copilotjosecelano
Copilot
and
josecelano
committed
docs: [#80] add comprehensive PR review guide for contributors
Co-authored-by: josecelano <[email protected]>
1 parent 06f3e0a commit 533a700

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)