install: name build_store's timing flag with an enum instead of a bool - #39157
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
WalkthroughThe change replaces boolean timing control with a ChangesIsolated install timing control
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. The diff is green; the remaining CI red is unrelated to it.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Confirmed
install_isolated_packagesandprune::build_store_withare the only callers ofisolated_install::build_store; both updated with the same semantics (is_verbose()→Print,false→Quiet). - The two
matches!(timings, Timings::Print)sites are equivalent to the oldif print_timingschecks. - Baseline drops: one is the finding this PR fixes; the other two (
PackageInstall.rs,node_crypto_binding.rs) are stale entries cleared by the regeneration — if wrong, the mordant CI job would catch it.
Extended reasoning...
Overview
This PR replaces the print_timings: bool parameter of build_store in src/install/isolated_install.rs with a two-variant Timings enum (Print/Quiet), updates both call sites, and regenerates mordant-baseline.toml. The regeneration drops the bare_bool_args:src/install/isolated_install.rs entry this PR fixes plus two unrelated stale entries whose underlying code changed in already-merged PRs.
Security risks
None. This is a type-only refactor of an internal function signature in the package manager's isolated linker. No user input handling, no I/O, no auth/crypto surface is touched.
Level of scrutiny
Low. The change is purely mechanical: a bool becomes an enum, and if print_timings becomes if matches!(timings, Timings::Print). I grepped for all callers of isolated_install::build_store and confirmed there are exactly two — both updated in this diff with values that preserve the prior behavior (manager.options.log_level.is_verbose() maps to Print/Quiet; prune's hardcoded false becomes Quiet). The unrelated build_store in BunObject.rs and Timings in test_command.rs are different symbols.
Other factors
The PR description documents that the isolated-install, prune, relink, and frozen-lockfile test suites pass and that verbose output was manually verified unchanged. The two extra baseline drops are the output of bun run rust:mordant:baseline and are self-checking: if either were dropped incorrectly, the rust-lints CI job would report over-baseline findings. The bug-hunting system found no issues.
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Both
build_storecallers map their old bool to the matchingTimingsvariant (is_verbose()→ Print/Quiet; prune'sfalse→ Quiet); no other install-side callers exist. - The two
matches!(timings, Timings::Print)gates are the only sites producing "Resolved peers"/"Created store", so the new tests' stderr assertions are anchored to real output. - New tests follow local harness conventions (concurrent pipe drain,
registry.createTestDir/setup/prunehelpers) and cover both the positive (--verbose prints) and negative (quiet install, prune --verbose) cases. mordant-baseline.tomldrops the fixed entry plus two stale entries from a regen — CI's mordant job would fail if those findings still existed.
Extended reasoning...
Overview
This PR replaces the print_timings: bool parameter of build_store in src/install/isolated_install.rs with a two-variant Timings enum (Print/Quiet), addressing a bare_bool_args mordant lint finding. The two call sites — install_isolated_packages in the same file and build_store_with in src/install/prune.rs — are updated to pass the enum instead. Two new tests pin the observable behavior (timing lines only under --verbose install, never under prune). mordant-baseline.toml is regenerated, dropping the fixed entry and two entries that were already stale on main.
Security risks
None. This is a type-level refactor of an internal function parameter that gates two verbose-only log lines. No user input handling, no I/O paths, no permission or auth surface is touched.
Level of scrutiny
Low. The change is a textbook bool→enum readability refactor with a 1:1 mapping at every site. I confirmed via grep that the only install-side callers of build_store are the two updated in this diff (the build_store in BunObject.rs is unrelated stdio code). The two if matches!(timings, Timings::Print) branches replace if print_timings verbatim, so behavior is preserved exactly. The install_isolated_packages caller previously passed manager.options.log_level.is_verbose() and now branches on the same predicate to pick Print/Quiet; prune previously passed false and now passes Quiet.
Other factors
- The new tests are well-formed per repo conventions: they drain stdout/stderr/exited concurrently, use the existing
setup/prune/outhelpers in bun-prune.test.ts andregistry.createTestDirin isolated-install.test.ts, assert output before exit code, and cover both the positive (verbose install prints both lines) and negative (non-verbose install andbun prune --verboseprint neither) cases. The prune test uses--productionso prune builds the store twice, exercising bothbuild_store_withinvocations. - The two extra baseline entries dropped (
always_unwrapped_option:PackageInstall.rs,narrowed_two_ways:node_crypto_binding.rs) are explained in the PR body as already-gone findings; the mordant CI job enforces this. - No outstanding reviewer comments; no prior claude reviews on this PR.
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed
- Confirmed
build_storehas exactly two call sites (install_isolated_packages, prune'sbuild_store_with); both updated with the same semantics as before (is_verbose()→Print,false→Quiet). Timingsispub(crate)and correctly re-imported inprune.rs; the twomatches!(timings, Timings::Print)sites are the only uses of the old bool.- New tests follow their files' conventions (
test.concurrentin bun-prune, plaintestin isolated-install), drain both pipes, use the local Verdaccio registry, and assert on the exact timing-line prefixes emitted bybuild_store. mordant-baseline.tomlchanges are removals only; the two extra dropped entries are explained as already-gone-on-main.
Extended reasoning...
Overview
This PR replaces the print_timings: bool parameter of build_store in src/install/isolated_install.rs with a two-variant Timings enum (Print / Quiet) to resolve a bare_bool_args mordant lint finding. The two call sites are updated: install_isolated_packages maps log_level.is_verbose() to Timings::Print/Timings::Quiet (previously passed the bool directly), and prune's build_store_with passes Timings::Quiet (previously false). Two tests pin the existing behavior at each call site. mordant-baseline.toml is regenerated, dropping the fixed entry plus two entries that are already stale on main.
Security risks
None. The change is a type-level rename of a debug-output flag; no user input handling, no I/O, no auth/crypto/permissions paths are touched.
Level of scrutiny
Low. This is a pure refactor: the enum is compared with matches!(timings, Timings::Print) at exactly the two sites that previously tested if print_timings, and the two callers pass values that map one-to-one to the old bools. Grep confirms there are no other build_store( call sites in src/install. The remaining bool parameter (install_root_dependencies) is intentionally left alone with a stated reason (threaded as a bool through several other layers).
Other factors
- The new tests use the existing harness helpers (
setup/prune/outin bun-prune.test.ts;registry.createTestDir/spawn/rm/joinin isolated-install.test.ts), all of which are already imported at file top. Both drain stdout+stderr concurrently and assert exit code last, per repo conventions. - The prune test's inline snapshot matches the shape of other snapshots in the same file (normalized via
out()). - No CODEOWNERS cover the touched paths.
- The baseline regeneration only removes entries (never adds), so it cannot mask new lint findings.
1aa0560 to
afe3dcd
Compare
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Confirmed
install_isolated_packagesand prune'sbuild_store_withare the only two callers ofisolated_install::build_store; both map to the same behavior as before (is_verbose()→Print,false→Quiet). - Both
if matches!(timings, Timings::Print)sites are equivalent to the priorif print_timingschecks. - New tests use existing file-local helpers/imports, drain pipes concurrently, and assert exit code last per repo conventions.
Extended reasoning...
Overview
This PR replaces build_store's trailing print_timings: bool parameter with a two-variant Timings enum (Print/Quiet) to resolve a mordant bare_bool_args lint finding. The two call sites — install_isolated_packages in isolated_install.rs and build_store_with in prune.rs — are updated to pass the enum equivalent of what they passed before. The corresponding baseline entry is removed from mordant-baseline.toml, and two tests are added pinning the existing timing-output behavior at each call site.
Security risks
None. This is a type-level refactor of an internal function signature controlling whether two diagnostic timing lines print to stderr. No untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Low. The change is mechanical and behavior-preserving by construction: manager.options.log_level.is_verbose() maps to Timings::Print (else Quiet), and prune's literal false maps to Timings::Quiet. The two if print_timings checks become if matches!(timings, Timings::Print), which is semantically identical. I grep-verified there are no other callers of this build_store (the BunObject.rs hit is an unrelated local function).
Other factors
The added tests follow the surrounding file conventions: bun-prune.test.ts uses the file's existing setup/prune/out helpers and test.concurrent like its neighbors; isolated-install.test.ts uses registry.createTestDir and imports already present at the top of the file. Both drain stdout/stderr/exited concurrently and assert the exit code last. The PR description explains why install_root_dependencies remains a bool (threaded widely elsewhere) and documents that mordant was verified both ways. No outstanding reviewer comments.
2 participants
Problem
build_storeinsrc/install/isolated_install.rstakes two bools,install_root_dependenciesandprint_timings. Its call insrc/install/prune.rs(build_store_with) readsbuild_store(&*manager, &manager.lockfile, true, &[], None, false): nothing at the call site says which flag is which, and swapping them would still compile.bare_bool_argsfinding baselined for that file.Fix
print_timings: boolbecomestimings: Timings, a two-variant enum (Timings::Print/Timings::Quiet) declared next tobuild_store.install_isolated_packagespassesPrintwhen the log level is verbose (the condition it passed as a bool before); prune passesQuiet(wasfalse).install_root_dependenciesstays a bool. It is threaded as a bool through the hoisted installer,Tree::Builder,Lockfile::hoist/filterandis_filtered_dependency_or_workspaceas well, so changing its type is a separate, wider refactor. With one bool left there is nothing to confuse it with, and the lint no longer applies tobuild_store.isolated-install.test.ts("store build timings are printed by --verbose only") checks thePrint/Quietchoice ininstall_isolated_packages, andbun-prune.test.ts("isolated linker: --verbose does not print the store build timings") checks prune'sQuiet. They pin the existing behavior, so they pass before and after the refactor.mordant-baseline.tomlloses thebare_bool_argsentry forsrc/install/isolated_install.rs, the only entry this finding accounted for (bun run rust:mordant:baselineon this branch produces the same file).bun run rust:mordanton the previous source with the baseline entry removed reports thebuild_storefinding (over-baseline.txt:bun_install 1); with this change and the regenerated baseline it reports nothing across the workspace.bun bd test test/cli/install/bun-prune.test.ts test/cli/install/isolated-install.test.ts(177 pass, 1 skip, including the two new tests)bun bd test test/cli/install/isolated-relink.test.ts test/cli/install/frozen-lockfile-pruned.test.ts(107 pass, 1 skip)USE_SYSTEM_BUN=1), as expected for a refactor. That older build has nobun pruneyet, so the prune test was only run against this branch.Background
build_storeturns the lockfile into the isolated linker's store layout (node_modules/.bun/...) in two passes, resolving peers and then deduplicating entries. A verbose install prints how long each pass took.bun prunealso builds the store (up to twice, with different feature sets) only to learn which store entry names are still wanted, and does not want those lines.rust-lintsworkflow.mordant-baseline.tomlrecords, per lint and file, how many findings predate the job; a run fails only on findings above those counts, so fixing a baselined finding means removing its entry.