install: collapse dependent bool groups into enums on CommandLineArguments - #36764
Conversation
…ments
Replace two groups of mutually-exclusive bools with the enums they
already fold into:
- Update{development,optional,peer} on Options and the matching
development/optional/peer bools on CommandLineArguments become the
existing bun_install_types::DependencyGroup. The if/else-if cascades
that picked exactly one of devDependencies/optionalDependencies/
peerDependencies/dependencies collapse to a single field read.
- silent/quiet/verbose on CommandLineArguments become LogLevel directly
(the type Options.log_level already carries). Options::load then just
folds in the no-progress bit via LogLevel::without_progress() instead
of a four-branch cascade.
Checking --silent first means LogLevel::is_silent() matches the --silent
flag exactly, so the four callers that previously read cli.silent to
suppress output behave the same. The one observable change is that
--silent now wins over an env-derived verbose (RUNNER_DEBUG=1), so
'bun install --silent' is actually silent there instead of leaking
verbose lockfile diagnostics.
|
Warning Review limit reached
Next review available in: 31 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean refactor collapsing mutually-exclusive bool groups into their existing enums.
What was reviewed:
DependencyGroupprecedence (dev > optional > peer > dependencies) and.propvalues match the removed cascades exactly.LogLevelprecedence flip (--silentnow beats--verbose/RUNNER_DEBUG) is intentional, documented, and covered by the new test;is_silent()semantics preserved for the fourcli.silentcallers.- Grepped for remaining readers of the removed fields (
cli.{silent,quiet,verbose,development,optional,peer},update.{development,optional,peer}) — none.pack_commandreadsoptions.log_level(unaffected). without_progress()+set_verbose_install(is_verbose())reproduce the oldOptions::loadcascade for all flag combinations except the documented precedence change.
Extended reasoning...
Overview
This PR collapses two groups of mutually-exclusive bools on install CLI types into the enums they already fold into:
CommandLineArguments::{development,optional,peer}andOptions::Update{development,optional,peer}→DependencyGroup(an existing type inbun_install_typeswith.propcarrying the package.json key). The if/else-if cascade inupdatePackageJSONAndInstallbecomes a single.propread.CommandLineArguments::{silent,quiet,verbose}→LogLevel(already the type onOptions). AddsLogLevel::is_silent()and::without_progress().Options::loadnow copiescli.log_leveland folds in the no-progress bit instead of re-deriving from three bools.
Touches 7 source files (CLI arg parsing, options load, one consumer, three command entry points, and a Default impl on DependencyGroup) plus one new test.
Security risks
None. This is internal representation of CLI flags for bun install and related subcommands. No auth, crypto, network, or untrusted-input parsing paths are affected.
Level of scrutiny
Moderate — mechanical refactor with one intentional user-facing precedence change. I traced both refactors end-to-end:
- DependencyGroup: The old parse set three independent bools; the old load picked with precedence dev > optional > peer; the old consumer mapped to the same four package.json keys
DependencyGroup::{DEV,OPTIONAL,PEER,DEPENDENCIES}.propcarries. The new parse encodes the same precedence directly. Behavior-identical. - LogLevel: The precedence changed from verbose > silent > quiet (in
Options::load) to silent > verbose > quiet (inparse). This meansRUNNER_DEBUG=1 bun install --silent(and explicit--silent --verbose) now yieldsSilentinstead ofVerbose. The PR documents this as a fix (silent should mean silent), and adds a test. The four sites that previously readcli.silentdirectly (Options::loadsummary gate, outdated, publish, update -i) now callis_silent(); because--silentis checked first,is_silent()is true exactly when--silentwas passed — matching the oldcli.silentbool.set_verbose_installandwithout_progress()reproduce the rest of the old cascade.
I grepped src/ for any remaining readers of the removed fields — none. pack_command.rs reads manager.options.log_level == LogLevel::Quiet, which is still populated correctly.
Other factors
- No CODEOWNERS on these paths.
- Existing test coverage: PR description reports
bun-add.test.ts(--dev/--optional/--peer) andbun-pack.test.ts(--silent/--quiet) pass unchanged. - New test asserts the RUNNER_DEBUG + --silent case produces empty stdout/stderr.
- Net -16 lines, deletes the
Updatestruct entirely (dead after this change). - The bug hunter found no issues.
|
Self-review probed 30 angles and found no surviving concerns. The existing |
2 participants
What
Two groups of mutually-exclusive bools on the install CLI types become the enums they already fold into:
Update{development,optional,peer}→DependencyGroupstruct UpdateonOptionswas three bools, at most one ever set (via anif/else ifcascade), consumed by an identical cascade inupdatePackageJSONAndInstallto pick one ofdependencies/devDependencies/optionalDependencies/peerDependencies.bun_install_types::DependencyGroupalready exists with exactly these four constants and a.propfield carrying the package.json key. The struct is removed, the field becomesDependencyGroup, and the consumer cascade becomes a single.propread.silent/quiet/verbose→LogLevelCommandLineArgumentscarried three separate bools thatOptions::loadfolded intoLogLevelvia a four-branch cascade. The CLI now storeslog_level: LogLeveldirectly;Options::loadjust folds in the no-progress bit via the newLogLevel::without_progress().--silentis checked first in the precedence, soLogLevel::is_silent()is true exactly when--silentwas passed. The four callers that previously readcli.silentto suppress summaries/errors (Options::load,bun outdated,bun publish,bun update -i) are routed throughis_silent()and behave identically.Why
Making illegal states unrepresentable:
Update{development:true, peer:true}was constructible but meaningless, and there were two parallel encodings of the same four-way choice (UpdatevsDependencyGroup). Net -16 lines with the cascades gone.The
LogLevelchange has one observable effect: previouslyOutput::is_verbose()(set byRUNNER_DEBUG=1in GitHub Actions) would win over an explicit--silentfor the log level, soRUNNER_DEBUG=1 bun install --silentleaked verbose lockfile diagnostics to stderr:while still suppressing the summary. With
--silentchecked first it is now fully silent, which is what--silentpromises.Verification
RUNNER_DEBUG=1 bun install --silentproduces no output (fails on main, passes here).bun-add.test.ts(54 tests, covers--dev/--optional/--peer) andbun-pack.test.ts(76 tests, covers--silent) pass unchanged.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts