docs(adr): initialize Architecture Decision Records#535
Merged
nanotaboada merged 2 commits intomasterfrom Mar 22, 2026
Merged
Conversation
Add docs/adr/ with 7 ADRs covering key decisions: SQLite as database engine, no Alembic with manual seed scripts, UUID surrogate primary key with v4/v5 split, squad number as mutation key, full replace PUT with no PATCH, in-memory caching with aiocache, and integration-only test strategy. Includes index (README.md), Nygard template, and updates to README, CONTRIBUTING, and CHANGELOG. ADRs for Alembic (#2), PATCH (#461), and unit tests (#490) reference their respective open issues. Closes #482 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Architecture Decision Records (ADRs) and ADR authoring guidance: introduces Changes
Sequence Diagram(s)(omitted — changes are documentation-only and do not introduce new control-flow code) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Out-of-scope changes
Possibly related issues
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #535 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 113 113
=========================================
Hits 113 113
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/adr/0007-integration-only-test-strategy.md (1)
31-34: Decouple this ADR from concrete test file paths.Line 31–Line 34 capture implementation layout, not architecture. Consider wording this as “current structure” (or moving to test docs) so simple test-file reorgs don’t require ADR edits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0007-integration-only-test-strategy.md` around lines 31 - 34, Update the ADR sentence that currently reads "All tests live in `tests/test_main.py`, use stubs from `tests/player_stub.py` for consistent test data, and rely on a `function`-scoped `client` fixture from `conftest.py` for per-test isolation" so it no longer hardcodes concrete test file paths: rephrase to state this as the "current structure" (e.g., "Currently, tests are organized in a single test module, use shared stubs for consistent test data, and rely on a function-scoped client fixture for per-test isolation") or move those implementation details into separate test documentation; keep the ADR focused on the architectural testing strategy rather than specific file locations.docs/adr/0006-in-memory-caching-with-aiocache.md (1)
37-37: Usedelete()instead ofclear()for per-key cache invalidation.Cache invalidation on POST, PUT, and DELETE works correctly in your implementation. However,
clear(CACHE_KEY)is semantically incorrect for removing a single key in aiocache 0.12.3. Theclear()method treats its argument as a namespace prefix and deletes all keys matching that prefix; the correct method for single-key removal isdelete(CACHE_KEY). While your code functionally works (the prefix "players" matches the key "players"), usingdelete()is more idiomatic and prevents brittleness if you add multiple cache keys in the future. Apply this change at lines 78, 223, and 263.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0006-in-memory-caching-with-aiocache.md` at line 37, Replace uses of clear(CACHE_KEY) with delete(CACHE_KEY) for per-key cache invalidation: locate the handlers that invalidate cache on POST, PUT, and DELETE (the code that calls clear(...) with the CACHE_KEY or "players" namespace) and change those calls to use delete(CACHE_KEY) instead so only the single key is removed; ensure you update all occurrences (the three places noted where clear(CACHE_KEY) is used) and keep the same CACHE_KEY symbol and async/await usage around the aiocache client or decorator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0003-uuid-surrogate-primary-key.md`:
- Around line 35-38: Update the ADR text to accurately reflect the
implementation: replace the incorrect references to a non-existent
HyphenatedUUID custom SQLAlchemy type with the actual storage (UUIDs stored as
hyphenated strings/text using the standard UUID/string column), state that
API-created records use UUID v4 (random) and that migration-seeded records use
hardcoded UUID values (not generated as UUID v5 from squad_number), and make the
same corrections where the ADR mentions v5 derivation (including the lines
corresponding to the earlier v5 claim).
In `@docs/adr/0005-full-replace-put-no-patch.md`:
- Around line 26-27: The ADR incorrectly states that "all domain fields are
required"; update the text to match the actual PlayerRequestModel contract by
noting that only first_name, last_name, squad_number and position are required
while middle_name, date_of_birth, abbr_position, team and league are optional;
locate references to PlayerRequestModel in the ADR and replace the incorrect
blanket "all domain fields are required" phrasing with a precise enumeration of
required vs optional fields to reflect the model definition.
In `@docs/adr/README.md`:
- Around line 15-21: The ADR index currently lists ADRs 0001–0007 but the
project objective expects 0001–0008; add a new table row for ADR-0008 in the ADR
index (README.md) with a descriptive title and Accepted/Proposed status and date
matching the other entries, and ensure the actual ADR file for 0008 (0008-...md)
is added to the docs/adr folder; alternatively, if you intentionally reduced
scope, add a brief note in the PR or the README explaining that ADR-0008 was
omitted and link to the decision discussion.
In `@docs/adr/template.md`:
- Around line 9-31: Update the ADR template by adding explicit "## Alternatives
Considered" and "## Trade-offs" sections (in addition to the existing "##
Context", "## Decision", and "## Consequences" headers) so each ADR records
evaluated alternatives and explicit trade-offs rather than relying on commented
guidance; ensure these new headers are real markdown sections (not HTML
comments) and include brief placeholder guidance under "## Alternatives
Considered" to list evaluated options and under "## Trade-offs" to record
pros/cons and known limitations.
---
Nitpick comments:
In `@docs/adr/0006-in-memory-caching-with-aiocache.md`:
- Line 37: Replace uses of clear(CACHE_KEY) with delete(CACHE_KEY) for per-key
cache invalidation: locate the handlers that invalidate cache on POST, PUT, and
DELETE (the code that calls clear(...) with the CACHE_KEY or "players"
namespace) and change those calls to use delete(CACHE_KEY) instead so only the
single key is removed; ensure you update all occurrences (the three places noted
where clear(CACHE_KEY) is used) and keep the same CACHE_KEY symbol and
async/await usage around the aiocache client or decorator.
In `@docs/adr/0007-integration-only-test-strategy.md`:
- Around line 31-34: Update the ADR sentence that currently reads "All tests
live in `tests/test_main.py`, use stubs from `tests/player_stub.py` for
consistent test data, and rely on a `function`-scoped `client` fixture from
`conftest.py` for per-test isolation" so it no longer hardcodes concrete test
file paths: rephrase to state this as the "current structure" (e.g., "Currently,
tests are organized in a single test module, use shared stubs for consistent
test data, and rely on a function-scoped client fixture for per-test isolation")
or move those implementation details into separate test documentation; keep the
ADR focused on the architectural testing strategy rather than specific file
locations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a6bd66a-d424-4ce8-9b1f-9d789688dd64
📒 Files selected for processing (12)
CHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/adr/0001-sqlite-as-database-engine.mddocs/adr/0002-no-alembic-manual-seed-scripts.mddocs/adr/0003-uuid-surrogate-primary-key.mddocs/adr/0004-squad-number-as-mutation-key.mddocs/adr/0005-full-replace-put-no-patch.mddocs/adr/0006-in-memory-caching-with-aiocache.mddocs/adr/0007-integration-only-test-strategy.mddocs/adr/README.mddocs/adr/template.md
- ADR-0003: clarify UUID v5 values are pre-computed constants in seed scripts, not derived at runtime from squad number - ADR-0005: replace "all domain fields are required" with accurate breakdown — 4 required fields, 6 optional - ADR-0007: rephrase hardcoded file paths to generic structure description - template.md: add explicit Alternatives Considered section Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs/adr/with 7 ADRs covering key architectural decisionsdocs/adr/README.md— index tabledocs/adr/template.md— Nygard template for future ADRsREADME.md— Architecture Decisions section + TOC entryCONTRIBUTING.md— ADR section with three-part test andappend-only rule; renumber existing sections
CHANGELOG.md—[Unreleased]entryADRs included
Closes #482
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit