meta: resolve fixable clippy warning by neunenak · Pull Request #484 · nix-community/nh

neunenak · 2025-11-27T09:55:02Z

Exactly what it says, I ran cargo clippy --fix, briefly checked the generated code to make sure it looked reasonable, and that's what I'm committing. This reduces the number of clippy warnings I see by about 10.

Sanity Checking

Add a 👍 reaction to pull requests you find important.

Summary by CodeRabbit

Refactor
- Builder methods made eligible for compile-time use to enable minor optimizations.
- Streamlined error handling in logging paths.
- Ownership/cloning reduced in a few places to avoid unnecessary copies.
Chores
- Enhanced type-safety by extending derived equality for an enum.
- A public utility now signals its return should be used to avoid accidental ignores.
- Minor control-flow tweaks for automatic configuration detection.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-27T09:55:11Z

Walkthrough

Small, focused edits across multiple modules: promote several methods to const fn, add #[must_use] and Eq derive, minor ownership/clone adjustments, small pattern-match and logging-statement tweaks, and a change to installable resolution using unwrap_or_else. No substantive behavioral changes.

Changes

Cohort / File(s)	Summary
Const promotions & builder API `src/commands.rs`	Converted `Command::dry` and `Command::show_output` to `pub const fn` (signatures changed; semantics unchanged). Also adjusted `ElevationStrategy` pattern matches to use `Self::` inside `resolve`.
Const utility & attributes `src/generations.rs`	Made `Field::column_info` a `const fn`; added `#[must_use]` to `pub fn describe(...)`.
Ownership / control-flow tweaks `src/home.rs`	Moved `out_path` into `target_profile` when no specialization (avoids clone); changed automatic detection loop to use equality check against `Some("true")` and added `break` on detection.
Clone instead of to_string() `src/installable.rs`	Replaced `to_string()` with `clone()` for `Expression` variants in `FromArgMatches` and `to_args`.
Logging statement adjustment `src/logging.rs`	In `Level::ERROR`, `Level::DEBUG`, and `Level::TRACE` arms, added semicolons after `write!` calls (converted final expressions to statements; propagation behavior preserved).
Installable resolution simplification `src/nixos.rs`	`resolve_installable_and_toplevel` now prefers `NH_OS_FLAKE` via `unwrap_or_else(... )` when present; behavior preserved when absent.
Trait derives `src/util.rs`	Added `Eq` to `#[derive(...)]` for `pub enum NixVariant` (now derives `Eq` in addition to existing traits).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pay attention to ownership change in src/home.rs (out_path moved — ensure no subsequent use).
Confirm const fn promotions don't break prior non-const call sites or expected initialization constraints.
Verify clone() replacements in src/installable.rs preserve lifetimes and ownership expectations.
Check logging adjustments in src/logging.rs to ensure the intended error handling behavior remains.

Possibly related PRs

nixos: cleanup #436: Similar change to resolve_installable_and_toplevel to derive the installable from NH_OS_FLAKE when present.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objective: resolving fixable clippy warnings through automated fixes.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/nixos.rs (1)
332-332: Address the formatting issue flagged by CI.

The clippy fix is correct—using unwrap_or_else on Option is more idiomatic than map_or_else. However, the CI pipeline has flagged a formatting difference on this line. Run cargo fmt to fix the newline handling around unwrap_or_else.

Also, the parentheses around (get_nh_os_flake_env()?) are unnecessary:
-    let installable = (get_nh_os_flake_env()?).unwrap_or_else(|| self.common.installable.clone());
+    let installable =
+      get_nh_os_flake_env()?.unwrap_or_else(|| self.common.installable.clone());
src/home.rs (1)

319-320: Address the formatting issue flagged by CI.

The logic is correct, but the CI pipeline flagged a formatting difference at this location. Run cargo fmt to fix the conditional block formatting (space/newline handling around the if condition).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 47374db and 45088d6.

📒 Files selected for processing (7)

src/commands.rs (3 hunks)
src/generations.rs (2 hunks)
src/home.rs (2 hunks)
src/installable.rs (2 hunks)
src/logging.rs (1 hunks)
src/nixos.rs (1 hunks)
src/util.rs (1 hunks)

🧰 Additional context used

🪛 GitHub Actions: Check formating & lints

src/nixos.rs

[error] 329-329: Diff in formatting detected by cargo fmt. Method call formatting differs (unwrap_or_else newline handling).

src/generations.rs

[error] 173-173: Diff in formatting detected by cargo fmt. #[must_use] attribute formatting differs from the expected style.

src/home.rs

[error] 316-316: Diff in formatting detected by cargo fmt. Conditional block formatting differs (space/newline handling).

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Test NH on Darwin
GitHub Check: Test NH on Linux
GitHub Check: Build NH on Darwin
GitHub Check: Build NH on Linux
GitHub Check: profile

🔇 Additional comments (8)

src/installable.rs (2)

60-60: LGTM!

Using clone() instead of to_string() for String types is more idiomatic and slightly more efficient, as it avoids the Display trait indirection.

289-289: LGTM!

Consistent with the change at line 60—using clone() on String is the correct approach.

src/logging.rs (1)

32-44: LGTM!

The match arm formatting is consistent, and error propagation via ? is correctly maintained across all log levels.

src/util.rs (1)

17-22: LGTM!

Adding Eq alongside PartialEq is correct for this simple enum. This satisfies clippy::derive_partial_eq_without_eq and enables using NixVariant in hash-based collections or other Eq-requiring contexts.

src/home.rs (1)

135-139: LGTM!

Moving out_path instead of cloning is correct here—the variable is not used after this assignment in the else branch, so the clone was redundant.

src/generations.rs (1)

72-83: Const-ifying Field::column_info is safe and idiomatic

The function body is fully const-friendly (pure match over self and ColumnWidths with string literals), so making it const fn is sound and widens where it can be used without changing runtime behavior.

src/commands.rs (2)

132-147: Using Self in ElevationStrategy::resolve keeps patterns concise

Switching match arms from ElevationStrategy::... to Self::... is purely stylistic, idiomatic, and does not change behavior.

225-237: const fn for Command::dry / show_output looks correct

Both methods only toggle boolean fields on a by-value self and return it, which is const-friendly and keeps all existing call sites working while enabling potential const-context usage.

github-actions · 2025-11-27T10:03:17Z

Performance Comparison `→`

Total Elapsed Time: 332.23ms → 30.06s (+8947.5%) ⚠️

Profiling Mode: timing - Execution duration of functions.

+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| Function                           | Calls               | Avg                               | P95                               | Total                             | % Total                       |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| main                               | 1 → 1 (+0.0%)       | 331.56ms → 30.06s (+8965.6%) ⚠️   | 331.61ms → 30.06s (+8966.2%) ⚠️   | 331.56ms → 30.06s (+8965.6%) ⚠️   | 100.00% → 100.00% (+0.0%)     |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| nh::search::run                    | 1 → 1 (+0.0%)       | 278.50ms → 30.01s (+10673.8%) ⚠️  | 278.66ms → 30.01s (+10671.0%) ⚠️  | 278.50ms → 30.01s (+10673.8%) ⚠️  | 83.99% → 99.82% (+18.8%)      |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| http_request                       | 1 → 1 (+0.0%)       | 86.64ms → 30.00s (+34524.8%) ⚠️   | 86.70ms → 30.01s (+34517.1%) ⚠️   | 86.64ms → 30.00s (+34524.8%) ⚠️   | 26.13% → 99.80% (+281.9%) ⚠️  |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| nh::checks::check_nix_version      | 1 → 1 (+0.0%)       | 52.45ms → 51.93ms (-1.0%)         | 52.46ms → 51.94ms (-1.0%)         | 52.45ms → 51.93ms (-1.0%)         | 15.82% → 0.17% (-98.9%) 🚀    |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| nh::checks::verify_nix_environment | 1 → 1 (+0.0%)       | 52.46ms → 51.94ms (-1.0%)         | 52.46ms → 51.97ms (-0.9%)         | 52.46ms → 51.94ms (-1.0%)         | 15.82% → 0.17% (-98.9%) 🚀    |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| nh::util::get_nix_version          | 1 → 1 (+0.0%)       | 25.91ms → 24.05ms (-7.2%)         | 25.92ms → 24.05ms (-7.2%)         | 25.91ms → 24.05ms (-7.2%)         | 7.81% → 0.08% (-99.0%) 🚀     |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+
| ️🗑️ json_parse                      | 1 → 0 (-100.0%) 🚀  | 12.13ms → 0.00ns (-100.0%) 🚀     | 12.13ms → 0.00ns (-100.0%) 🚀     | 12.13ms → 0.00ns (-100.0%) 🚀     | 3.65% → 0.00% (-100.0%) 🚀    |
+------------------------------------+---------------------+-----------------------------------+-----------------------------------+-----------------------------------+-------------------------------+

Generated with hotpath-rs

📊 View Raw JSON Metrics

PR Metrics

{
  "hotpath_profiling_mode": "timing",
  "total_elapsed": 30058334185,
  "description": "Execution duration of functions.",
  "caller_name": "main",
  "output": {
    "http_request": {
      "calls": 1,
      "avg": 30000127588,
      "p95": 30014439423,
      "total": 30000127588,
      "percent_total": 9980
    },
    "nh::checks::check_nix_version": {
      "calls": 1,
      "avg": 51931335,
      "p95": 51937279,
      "total": 51931335,
      "percent_total": 17
    },
    "nh::checks::verify_nix_environment": {
      "calls": 1,
      "avg": 51937737,
      "p95": 51970047,
      "total": 51937737,
      "percent_total": 17
    },
    "nh::search::run": {
      "calls": 1,
      "avg": 30005111256,
      "p95": 30014439423,
      "total": 30005111256,
      "percent_total": 9982
    },
    "main": {
      "calls": 1,
      "avg": 30057647179,
      "p95": 30064771071,
      "total": 30057647179,
      "percent_total": 10000
    },
    "nh::util::get_nix_version": {
      "calls": 1,
      "avg": 24051176,
      "p95": 24051711,
      "total": 24051176,
      "percent_total": 8
    }
  }
}

Main Branch Metrics

{
  "hotpath_profiling_mode": "timing",
  "total_elapsed": 332229313,
  "description": "Execution duration of functions.",
  "caller_name": "main",
  "output": {
    "main": {
      "calls": 1,
      "avg": 331558993,
      "p95": 331612159,
      "total": 331558993,
      "percent_total": 10000
    },
    "json_parse": {
      "calls": 1,
      "avg": 12126864,
      "p95": 12132351,
      "total": 12126864,
      "percent_total": 365
    },
    "http_request": {
      "calls": 1,
      "avg": 86643417,
      "p95": 86704127,
      "total": 86643417,
      "percent_total": 2613
    },
    "nh::checks::check_nix_version": {
      "calls": 1,
      "avg": 52454532,
      "p95": 52461567,
      "total": 52454532,
      "percent_total": 1582
    },
    "nh::checks::verify_nix_environment": {
      "calls": 1,
      "avg": 52460001,
      "p95": 52461567,
      "total": 52460001,
      "percent_total": 1582
    },
    "nh::search::run": {
      "calls": 1,
      "avg": 278500865,
      "p95": 278659071,
      "total": 278500865,
      "percent_total": 8399
    },
    "nh::util::get_nix_version": {
      "calls": 1,
      "avg": 25909303,
      "p95": 25919487,
      "total": 25909303,
      "percent_total": 781
    }
  }
}

Exactly what it says, I ran `cargo clippy --fix`, briefly checked the generated code to make sure it looked reasonable, and that's what I'm committing.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/home.rs (1)

291-327: Automatic configuration detection logic is sound; consider minor DRY/refactor

The new equality check against Some("true") plus the break on success makes the auto-detection both clearer and safer (no risk of extending attribute multiple times if more than one candidate matches). This also aligns with the explicit configuration check above.

You now have nearly identical “run nix eval and check for == Some("true")” blocks in both the explicit and automatic branches; consider factoring this into a small helper (e.g., fn has_attr(...) -> Result<bool>) to reduce duplication and keep future clippy fixes localized.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45088d6 and 983a2c5.

📒 Files selected for processing (7)

src/commands.rs (3 hunks)
src/generations.rs (2 hunks)
src/home.rs (2 hunks)
src/installable.rs (2 hunks)
src/logging.rs (1 hunks)
src/nixos.rs (1 hunks)
src/util.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (6)

src/installable.rs
src/generations.rs
src/commands.rs
src/util.rs
src/nixos.rs
src/logging.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

GitHub Check: Test NH on Linux
GitHub Check: Test NH on Darwin
GitHub Check: profile
GitHub Check: Build NH on Linux
GitHub Check: Build NH on Darwin
GitHub Check: treewide-checks

🔇 Additional comments (1)

src/home.rs (1)

135-139: Ownership change on out_path looks correct

Using out_path directly in the None branch (instead of cloning) is fine here: all prior uses only borrow it, and it’s not used after target_profile is constructed, so this removes an unnecessary clone without changing behavior.

NotAShelf

LGTM, thank you :)

coderabbitai Bot reviewed Nov 27, 2025

View reviewed changes

Comment thread src/generations.rs Outdated

run cargo clippy --fix

983a2c5

Exactly what it says, I ran `cargo clippy --fix`, briefly checked the generated code to make sure it looked reasonable, and that's what I'm committing.

neunenak force-pushed the clippy-fix branch from 45088d6 to 983a2c5 Compare November 27, 2025 10:31

coderabbitai Bot reviewed Nov 27, 2025

View reviewed changes

NotAShelf changed the title ~~run cargo clippy --fix~~ meta: resolve fixable clippy warning Nov 27, 2025

NotAShelf approved these changes Nov 27, 2025

View reviewed changes

NotAShelf merged commit 76db546 into nix-community:master Nov 27, 2025
9 checks passed

coderabbitai Bot mentioned this pull request Nov 28, 2025

installable: handle invalid installable resolution more gracefully #486

Merged

coderabbitai Bot mentioned this pull request Jan 22, 2026

nixos: add nh os build-image subcommand #528

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

meta: resolve fixable clippy warning#484

meta: resolve fixable clippy warning#484
NotAShelf merged 1 commit into
nix-community:masterfrom
neunenak:clippy-fix

neunenak commented Nov 27, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Nov 27, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Nov 27, 2025 •

edited

Loading

PR Metrics

Main Branch Metrics

Uh oh!

coderabbitai Bot left a comment

Uh oh!

NotAShelf left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

neunenak commented Nov 27, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sanity Checking

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance Comparison →

PR Metrics

Main Branch Metrics

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

NotAShelf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

neunenak commented Nov 27, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 27, 2025 •

edited

Loading

github-actions Bot commented Nov 27, 2025 •

edited

Loading

Performance Comparison `→`