nh-installable: init#684
Conversation
📝 WalkthroughWalkthroughThis pull request extracts the ChangesInstallable extraction and API refactoring
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/nh-installable/src/tests.rs (1)
1-363: ⚡ Quick winConsider adding test coverage for NH_FILE resolution and error cases.
The test suite comprehensively covers flake-based environment variable resolution and precedence, but two areas lack coverage:
- NH_FILE resolution: The code at
lib.rs:318-326handlesNH_FILE(with optionalNH_ATTRP), but no tests exercise this path.- Error cases: Malformed environment variables (e.g.,
NH_FLAKEwithout a reference part) should return errors (lib.rs:303-314,332-343), but no tests verify this behavior.Example test additions
#[test] #[serial] fn test_resolve_uses_nh_file() { let env_guard = EnvGuard::clear(); env_guard.set("NH_FILE", "/path/to/config.nix"); env_guard.set("NH_ATTRP", "system.config"); let resolved = Installable::Unspecified .resolve(CommandContext::Os) .unwrap(); match resolved { Installable::File { path, attribute } => { assert_eq!(path, PathBuf::from("/path/to/config.nix")); assert_eq!(attribute, vec!["system", "config"]); }, _ => panic!("Expected File, got {resolved:?}"), } } #[test] #[serial] fn test_resolve_errors_on_malformed_nh_flake() { let env_guard = EnvGuard::clear(); env_guard.set("NH_FLAKE", "`#missing-reference`"); let result = Installable::Unspecified.resolve(CommandContext::Os); assert!(result.is_err()); }🤖 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 `@crates/nh-installable/src/tests.rs` around lines 1 - 363, Add tests exercising NH_FILE and malformed-env error paths: create serial tests similar to existing ones that use EnvGuard::clear() and env_guard.set to set NH_FILE and NH_ATTRP and then call Installable::Unspecified.resolve(CommandContext::Os) asserting it returns Installable::File with expected PathBuf and attribute vector (split on '.'); and add tests that set malformed values (e.g., NH_FLAKE="`#missing-reference`" and NH_FILE="`#bad`") and assert Installable::Unspecified.resolve(...) returns an Err; reference the existing helpers and behavior around Installable::Unspecified.resolve, EnvGuard, and attribute parsing to mirror the style of tests like test_resolve_with_nested_attribute and test_resolve_no_env_vars_returns_unspecified.
🤖 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.
Nitpick comments:
In `@crates/nh-installable/src/tests.rs`:
- Around line 1-363: Add tests exercising NH_FILE and malformed-env error paths:
create serial tests similar to existing ones that use EnvGuard::clear() and
env_guard.set to set NH_FILE and NH_ATTRP and then call
Installable::Unspecified.resolve(CommandContext::Os) asserting it returns
Installable::File with expected PathBuf and attribute vector (split on '.'); and
add tests that set malformed values (e.g., NH_FLAKE="`#missing-reference`" and
NH_FILE="`#bad`") and assert Installable::Unspecified.resolve(...) returns an Err;
reference the existing helpers and behavior around
Installable::Unspecified.resolve, EnvGuard, and attribute parsing to mirror the
style of tests like test_resolve_with_nested_attribute and
test_resolve_no_env_vars_returns_unspecified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3e86c7dd-120c-4174-883a-1f8b30d2eef6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/nh-core/Cargo.tomlcrates/nh-core/src/args.rscrates/nh-core/src/command.rscrates/nh-core/src/lib.rscrates/nh-core/src/update.rscrates/nh-core/src/util.rscrates/nh-darwin/Cargo.tomlcrates/nh-darwin/src/args.rscrates/nh-darwin/src/darwin.rscrates/nh-home/Cargo.tomlcrates/nh-home/src/args.rscrates/nh-home/src/home.rscrates/nh-installable/Cargo.tomlcrates/nh-installable/src/lib.rscrates/nh-installable/src/tests.rscrates/nh-nixos/Cargo.tomlcrates/nh-nixos/src/args.rscrates/nh-nixos/src/nixos.rscrates/nh-remote/Cargo.tomlcrates/nh-remote/src/remote.rs
💤 Files with no reviewable changes (1)
- crates/nh-core/src/lib.rs
|
needs to be rebased @faukah |
Move installable resolution out of
nh-coreand intonh-installable. More and more crates depend onnh-installable, and separating it fromnh-coreis easy. I'd argue this is a good choice because it's well-separated from other functionality/behavior.Also makes implementing
nh diffeasier/cleaner.Sanity Checking
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.