refactor: optional DI pattern + documentation#517
Conversation
…t the pattern PRs #332, #333, and #337 introduced a dependency-injection seam for side-effecting code: one trait per capability, one zero-sized provider gathering every impl, all methods static, generic threaded through the call site. The pattern lived only in PR comments. Subsequent PRs kept rediscovering it from scratch and drifting on the details. This commit picks one consistent set of identifiers, applies them across every affected crate, and writes the pattern down in `CODE_STYLE_GUIDE.md` so future PRs have a single reference point. Identifier choices, with the nits they address: * `Api` (generic parameter) -> `Sys`. The provider is a system seam, not an API surface — `Api` overloads with the Rust idiom for "the public interface of a crate." * `RealApi` (production type) -> `Host`. The `Real` prefix is a Platonic-prefix anti-pattern: it implies the unqualified name belongs to something else. Convention runs the other way — production is unqualified, fakes carry the qualifier. * `FsReadString` (cmd-shim) -> `FsReadToString`, matching `std::fs::read_to_string` and the trait that already shipped in `modules-yaml`. Trait names stay domain-prefixed (`Fs*`, `Env*`, `Get*`). The new `### Dependency injection for tests` section in `CODE_STYLE_GUIDE.md` lists the eight principles, walks through the filesystem capabilities in `modules-yaml` as the canonical example, covers cross-domain composition under one generic, and spells out when a real fixture is simpler than a fake. `AGENTS.md` gains a pointer so agents working in the repo find the section before reaching for `&self` or a lumped `FsApi`. Crates touched: `modules-yaml`, `cmd-shim`, `config`, `reporter`, `package-manager`, `cli`. The consolidation step the issue asks about (folding the four `Host` declarations into a shared `crates/api`) is deliberately deferred to a follow-up. The structures are similar enough to consolidate, but the move involves a new crate, redistributing the `walkdir` / `httpdate` / `gethostname` dependency footprint, and updating every import — a clean diff on its own. Renaming first is the prerequisite that follow-up needs. Resolves <#339>.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
# Conflicts: # crates/config/src/lib.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #517 +/- ##
=======================================
Coverage 88.51% 88.51%
=======================================
Files 125 125
Lines 13821 13821
=======================================
Hits 12234 12234
Misses 1587 1587 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.6733645010400005,
"stddev": 0.09548016326925188,
"median": 2.6731865101400003,
"user": 2.55109312,
"system": 3.7230563000000005,
"min": 2.55325610614,
"max": 2.8237600301400003,
"times": [
2.7761917131400002,
2.6840690941400003,
2.56901684514,
2.6307872421400003,
2.55859278014,
2.8237600301400003,
2.7396514991400003,
2.73601577414,
2.6623039261400003,
2.55325610614
]
},
{
"command": "pacquet@main",
"mean": 2.66753851794,
"stddev": 0.07411069117451419,
"median": 2.6932574911400002,
"user": 2.58963202,
"system": 3.7251929999999995,
"min": 2.55482195614,
"max": 2.75723878314,
"times": [
2.7046758091400003,
2.68183917314,
2.74672049714,
2.75723878314,
2.6018308081400003,
2.71230856814,
2.5600464631400004,
2.64098095214,
2.7149221691400003,
2.55482195614
]
},
{
"command": "pnpm",
"mean": 6.42631451674,
"stddev": 0.1449489518571447,
"median": 6.3986031171399995,
"user": 9.528358720000002,
"system": 4.6376139,
"min": 6.19885972214,
"max": 6.76219161714,
"times": [
6.3988835891399996,
6.5341519651399995,
6.76219161714,
6.42440300914,
6.395611243139999,
6.3983226451399995,
6.3790490351399995,
6.43543656414,
6.19885972214,
6.33623577714
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7487266933400001,
"stddev": 0.03150065700234505,
"median": 0.74426499234,
"user": 0.3743571,
"system": 1.6388593,
"min": 0.7171887663400001,
"max": 0.82523524334,
"times": [
0.82523524334,
0.76113552434,
0.75723221634,
0.75302263834,
0.73550734634,
0.72748347134,
0.7171887663400001,
0.75888813134,
0.73107297134,
0.7205006243400001
]
},
{
"command": "pacquet@main",
"mean": 0.8565669222400001,
"stddev": 0.06709994571403975,
"median": 0.8383688448400001,
"user": 0.3710002,
"system": 1.6309460000000002,
"min": 0.78892413034,
"max": 0.97890339534,
"times": [
0.97890339534,
0.78892413034,
0.81378832934,
0.9710498213400001,
0.84386080334,
0.79689837034,
0.8204183943400001,
0.8750882883400001,
0.8405647083400001,
0.8361729813400001
]
},
{
"command": "pnpm",
"mean": 2.65351678644,
"stddev": 0.06678698511354647,
"median": 2.64289430484,
"user": 3.3091628,
"system": 2.2741622999999995,
"min": 2.58870333434,
"max": 2.7794539933399998,
"times": [
2.64807091034,
2.58870333434,
2.75719482034,
2.63771769934,
2.5980847963400002,
2.60309575534,
2.59684902634,
2.7794539933399998,
2.65804353534,
2.6679539933400003
]
}
]
} |
# Conflicts: # crates/package-manager/src/install_frozen_lockfile.rs
Summary
Pick a coherent set of identifiers for the dependency-injection seam introduced in PRs #332, #333, and #337, apply them across every affected crate, and write the pattern down in
CODE_STYLE_GUIDE.mdso future PRs have a single reference point.Naming:
Api(generic parameter) →Sys. The provider is a system seam, not an API surface.RealApi(production type) →Host. Drops the PlatonicRealprefix; production stays unqualified, fakes carry the qualifier.FsReadString(cmd-shim) →FsReadToString, unifying with the trait already shipped inmodules-yamland matchingstd::fs::read_to_string.Documentation:
### Dependency injection for testssection inCODE_STYLE_GUIDE.md(near### Unit test file layout) — the eight principles, a worked filesystem example anchored tocrates/modules-yaml, cross-domain composition under one generic, and when a real fixture is simpler than a fake.AGENTS.mdgains a pointer to the new section.Crates touched:
modules-yaml,cmd-shim,config,reporter,package-manager,cli.Consolidation evaluation: Deferred to a follow-up. The four
Hostdeclarations are similar enough to fold into a sharedcrates/api, but the move involves a new crate plus redistributing thewalkdir/httpdate/gethostnamedependency footprint — a cleaner diff on its own. Renaming first is the prerequisite that follow-up needs.Test plan
cargo fmtcargo check --lockedcargo clippy --locked -- --deny warningsmodules-yaml,cmd-shim,config,reporterpass locally.Resolves #339.
Written by an agent (Claude Code, claude-opus-4-7).