Repository navigation
Simplify CLI surface and keep nun uninstall - #9
Merged
Merged
Conversation
Replace legacy parsing/config helpers with standard crates, remove interactive flows and unused aliases, keep the node shim, and retain nun as the uninstall command.
|
Warning Review limit reached
More reviews will be available in 18 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors CLI parsing to derive-based clap, migrates config loading from INI to TOML via Figment, replaces ad-hoc error strings with thiserror types, removes deprecated aliases/interactive features, updates native execution helpers and dependency list, renames HNI_FAST → HNI_FAST_MODE, and updates tests and docs accordingly. ChangesCLI and Core System Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/parity_against_antfu.rs (1)
44-48: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep
nunin the parity matrix.This harness no longer creates or resolves
nun, and none of the fixture cases exercise uninstall parity. That leaves the PR’s required public uninstall alias unverified against the antfu reference, so a regression innunwiring or uninstall rendering can still pass parity.♻️ Suggested parity coverage update
create_alias(&our_bin, &our_alias_dir, "ni"); create_alias(&our_bin, &our_alias_dir, "nr"); create_alias(&our_bin, &our_alias_dir, "nlx"); + create_alias(&our_bin, &our_alias_dir, "nun"); create_alias(&our_bin, &our_alias_dir, "nci"); @@ Case { + antfu_bin: cmds.nun.clone(), + our_bin: "nun".into(), + args: vec!["lodash".into()], + }, + Case { antfu_bin: cmds.nci.clone(), our_bin: "nci".into(), args: vec![], }, @@ struct AntfuBins { ni: PathBuf, nr: PathBuf, nlx: PathBuf, + nun: PathBuf, nci: PathBuf, } @@ Some(AntfuBins { ni: which::which("ni").ok()?, nr: which::which("nr").ok()?, nlx: which::which("nlx").ok()?, + nun: which::which("nun").ok()?, nci: which::which("nci").ok()?, }) }Also applies to: 127-188, 289-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/parity_against_antfu.rs` around lines 44 - 48, The test harness removed the uninstall alias "nun" from the parity matrix so uninstall parity isn't verified; restore coverage by adding create_alias(&our_bin, &our_alias_dir, "nun") where other aliases are created (see create_alias calls for "ni","nr","nlx","nci") and ensure the same "nun" alias is present in the corresponding fixture blocks referenced (around the other alias creation ranges) so uninstall-related test cases exercise the "nun" path against the antfu reference.src/core/resolve/build.rs (1)
152-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the uninstall target before package-manager detection.
Line 153 resolves the package manager before we know whether
nunhas anything to remove. In an empty project,nun/nun -gcan currently fail with a detection error instead of the intended “requires a dependency” usage error, andnun -gcan surface the Yarn Berry global-operation error even when no package name was provided. Strip-g, check for remaining args, then detect.Suggested fix
pub fn resolve_nun(args: Vec<String>, ctx: &ResolveContext) -> Result<ResolvedExecution> { let use_global = args.iter().any(|arg| arg == "-g"); - let detected = detect_for_action(ctx, use_global)?; let args = if use_global { exclude_flag(args, "-g") } else { args }; if args.is_empty() { return Err(anyhow!( "execution error: nun requires a dependency to uninstall.\nTry: nun lodash" )); } + let detected = detect_for_action(ctx, use_global)?; ensure_detected_available(&detected, ctx)?; Ok(build_uninstall_exec( detected.pm, args, ctx.cwd(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/resolve/build.rs` around lines 152 - 166, The code calls detect_for_action(ctx, use_global) before validating that a package name was provided, causing package-manager detection errors to surface when args are empty; change the order so you strip the "-g" flag first (use exclude_flag on args when use_global is true), then check args.is_empty() and return the "requires a dependency" error if empty, and only after that call detect_for_action(ctx, use_global) and ensure_detected_available(&detected, ctx). Update references: move the existing detect_for_action/ensure_detected_available calls to after the args.is_empty() check and keep use_global, exclude_flag, detect_for_action, args.is_empty(), and ensure_detected_available logic intact.
🧹 Nitpick comments (2)
src/core/project.rs (1)
357-373: 💤 Low valueUpdate:
Comparator::parsesupports bare versions; consider build-metadata handling
semver::Comparator::parse("1.2.3")succeeds without an operator (treated as an exact match), so extractingmajor/minor/patchfromcomparatoris appropriate for barepackageManagerversion strings.- In
src/core/project.rs(parse_version_hint, lines 357-373),VersionHint.valueis rebuilt frommajor/minor/patch/preonly; if inputs can include build metadata like1.2.3+sha, the current normalization drops the+...portion—add a test/fixture if that metadata is expected to be preserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/project.rs` around lines 357 - 373, The normalization currently reconstructs VersionHint.value from Comparator::parse output (in parse_version_hint) but drops any build metadata (the "+..." suffix) present in the original trimmed input; update parse_version_hint to detect and preserve build metadata when present (e.g., parse or split the original trimmed string on '+' and append the "+..." portion to normalized after adding pre), so VersionHint.value retains the complete input (major/minor/patch/pre plus build metadata) while still using comparator.major/minor/patch and comparator.pre for the normalized core; refer to Comparator::parse, comparator.pre, and the VersionHint struct/value when implementing this.src/app/dispatch.rs (1)
31-94: 💤 Low valueThe
parsed.command.clone()is unnecessary.The
matchconsumes ownership ofparsed.command, butparsedis still borrowed later in some arms forresolve_context(&parsed, ...). However, sinceresolve_contextonly usesparsed.cwd,parsed.fast_override, the clone could be avoided by extracting these values first or restructuring slightly. That said,ParsedCommandis a lightweight enum and the clone cost is negligible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/dispatch.rs` around lines 31 - 94, The match currently does parsed.command.clone() in run_from_env which is unnecessary; change to match on &parsed.command (or destructure parsed first) so you don't clone the whole enum, update each arm to pattern-match by reference (e.g., ParsedCommand::PrintHelp(ref topic)) and only clone specific owned pieces when needed (for example clone invocation/args inside the ParsedCommand::Execute arm before calling dispatch_invocation or refactor dispatch_invocation to accept references), while keeping calls to resolve_context(&parsed, ...) and runner::run(&resolved) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 244-248: README.md omits the Windows default config location;
update the "Config file" section to document the Windows default path used by
the code by referencing how src/core/config.rs resolves it via
dirs::config_dir()/hni/config.toml (i.e., add a line for Windows alongside the
XDG and macOS entries). Make the wording consistent with the existing entries
and mention that the application uses dirs::config_dir() on Windows to determine
the default config location.
In `@src/core/resolve/detect.rs`:
- Around line 48-50: The check in detect.rs that returns
ResolveDetectionError::UnsupportedGlobalYarnBerry when use_global && pm ==
PackageManager::YarnBerry is too install-specific and is used by both resolve_ni
and resolve_nun; update the error to be action-neutral by changing the message
for ResolveDetectionError::UnsupportedGlobalYarnBerry to refer to "global
operation" (or "global install/uninstall") instead of "global install", or
alternatively add an explicit intent parameter to the detection helper and
include that intent in the error; adjust calls from resolve_ni and resolve_nun
accordingly so the error text correctly reflects install or uninstall intent
while still triggered by PackageManager::YarnBerry and use_global.
---
Outside diff comments:
In `@src/core/resolve/build.rs`:
- Around line 152-166: The code calls detect_for_action(ctx, use_global) before
validating that a package name was provided, causing package-manager detection
errors to surface when args are empty; change the order so you strip the "-g"
flag first (use exclude_flag on args when use_global is true), then check
args.is_empty() and return the "requires a dependency" error if empty, and only
after that call detect_for_action(ctx, use_global) and
ensure_detected_available(&detected, ctx). Update references: move the existing
detect_for_action/ensure_detected_available calls to after the args.is_empty()
check and keep use_global, exclude_flag, detect_for_action, args.is_empty(), and
ensure_detected_available logic intact.
In `@tests/parity_against_antfu.rs`:
- Around line 44-48: The test harness removed the uninstall alias "nun" from the
parity matrix so uninstall parity isn't verified; restore coverage by adding
create_alias(&our_bin, &our_alias_dir, "nun") where other aliases are created
(see create_alias calls for "ni","nr","nlx","nci") and ensure the same "nun"
alias is present in the corresponding fixture blocks referenced (around the
other alias creation ranges) so uninstall-related test cases exercise the "nun"
path against the antfu reference.
---
Nitpick comments:
In `@src/app/dispatch.rs`:
- Around line 31-94: The match currently does parsed.command.clone() in
run_from_env which is unnecessary; change to match on &parsed.command (or
destructure parsed first) so you don't clone the whole enum, update each arm to
pattern-match by reference (e.g., ParsedCommand::PrintHelp(ref topic)) and only
clone specific owned pieces when needed (for example clone invocation/args
inside the ParsedCommand::Execute arm before calling dispatch_invocation or
refactor dispatch_invocation to accept references), while keeping calls to
resolve_context(&parsed, ...) and runner::run(&resolved) unchanged.
In `@src/core/project.rs`:
- Around line 357-373: The normalization currently reconstructs
VersionHint.value from Comparator::parse output (in parse_version_hint) but
drops any build metadata (the "+..." suffix) present in the original trimmed
input; update parse_version_hint to detect and preserve build metadata when
present (e.g., parse or split the original trimmed string on '+' and append the
"+..." portion to normalized after adding pre), so VersionHint.value retains the
complete input (major/minor/patch/pre plus build metadata) while still using
comparator.major/minor/patch and comparator.pre for the normalized core; refer
to Comparator::parse, comparator.pre, and the VersionHint struct/value when
implementing this.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9efa2e0f-622e-476e-83f7-b22738cf8653
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
Cargo.tomlREADME.mdaliases.jsonbenchmark/run.mjsjsr.jsonjsr/na.tsjsr/nru.tsjsr/shared.tsjustfilesrc/app/cli.rssrc/app/command_registry.rssrc/app/commands.rssrc/app/completion.rssrc/app/dispatch.rssrc/app/doctor.rssrc/app/error_report.rssrc/app/help.rssrc/core/config.rssrc/core/detect.rssrc/core/native/bin_resolver.rssrc/core/native/deno.rssrc/core/native/exec.rssrc/core/native/plan.rssrc/core/pkg_json.rssrc/core/project.rssrc/core/resolve/build.rssrc/core/resolve/detect.rssrc/core/resolve/map.rssrc/core/resolve/mod.rssrc/core/types.rssrc/core/util.rssrc/features/interactive/completion.rssrc/features/interactive/mod.rssrc/features/interactive/ni_search.rssrc/features/interactive/nr_scripts.rssrc/features/interactive/nun_select.rssrc/features/mod.rssrc/features/node_shim.rssrc/features/nr.rssrc/platform/node.rstests/cli_contract.rstests/config_detect.rstests/dispatch_multicall.rstests/error_contract.rstests/fixture_modes.rstests/hni_meta.rstests/init_contract.rstests/native_deno_fast.rstests/native_execution.rstests/native_package_managers.rstests/native_regression.rstests/node_shim.rstests/nr_interactive_behavior.rstests/parity_against_antfu.rstests/resolve_matrix.rstests/support.rs
💤 Files with no reviewable changes (13)
- src/features/mod.rs
- src/features/interactive/nr_scripts.rs
- src/features/interactive/ni_search.rs
- src/features/interactive/mod.rs
- src/features/nr.rs
- src/features/interactive/completion.rs
- jsr.json
- src/core/resolve/map.rs
- jsr/nru.ts
- jsr/na.ts
- jsr/shared.ts
- src/features/interactive/nun_select.rs
- src/core/pkg_json.rs
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
1 participant
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.
Summary
Verification
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Chores