refactor(pacquet/config): replace default callback with self#11736
Conversation
… a mut-self builder
The closure was invoked unconditionally on the first line, so the laziness
it advertised was never exercised. Production passed `Default::default`,
17 of 18 test callers passed `Config::new`, and a single test passed a
custom closure only to seed a non-default starting value.
Replace it with a builder-style `current_dir(mut self, start_dir)` method.
Callers build the starting `Config` themselves and the method layers
`.npmrc` and `pnpm-workspace.yaml` on top. The lone outlier test becomes
`Config { symlink: false, ..Config::new() }.current_dir::<H>(...)`.
Resolves #11731.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThis PR refactors ChangesConfig::current builder pattern migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11736 +/- ##
=======================================
Coverage 89.85% 89.85%
=======================================
Files 145 145
Lines 16424 16426 +2
=======================================
+ Hits 14757 14759 +2
Misses 1667 1667 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous commit renamed the loader to current_dir; revert the name back to Config::current. Only the entry shape (builder-style mut self instead of a default callback) is intentional — the rename was not.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4392141665,
"stddev": 0.0763076835019069,
"median": 2.4175111162,
"user": 2.6485406999999994,
"system": 3.7357871000000005,
"min": 2.3421295267,
"max": 2.5658202906999996,
"times": [
2.5233206716999996,
2.3421295267,
2.4876508506999997,
2.4102093907,
2.5658202906999996,
2.5075095967,
2.3859676157,
2.4248128416999997,
2.3676577326999997,
2.3770631477
]
},
{
"command": "pacquet@main",
"mean": 2.4024879022,
"stddev": 0.04312745452273411,
"median": 2.4146350252,
"user": 2.6116134,
"system": 3.7462828000000004,
"min": 2.3319620017,
"max": 2.4724483456999997,
"times": [
2.3895882836999998,
2.4216151246999997,
2.4234152066999997,
2.3319620017,
2.4724483456999997,
2.4164149836999997,
2.3453527306999997,
2.3715855857,
2.4396416927,
2.4128550666999997
]
},
{
"command": "pnpm",
"mean": 4.8533832765,
"stddev": 0.03661553512910324,
"median": 4.8444775366999995,
"user": 8.1632713,
"system": 4.2837559,
"min": 4.8077602836999995,
"max": 4.9064369057,
"times": [
4.8083310337,
4.9015440707,
4.9064369057,
4.8619727317,
4.8371334097,
4.8518216637,
4.8934660687,
4.829760880699999,
4.8356057167,
4.8077602836999995
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.67919360546,
"stddev": 0.0624314253049751,
"median": 0.6602758077599999,
"user": 0.3687704,
"system": 1.5528836600000002,
"min": 0.64998060576,
"max": 0.85562461676,
"times": [
0.85562461676,
0.66167595576,
0.66365376876,
0.65842749976,
0.65232133576,
0.66148420076,
0.65906741476,
0.64998060576,
0.65331788876,
0.67638276776
]
},
{
"command": "pacquet@main",
"mean": 0.69406727646,
"stddev": 0.03622546331243175,
"median": 0.67958125776,
"user": 0.3941126,
"system": 1.55855056,
"min": 0.66316180476,
"max": 0.75797928376,
"times": [
0.75205933876,
0.66859558576,
0.67912486576,
0.72221584376,
0.6662228717600001,
0.68240996976,
0.66886555076,
0.75797928376,
0.68003764976,
0.66316180476
]
},
{
"command": "pnpm",
"mean": 2.65374178926,
"stddev": 0.0585076242150254,
"median": 2.65083848076,
"user": 3.2331701,
"system": 2.2706366599999996,
"min": 2.56519553376,
"max": 2.79030943176,
"times": [
2.6775434587599998,
2.64918364876,
2.63233325076,
2.79030943176,
2.6795412707599997,
2.60996712976,
2.65333769176,
2.65249331276,
2.56519553376,
2.6275131637599998
]
}
]
} |
There was a problem hiding this comment.
Pull request overview
Refactors Config::current from a function taking a default: FnOnce() -> Config closure into a builder method taking mut self. The previous closure was invoked unconditionally on the first line, so its laziness was never used, and 18 of 19 callers passed Config::new/Default::default. The new shape lets callers construct the starting Config directly and chain .current::<Sys>(&dir).
Changes:
- Reshape
Config::currentsignature: drop theDefaultgeneric and closure parameter, takemut self, rename internalconfig→self. - Update the single production call site in
cli_args.rstoConfig::default().current::<Host>(&dir). - Update all test call sites; the one outlier now uses
Config { symlink: false, ..Config::new() }.current::<HostWithHome>(...).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pacquet/crates/config/src/lib.rs | Convert Config::current to a mut self builder; update all in-module test callers. |
| pacquet/crates/config/src/api.rs | Update doc examples to the new Config::default().current::<Host>(&dir) form. |
| pacquet/crates/cli/src/cli_args.rs | Update the production call site to the builder form. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Config::current'sdefaultcallback was invoked unconditionally on the function's first line, so the laziness it advertised was never exercised. The single production caller passedDefault::default, and 17 of 18 test callers passedConfig::new. Only one test passed a custom closure — to seed a non-default startingConfig.Reshape
Config::currentinto a builder that takesmut self. Callers build the startingConfigthemselves; the method layers.npmrcandpnpm-workspace.yamlon top. The method name stayscurrent.Config::default().current::<Host>(&dir)Config { symlink: false, ..Config::new() }.current::<H>(...)Out of scope: the inner layering logic — only the entry shape changes.
Resolves #11731.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit