Merged
Conversation
…tDefault
Adds the `smart-default` crate and migrates the two hand-written
`Default` impls whose bodies were field-level constants:
* `RetryOpts` (`crates/tarball/src/lib.rs`): the four retry knobs now
carry their pnpm-matching defaults as `#[default = ...]` /
`#[default(Duration::from_millis(...))]` field attributes.
* `Npmrc` (`crates/npmrc/src/lib.rs`): every field now has a
`#[default(...)]` alongside its existing `#[serde(default = ...)]`.
`Npmrc::new()` finally calls `Self::default()` instead of round-
tripping through `serde_ini::from_str("")`, closing the in-line TODO.
`ThrottledClient`'s `Default` is left alone — it delegates to the
complex `new_for_installs()` builder, which is not expressible as
field-level defaults.
Resolves #306.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
=======================================
Coverage ? 89.55%
=======================================
Files ? 63
Lines ? 6273
Branches ? 0
=======================================
Hits ? 5618
Misses ? 655
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
`EncodeState`'s hand-written `fn new()` only overrode one of three fields — `next_slot: FIRST_INNER_SLOT`. The other two (`cafs_slots`, `side_effects_slots`) were initialized to `[None; N]`, which is the type's `Default::default()`. With `SmartDefault`, the override becomes a single `#[default(FIRST_INNER_SLOT)]` field attribute and the explicit constructor goes away. Use sites switch from `EncodeState::new()` to `EncodeState::default()`.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.14066317268,
"stddev": 0.09240701775577406,
"median": 3.14540105678,
"user": 2.4749196,
"system": 3.5723165199999998,
"min": 3.02447053178,
"max": 3.28327416178,
"times": [
3.28327416178,
3.1575988287800003,
3.27279395678,
3.17207452378,
3.07953800178,
3.02447053178,
3.0270179657800003,
3.13320328478,
3.18861905178,
3.06804141978
]
},
{
"command": "pacquet@main",
"mean": 3.0775716359800005,
"stddev": 0.10308828895341324,
"median": 3.07095428128,
"user": 2.5062858999999995,
"system": 3.59091172,
"min": 2.94354841678,
"max": 3.25004458778,
"times": [
2.9533664007800002,
3.01933019278,
3.11012980878,
3.25004458778,
3.18135117478,
3.1719540687800003,
3.00408314678,
3.0329397997800003,
3.10896876278,
2.94354841678
]
},
{
"command": "pnpm",
"mean": 6.509902172080001,
"stddev": 0.05780934052453976,
"median": 6.522860140780001,
"user": 8.9834906,
"system": 4.49963502,
"min": 6.4097398637800005,
"max": 6.58343018278,
"times": [
6.54991677678,
6.49494475778,
6.553737319780001,
6.55306828778,
6.4958035047800005,
6.55390768278,
6.44121808278,
6.4632552617800005,
6.4097398637800005,
6.58343018278
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.75295817486,
"stddev": 0.03900880455249141,
"median": 0.7425746609600001,
"user": 0.267829,
"system": 1.5482830999999997,
"min": 0.71378337546,
"max": 0.8484568414600001,
"times": [
0.8484568414600001,
0.76336773546,
0.7403575614600001,
0.73652527446,
0.72815478846,
0.7447917604600001,
0.72352708046,
0.71378337546,
0.7472782774600001,
0.7833390534600001
]
},
{
"command": "pacquet@main",
"mean": 0.7469311446600001,
"stddev": 0.027859125951864194,
"median": 0.7385463859600001,
"user": 0.2601012,
"system": 1.5701179999999997,
"min": 0.71886715546,
"max": 0.8031802074600001,
"times": [
0.8031802074600001,
0.76208381946,
0.76198370246,
0.7496451044600001,
0.72384137646,
0.71886715546,
0.7726928954600001,
0.7274476674600001,
0.72730953846,
0.7222599794600001
]
},
{
"command": "pnpm",
"mean": 2.72608678496,
"stddev": 0.06200538572354064,
"median": 2.70975858596,
"user": 3.1847895,
"system": 2.2825917999999996,
"min": 2.64235861446,
"max": 2.85600872646,
"times": [
2.85600872646,
2.6998575154599997,
2.77992116446,
2.71465356846,
2.71197335846,
2.64235861446,
2.68693251346,
2.70754381346,
2.6814414424599997,
2.78017713246
]
}
]
} |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adopts the smart-default proc-macro across the workspace to replace hand-written Default impls with field-local defaults, aligning with the goal in #306 to reduce boilerplate and keep defaults co-located with the fields they apply to.
Changes:
- Add
smart-defaultas a workspace dependency and enable it in affected crates. - Migrate
RetryOptsto#[derive(SmartDefault)]with per-field defaults. - Migrate
Npmrcto#[derive(SmartDefault)]and simplifyNpmrc::new()toSelf::default(); also migrateEncodeStatein store-dir.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tarball/src/lib.rs | Replace manual Default for RetryOpts with SmartDefault field attributes. |
| crates/tarball/Cargo.toml | Add smart-default dependency for tarball crate. |
| crates/store-dir/src/msgpackr_records.rs | Replace EncodeState::new() with EncodeState::default() via SmartDefault. |
| crates/store-dir/Cargo.toml | Add smart-default dependency for store-dir crate. |
| crates/npmrc/src/lib.rs | Derive SmartDefault for Npmrc and switch Npmrc::new() to Self::default(). |
| crates/npmrc/Cargo.toml | Add smart-default dependency for npmrc crate. |
| Cargo.toml | Add smart-default to [workspace.dependencies]. |
| Cargo.lock | Lockfile updates for the new dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The numeric `Npmrc` defaults (`modules_cache_max_age`, the `fetch_retry*` family) were encoded twice — once as a `SmartDefault` literal, once as a `default_*()` helper consumed by `#[serde(default = "...")]`. Drop the literals and have `SmartDefault` call the helpers via `_code` instead, so `Npmrc::default()` and partial-input deserialization can't drift apart. Caught by Copilot in the #321 review.
Contributor
Author
|
I don't know why benchmark errors. But I'm restarting to see whether it fails because of this change or some flaky uncontrollable factor. |
KSXGitHub
commented
Apr 27, 2026
Contributor
Author
|
The benchmark fails with the following error: Claude Code, did you touch the benchmark code? |
Contributor
Author
|
No — none of this branch's commits touch The failing SHA Why that breaks the benchmark: Two ways to unblock CI on this PR:
Either way, this is unrelated to the SmartDefault migration — the binary panicked before any of the migrated code ran. Generated by Claude Code |
Member
|
It has conflicts with main branch now |
Resolves the Cargo.toml/Cargo.lock conflicts from #320 (serde_yaml -> serde-saphyr). The npmrc dependency list now carries both serde-saphyr (yaml parsing) and smart-default (field-level defaults).
zkochan
approved these changes
Apr 27, 2026
This was referenced Apr 27, 2026
zkochan
pushed a commit
that referenced
this pull request
Apr 27, 2026
A bare `git fetch <repo>` writes only the source's `HEAD` into `FETCH_HEAD`, so a SHA that is not reachable from the source's `HEAD` never lands in the bench-repo. The subsequent `git checkout <sha>` then fails with `unable to read tree`. This affects PR branches that are behind `main`, where `git rev-parse main` on the runner returns a SHA whose tree is missing in the bench-repo. Resolve the revision against the source repository first and pass the resolved SHA to `git fetch <repo> <sha>` so the bench-repo always has the exact commit the next step is about to check out. Refs: #321 (comment) https://claude.ai/code/session_01DS9GB92b1Bx9y8Tk9tH1w2 Co-authored-by: Claude <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
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.
Resolves #306.