22d0e4f537
Codifies the quality bar already implicit in the CI gates and existing code patterns (crate-boundary SRP, per-domain db modules, test comment style) so it's explicit and reviewable rather than tribal knowledge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PqTJmazHBQK878vjQ4JnnF
76 lines
3.4 KiB
Markdown
76 lines
3.4 KiB
Markdown
# Definition of Done — feedsignal
|
|
|
|
A change is done when all of the following hold, not just when it compiles.
|
|
|
|
## Single Responsibility
|
|
|
|
- Each module/function does one thing. If a PR description needs "and" to
|
|
describe what a function does, split it.
|
|
- Crate boundaries stay honest: `core` has no I/O (see `crates/core` —
|
|
domain models + scoring/affinity math only); `db` only talks to SQLite;
|
|
`feeds` only fetches/parses; `llm` only talks to Ollama. A change that
|
|
makes `core` reach for a DB handle or `db` parse RSS is a sign the code
|
|
landed in the wrong crate.
|
|
- Within `db`, one file per domain concern (`feeds.rs`, `articles.rs`,
|
|
`reading_events.rs`, `affinities.rs`) — a new table gets its own file,
|
|
not a growing `queries.rs`.
|
|
|
|
## DRY, but not premature
|
|
|
|
- No copy-pasted logic across call sites — extract a shared function once
|
|
a second real call site exists, not in anticipation of one.
|
|
- No new abstraction (trait, generic helper, config knob) unless at least
|
|
two concrete cases need it today.
|
|
|
|
## Testing
|
|
|
|
- New logic in `core` (scoring, affinity math) ships with unit tests —
|
|
that crate is the one place pure functions make this cheap, and it's
|
|
already the pattern (`crates/core/src/affinity.rs`).
|
|
- Test rationale goes in a `///` doc comment above `#[test] fn`, not
|
|
inline in the body.
|
|
- A bug fix includes a regression test that fails on the old code.
|
|
- `cargo test --workspace --exclude feedsignal-web` passes locally before
|
|
pushing (CI runs this; `feedsignal-web` itself currently has no test
|
|
target).
|
|
|
|
## Static checks (all of CI, not a subset)
|
|
|
|
- `cargo fmt --check` clean.
|
|
- `cargo clippy --workspace --exclude feedsignal-web --all-targets -- -D warnings` clean.
|
|
- `cargo clippy -p feedsignal-web --no-default-features --features server --all-targets -- -D warnings` clean.
|
|
- `cargo clippy -p feedsignal-web --no-default-features --features web --target wasm32-unknown-unknown -- -D warnings` clean.
|
|
- `cargo audit` clean, or a documented reason if it can't be (e.g. an
|
|
unfixed transitive advisory).
|
|
|
|
## No half-finished work
|
|
|
|
- No `TODO`s standing in for a decision the PR itself was supposed to
|
|
make (a `TODO` marking genuinely deferred, ticketed follow-up work is
|
|
fine — see `LlmConfig`'s config-story TODO).
|
|
- No dead code kept "just in case" — if it's unused, delete it. Library-
|
|
surface exceptions (e.g. `dioxus-primitives`-derived components) get an
|
|
explicit `#[allow(dead_code)]` with a one-line reason, matching the
|
|
existing pattern in `components/button/component.rs`.
|
|
- No commented-out code left in.
|
|
|
|
## Correctness at the boundary
|
|
|
|
- New `#[server]` fns validate/trim input the way `services::feeds::subscribe`
|
|
does (`anyhow::ensure!` on empty URL) — don't trust the browser.
|
|
- DB writes that must be atomic with respect to a failure path (e.g.
|
|
"fetch before writing so a bad URL leaves no half-registered row") get
|
|
a comment explaining why, like `subscribe`'s doc comment does.
|
|
- Migrations are additive/reversible — no editing a shipped migration file.
|
|
|
|
## Docs
|
|
|
|
- A non-obvious "why" (a workaround, a chosen threshold, a design
|
|
tradeoff) gets a comment; "what" is left to the code.
|
|
- `README.md`'s architecture section gets updated if the change adds a
|
|
crate, a pipeline stage, or a scoring input.
|
|
|
|
## PR hygiene
|
|
|
|
- Diff is scoped to the stated task — no drive-by refactors bundled in.
|
|
- `Cargo.lock` changes match what the dependency change actually requires.
|