Skip to content

nixos: allow specifying fields to display in info subcmd#413

Merged
NotAShelf merged 3 commits intonix-community:masterfrom
cloud-yu:nh-generations
Sep 12, 2025
Merged

nixos: allow specifying fields to display in info subcmd#413
NotAShelf merged 3 commits intonix-community:masterfrom
cloud-yu:nh-generations

Conversation

@cloud-yu
Copy link
Contributor

@cloud-yu cloud-yu commented Sep 8, 2025

Hi,
for #375 , I make some changes for nh os info

  1. add a closure size column for every generations
image
  1. add a args --fmt to specify which field to display
image

Sanity Checking

  • I have updated the changelog as per my changes
  • I have tested, and self-reviewed my code
  • Style and consistency
    • I ran nix fmt to format my Nix code
    • I ran cargo fmt to format my Rust code
    • I have added appropriate docunentation to new code
    • My changes are consistent with the rest of the codebase
  • Correctness
    • I ran cargo clippy and fixed any new linter warnings.
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas to explain the
      logic
    • I have documented the motive for those changes in the PR body or commit
      description.
  • Tested on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin

Add a 👍 reaction to pull requests you find important.

Summary by CodeRabbit

  • New Features

    • Generation listings now include a per-generation "Closure Size" column.
    • Added a --fields option to select which columns and their order (comma-delimited; defaults to id,date,nver,kernel,confRev,spec,size).
  • Documentation

    • Changelog updated to document the new --fields option and closure size display; minor formatting cleanup.

@coderabbitai
Copy link

coderabbitai bot commented Sep 8, 2025

Walkthrough

Adds per-generation closure size retrieval and exposes it as GenerationInfo.closure_size. Refactors printing to accept a configurable set of columns via a new --fields CLI option (parsed into Vec<Field>), wires that option through nixos.rs to generations::print_info, and updates CHANGELOG.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Updates Unreleased → Changed to note nh os info supports --fields selection and adds a per-generation "Closure Size" column; removes an extra blank line.
Generations: closure size + dynamic columns
src/generations.rs
Adds pub closure_size: String to GenerationInfo; adds pub fn get_closure_size(generation_dir: &Path) -> Option<String> which runs nix path-info/JSON to obtain closureSize (falls back to "Unknown"); describe populates closure_size for each generation; introduces pub enum Field { Id, Date, Nver, Kernel, Confrev, Spec, Size } and changes print_info to accept fields: &[Field] and render columns dynamically (mapping Size to closure_size).
CLI args: --fields
src/interface.rs
Adds pub fields: Vec<Field> to OsGenerationsArgs with #[arg(long, value_delimiter = ',', default_value = "id,date,nver,kernel,confRev,spec,size")], importing crate::generations::Field.
Invocation wiring & error propagation
src/nixos.rs
OsGenerationsArgs::info now calls generations::print_info(descriptions, &self.fields)?, propagating errors from print_info; data collection/filtering unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant CLI as nh os info
  participant IF as interface::OsGenerationsArgs
  participant NX as nixos::OsGenerationsArgs::info
  participant G as generations
  participant OS as nix path-info
  participant OUT as stdout

  U->>CLI: run with optional --fields
  CLI->>IF: parse args (fields: Vec<Field>)
  IF->>NX: call info(self)
  NX->>G: describe() -> Vec<GenerationInfo>
  loop each generation
    G->>OS: get_closure_size(generation_path)
    OS-->>G: closureSize or error
    G-->>G: set GenerationInfo.closure_size ("Unknown" on error)
  end
  NX->>G: print_info(generations, &self.fields)
  G-->>OUT: render table with selected columns (e.g., id,date,...,size)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately describes the primary change: adding the ability to specify which fields are shown by the info subcommand. It is concise, on-topic, and avoids unnecessary detail or noise. Stylistically, "subcmd" could be expanded to "subcommand" for clarity, but the meaning is clear.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d942d93 and 62e1177.

📒 Files selected for processing (1)
  • src/interface.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/interface.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). (5)
  • GitHub Check: Test NH on Darwin
  • GitHub Check: treewide-checks
  • GitHub Check: Test NH on Linux
  • GitHub Check: Build NH on Darwin
  • GitHub Check: Build NH on Linux
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
src/interface.rs (1)

333-346: Validate --fmt values and tighten/help the docs

Unknown keys are silently ignored downstream; validate at parse-time and clarify the help text.

Apply:

-    /// Specify which field to show, fields delimited by ',',default show all
+    /// Specify which field(s) to show (comma-delimited). Defaults to all fields.
@@
-    /// field:
+    /// Fields:
@@
-    #[arg(long, value_delimiter = ',', verbatim_doc_comment)]
+    #[arg(
+        long,
+        value_delimiter = ',',
+        verbatim_doc_comment,
+        value_parser = clap::builder::PossibleValuesParser::new([
+            "id","date","nver","kernel","confRev","spec","size"
+        ])
+        .map(|s| s.to_string())
+    )]
     pub fmt: Option<Vec<String>>,
src/generations.rs (4)

52-76: Optional: remove jq and parse JSON directly

Fewer external deps and faster. nix path-info -S --json returns an array; parse the first element.

-    match (Exec::cmd("nix")
-        .arg("path-info")
-        .arg(generation_dir)
-        .arg("-S")
-        .arg("--json")
-        | Exec::cmd("jq").arg(".[]"))
-    .capture()
+    match Exec::cmd("nix")
+        .arg("path-info")
+        .arg(generation_dir)
+        .arg("-S")
+        .arg("--json")
+        .capture()
     {
-        Ok(output) => {
-            match serde_json::from_str::<serde_json::Value>(&String::from_utf8_lossy(
-                &output.stdout,
-            )) {
+        Ok(output) => {
+            match serde_json::from_slice::<serde_json::Value>(&output.stdout) {
                 #[allow(clippy::cast_precision_loss)]
-                Ok(json) => Some(json["closureSize"].as_u64().map_or_else(
+                Ok(json) => {
+                    let size = json
+                        .as_array()
+                        .and_then(|arr| arr.first())
+                        .and_then(|v| v.get("closureSize"))
+                        .and_then(|v| v.as_u64());
+                    Some(size.map_or_else(
                         || "Unknown".to_string(),
                         |bytes| format!("{:.1} GB", bytes as f64 / 1_073_741_824.0),
-                )),
+                    ))
+                }
                 Err(_) => Some("Unknown".to_string()),
             }
         }
         Err(_) => Some("Unknown".to_string()),
     }

52-76: Return type can be String, not Option

Function never returns None; simplify API and avoid ? at call sites.

-pub fn get_closure_size(generation_dir: &Path) -> Option<String> {
+pub fn get_closure_size(generation_dir: &Path) -> String {
@@
-        Ok(output) => {
-            match serde_json::from_slice::<serde_json::Value>(&output.stdout) {
+        Ok(output) => {
+            match serde_json::from_slice::<serde_json::Value>(&output.stdout) {
@@
-                Ok(json) => Some(/* ... */),
-                Err(_) => Some("Unknown".to_string()),
+                Ok(json) => /* ... */,
+                Err(_) => "Unknown".to_string(),
             }
         }
-        Err(_) => Some("Unknown".to_string()),
+        Err(_) => "Unknown".to_string(),
     }
 }

And in describe(...):

-    let closure_size = get_closure_size(generation_dir)?;
+    let closure_size = get_closure_size(generation_dir);

268-312: Unknown fmt keys are silently dropped

If the user passes only invalid keys, the table prints empty lines. Either validate at CLI (preferred, see interface.rs comment) or guard here.

Quick guard:

     let mut table = HashMap::new();
@@
     let header = formats
@@
     println!("{}", header);
+    if header.trim().is_empty() {
+        bail!("No valid fields to display. Allowed: id,date,nver,kernel,confRev,spec,size");
+    }

342-369: Match on keys, not display names

Matching on header labels couples rendering to logic. Match on f.as_str() to decouple from display text.

Example:

-                table.get(f).map(|(name, lens)| match name.as_str() {
-                    "Generation No" => { /* ... */ }
+                table.get(f).map(|(_, lens)| match f.as_str() {
+                    "id" => { /* ... */ }
                     /* ... other arms ... */
-                    "Closure Size" => format!("{:1$}", generation.closure_size, lens),
+                    "size" => format!("{:1$}", generation.closure_size, lens),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 450cc45 and d5b39e1.

📒 Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • src/generations.rs (9 hunks)
  • src/interface.rs (1 hunks)
  • src/nixos.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (1)
src/generations.rs (1)
  • print_info (229-372)
🔇 Additional comments (1)
src/generations.rs (1)

35-36: New field looks good

Adding closure_size to GenerationInfo is consistent with the new column and usage.

CHANGELOG.md Outdated
factor and results may differ.
- Added more context to some minor debug messages across platform commands.

- `nh os info` allow specifying which field to display
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix grammar and fully document the change (mention --fmt and closure size).

Current bullet is ungrammatical and omits the added “Closure Size” column.

-- `nh os info` allow specifying which field to display
+- `nh os info` now supports `--fmt` to select which field(s) to display; also adds a per-generation "Closure Size" column.

Optionally add the issue reference for traceability:

- 
+  ([#375])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- `nh os info` allow specifying which field to display
- `nh os info` now supports `--fmt` to select which field(s) to display; also adds a per-generation "Closure Size" column.
([#375])
🤖 Prompt for AI Agents
In CHANGELOG.md around line 49, the bullet " `nh os info` allow specifying which
field to display" is ungrammatical and incomplete; update it to a clear,
grammatical sentence that documents the full change: mention the new --fmt
option and that a "Closure Size" column was added, and optionally append the
related issue number for traceability. Ensure the phrasing follows existing
changelog style (e.g., use past tense like "Added" or "Now supports") and
include both the flag name (--fmt) and the new column name ("Closure Size") in
the single bullet.

.arg(generation_dir)
.arg("-Sh")
.arg("--json")
| Exec::cmd("jq").arg(".[]"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on a shell tool for JSON parsing is unacceptable. We already have serde_json, use it please. I'm also not sure why this uses Exec now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Exec just for the pipeline operation.

nix path-info -S --json returns a map not an array; parse the first element with jq[0] get Value::Null
I'll rollback to Command, and add get the realpath with read_link for the map index name.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use jq for this.

src/interface.rs Outdated
Comment on lines +344 to +345
#[arg(long, value_delimiter = ',', verbatim_doc_comment)]
pub fmt: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fields and not fmt imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer fmt because it also support ordered the table header.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think fields makes more sense here. It does not imply an ordering but the name of the flag itself makes more sense. With something like fmt I'd assume I can customize more than just the fields to be displayed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll fix it

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
src/generations.rs (1)

51-77: Fix: drop -h when using --json; parse robustly; label GiB

-h humanizes numbers and turns closureSize into strings, breaking as_u64(); also check exit status and read the first (only) map value. This aligns with prior feedback.

Apply:

@@
 pub fn get_closure_size(generation_dir: &Path) -> Option<String> {
     let store_path = generation_dir
         .read_link()
         .map_or_else(|_| generation_dir.to_path_buf(), |p| p);
     match process::Command::new("nix")
-        .arg("path-info")
-        .arg(generation_dir)
-        .arg("-Sh")
-        .arg("--json")
+        .arg("path-info")
+        .arg("-S") // keep JSON numeric
+        .arg("--json")
+        .arg(&store_path)
         .output()
     {
         Ok(output) => {
-            match serde_json::from_str::<serde_json::Value>(&String::from_utf8_lossy(
-                &output.stdout,
-            )) {
-                #[allow(clippy::cast_precision_loss)]
-                Ok(json) => Some(json[store_path.to_string_lossy().to_string()]["closureSize"].as_u64().map_or_else(
-                    || "Unknown".to_string(),
-                    |bytes| format!("{:.1} GB", bytes as f64 / 1_073_741_824.0),
-                )),
-                Err(_) => Some("Unknown".to_string()),
-            }
+            if !output.status.success() {
+                return Some("Unknown".to_string());
+            }
+            match serde_json::from_slice::<serde_json::Value>(&output.stdout) {
+                #[allow(clippy::cast_precision_loss)]
+                Ok(serde_json::Value::Object(map)) => {
+                    let size = map
+                        .values()
+                        .next()
+                        .and_then(|v| v.get("closureSize"))
+                        .and_then(|v| v.as_u64());
+                    Some(size.map_or_else(
+                        || "Unknown".to_string(),
+                        |bytes| format!("{:.1} GiB", bytes as f64 / 1_073_741_824.0),
+                    ))
+                }
+                _ => Some("Unknown".to_string()),
+            }
         }
         Err(_) => Some("Unknown".to_string()),
     }
 }
src/nixos.rs (1)

761-762: LGTM: propagate print_info errors

This resolves silent failures noted earlier; returning the Result is the right call.

🧹 Nitpick comments (6)
src/generations.rs (6)

34-35: Prefer storing raw size (bytes) and format at print time

Keeping raw bytes (e.g., Option) in GenerationInfo and formatting in print_info gives flexibility for units/sorting and avoids repeated parsing later.


79-83: Avoid N external calls: batch path-info

describe() invokes nix once per generation. Consider a single nix path-info -S --json and mapping results by path for O(1) lookup.

Happy to sketch a small helper that collects all store paths and returns a HashMap<PathBuf, u64>.


230-230: Docstring should mention the new fmt parameter and defaults

Add rustdoc noting accepted keys (id,date,nver,kernel,confRev,spec,size) and the default order.


269-277: Validate --fmt: reject empty/unknown keys early

Fail fast on typos; also guard against an empty list.

-    let formats = fmt.as_ref().map_or_else(
-        || {
-            ["id", "date", "nver", "kernel", "confRev", "spec", "size"]
-                .iter()
-                .map(|s| s.to_string())
-                .collect::<Vec<String>>()
-        },
-        |v| v.to_vec(),
-    );
+    let mut formats = fmt.as_ref().map_or_else(
+        || {
+            ["id", "date", "nver", "kernel", "confRev", "spec", "size"]
+                .into_iter()
+                .map(String::from)
+                .collect::<Vec<String>>()
+        },
+        |v| v.to_vec(),
+    );
+    let allowed = ["id", "date", "nver", "kernel", "confRev", "spec", "size"];
+    if formats.is_empty() {
+        bail!("--fmt cannot be empty. Allowed keys: {}", allowed.join(", "));
+    }
+    if let Some(bad) = formats.iter().find(|s| !allowed.contains(&s.as_str())) {
+        bail!("Unknown --fmt key: {bad}. Allowed keys: {}", allowed.join(", "));
+    }

279-313: Fix header width for “Configuration Revision”

The header is 24 chars; width is 22, causing misalignment. Bump to 24.

-                ("Configuration Revision".to_string(), 22),
+                ("Configuration Revision".to_string(), 24),

343-370: Match on fmt keys, not header labels

Avoid brittle string matches on localized header names; use the stable fmt keys.

-        let row: String = formats
-            .iter()
-            .filter_map(|f| {
-                table.get(f).map(|(name, lens)| match name.as_str() {
-                    "Generation No" => format!(
+        let row: String = formats
+            .iter()
+            .filter_map(|f| {
+                table.get(f).map(|(_, lens)| match f.as_str() {
+                    "id" => format!(
                         "{:1$}",
                         format!(
                             "{}{}",
                             generation.number,
                             if generation.current { " (current)" } else { "" }
                         ),
                         lens
                     ),
-                    "Build Date" => format!("{:1$}", formatted_date, lens),
-                    "NixOS Version" => format!("{:1$}", generation.nixos_version, lens),
-                    "Kernel" => format!("{:1$}", generation.kernel_version, lens),
-                    "Configuration Revision" => {
+                    "date" => format!("{:1$}", formatted_date, lens),
+                    "nver" => format!("{:1$}", generation.nixos_version, lens),
+                    "kernel" => format!("{:1$}", generation.kernel_version, lens),
+                    "confRev" => {
                         format!("{:1$}", generation.configuration_revision, lens)
                     }
-                    "Specialisations" => format!("{:1$}", specialisations, lens),
-                    "Closure Size" => format!("{:1$}", generation.closure_size, lens),
+                    "spec" => format!("{:1$}", specialisations, lens),
+                    "size" => format!("{:1$}", generation.closure_size, lens),
                     _ => "".to_string(),
                 })
             })
             .collect::<Vec<String>>()
             .join(" ");
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5b39e1 and ddb2879.

📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • src/generations.rs (8 hunks)
  • src/nixos.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (1)
src/generations.rs (1)
  • print_info (230-373)
🔇 Additional comments (1)
src/generations.rs (1)

183-193: LGTM: closure_size is consistently propagated

All early returns and the final struct include closure_size. Good attention to completeness.

Also applies to: 199-209, 213-223

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/generations.rs (1)

106-111: Drop “-h” with --json; else closureSize becomes a string → “Unknown” everywhere

nix path-info --json -h humanizes sizes (strings), so as_u64() fails. Also, bail out on non-zero exit codes to avoid parsing garbage. This echoes previous feedback.

-    match process::Command::new("nix")
+    match process::Command::new("nix")
         .arg("path-info")
-        .arg(generation_dir)
-        .arg("-Sh")
+        .arg(generation_dir)
+        .arg("-S")    // keep closureSize in JSON, but do NOT humanize
         .arg("--json")
         .output()
     {
-        Ok(output) => {
+        Ok(output) => {
+            if !output.status.success() {
+                return Some("Unknown".to_string());
+            }
             match serde_json::from_str::<serde_json::Value>(&String::from_utf8_lossy(
                 &output.stdout,
             )) {

Also applies to: 113-131

🧹 Nitpick comments (5)
src/generations.rs (4)

101-131: Optional: avoid per-generation “nix” calls; query all at once

Spawning nix path-info N times scales poorly. Consider collecting all store paths and calling one nix path-info -S --json <paths...>, then look up closureSize per path. I can sketch this refactor if desired.


35-37: Prefer a typed size over a display string

Storing raw bytes (e.g., Option<u64>) keeps data usable (sorting, alternate units). Format only at print time. Backwards-compatible display remains unchanged.

-    /// Closure size of the generation.
-    pub closure_size: String,
+    /// Closure size in bytes (None when unknown).
+    pub closure_size: Option<u64>,

And in printing (within the Field::Size arm):

-    Field::Size => generation.closure_size.clone(),
+    Field::Size => generation
+        .closure_size
+        .map(|b| format!("{:.1} GB", b as f64 / 1_073_741_824.0))
+        .unwrap_or_else(|| "Unknown".to_string()),

288-295: Show “Unknown” for unknown dates instead of epoch 1970

Falling back to Local.timestamp_opt(0, 0) renders “1970-01-01 …”, which looks like real data. If generation.date is “Unknown”, keep it “Unknown”.

-    let date = DateTime::parse_from_rfc3339(&generation.date).map_or_else(
-        |_| Local.timestamp_opt(0, 0).unwrap(),
-        |dt| dt.with_timezone(&Local),
-    );
-    parsed_dates.insert(
-        generation.date.clone(),
-        date.format("%Y-%m-%d %H:%M:%S").to_string(),
-    );
+    let formatted = if generation.date == "Unknown" {
+        "Unknown".to_string()
+    } else {
+        DateTime::parse_from_rfc3339(&generation.date)
+            .map(|dt| dt.with_timezone(&Local).format("%Y-%m-%d %H:%M:%S").to_string())
+            .unwrap_or_else(|_| "Unknown".to_string())
+    };
+    parsed_dates.insert(generation.date.clone(), formatted);

323-331: Guard widths so headers don’t get truncated

Make each column at least as wide as its header label.

-    let widths = ColumnWidths {
-        id: 13,   // "Generation No"
-        date: 20, // "Build Date"
-        nver: max_nixos_version_len,
-        kernel: max_kernel_len,
-        confrev: 22, // "Configuration Revision"
-        spec: 15,    // "Specialisations"
-        size: 12,    // "Closure Size"
-    };
+    let widths = ColumnWidths {
+        id: 13,   // "Generation No"
+        date: 20, // "Build Date"
+        nver: max_nixos_version_len.max("NixOS Version".len()),
+        kernel: max_kernel_len.max("Kernel".len()),
+        confrev: 22, // "Configuration Revision"
+        spec: 15,    // "Specialisations"
+        size: 12,    // "Closure Size"
+    };

Also applies to: 75-87

src/interface.rs (1)

335-342: Polish help text (clarity + spacing) for --fields

Minor wording and spacing fix.

-    /// Specify which field(s) to show (comma-delimited).Defaults to show all fields.
+    /// Specify which field(s) to show (comma-delimited). Defaults to all fields.
+    ///
+    /// Example: --fields id,date,size
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ddb2879 and ebee262.

📒 Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • src/generations.rs (9 hunks)
  • src/interface.rs (2 hunks)
  • src/nixos.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (1)
src/generations.rs (1)
  • print_info (284-386)
🔇 Additional comments (1)
src/nixos.rs (1)

761-762: Good: propagate print_info errors

Switching to ? ensures failures (e.g., no current generation) surface properly.

@NotAShelf NotAShelf changed the title nh os info allow specifying which field to display nixos: allow specifying fields to display in info subcmd Sep 10, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
src/generations.rs (3)

98-130: Drop “-h” with --json; add robust JSON parsing and status check.
With --json, -h humanizes sizes into strings, making as_u64() fail and yielding “Unknown”. Also check status.success() and handle both map/array JSON shapes; pass the resolved store path to nix for consistent keys. This was flagged earlier but still present here.

Apply:

@@
 pub fn get_closure_size(generation_dir: &Path) -> Option<String> {
-  let store_path = generation_dir
-    .read_link()
-    .unwrap_or_else(|_| generation_dir.to_path_buf());
-  match process::Command::new("nix")
-    .arg("path-info")
-    .arg(generation_dir)
-    .arg("-Sh")
-    .arg("--json")
-    .output()
-  {
+  let store_path = generation_dir
+    .read_link()
+    .unwrap_or_else(|_| generation_dir.to_path_buf());
+  let store_path_str = store_path.to_string_lossy().to_string();
+  match process::Command::new("nix")
+    .arg("path-info")
+    .arg(&store_path)
+    .arg("-S")            // keep closure sizes, but no -h with --json
+    .arg("--json")
+    .output()
+  {
     Ok(output) => {
-      match serde_json::from_str::<serde_json::Value>(&String::from_utf8_lossy(
-        &output.stdout,
-      )) {
+      if !output.status.success() {
+        return Some("Unknown".to_string());
+      }
+      match serde_json::from_slice::<serde_json::Value>(&output.stdout) {
         #[allow(clippy::cast_precision_loss)]
         Ok(json) => {
-          Some(
-            json[store_path.to_string_lossy().to_string()]["closureSize"]
-              .as_u64()
-              .map_or_else(
-                || "Unknown".to_string(),
-                |bytes| format!("{:.1} GB", bytes as f64 / 1_073_741_824.0),
-              ),
-          )
+          // Support both map and array outputs
+          let bytes = match &json {
+            serde_json::Value::Object(map) => map
+              .get(&store_path_str)
+              .and_then(|v| v.get("closureSize"))
+              .and_then(|n| n.as_u64()),
+            serde_json::Value::Array(arr) => arr
+              .get(0)
+              .and_then(|v| v.get("closureSize"))
+              .and_then(|n| n.as_u64()),
+            _ => None,
+          };
+          Some(bytes.map_or_else(
+            || "Unknown".to_string(),
+            |b| format!("{:.1} GB", b as f64 / 1_073_741_824.0),
+          ))
         },
         Err(_) => Some("Unknown".to_string()),
       }
     },
     Err(_) => Some("Unknown".to_string()),
   }
 }

Also consider batching: call nix path-info -S --json <all store paths> once and look up per path to avoid N processes. Happy to provide a follow-up patch.


338-346: Fix dynamic width formatting — won’t compile as-is.
Pass the width named arg.

-      format!("{:<width$}", name)
+      format!("{:<width$}", name, width = width)

366-389: Same here: supply width to format!.

-        format!("{:width$}", cell_content)
+        format!("{:width$}", cell_content, width = width)
🧹 Nitpick comments (5)
src/generations.rs (5)

36-59: Verify clap attribute for ValueEnum variant rename (clap v4 uses #[value(...)])
If the project is on clap v4, prefer #[value(name = "confRev")] (and optionally aliases) over #[clap(name = ...)]. Otherwise compilation may warn/fail.

Would you like me to adjust the attributes and add aliases (e.g., "conf-rev")?


133-135: Simplify: get_closure_size never returns None; drop “?” or change signature.
Either make get_closure_size return String or keep Option<String> but call without ?.

Proposed minimal tweak:

-  let closure_size = get_closure_size(generation_dir)?;
+  let closure_size = get_closure_size(generation_dir).unwrap_or_else(|| "Unknown".into());

150-152: Trim nixos-version for clean output.
The file usually ends with a newline.

-  let nixos_version = fs::read_to_string(generation_dir.join("nixos-version"))
-    .unwrap_or_else(|_| "Unknown".to_string());
+  let nixos_version = fs::read_to_string(generation_dir.join("nixos-version"))
+    .map(|s| s.trim().to_string())
+    .unwrap_or_else(|_| "Unknown".to_string());

167-193: Deterministic output: sort kernel versions before join.
Directory iteration order is not guaranteed.

-        versions.join(", ")
+        versions.sort();
+        versions.join(", ")

Apply similarly in the fallback branch.


213-230: Deterministic output: sort specialisations.
Improves stability and testability.

-          specs
+          specs.sort();
+          specs
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebee262 and d942d93.

📒 Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • src/generations.rs (9 hunks)
  • src/interface.rs (2 hunks)
  • src/nixos.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/interface.rs
  • src/nixos.rs
  • CHANGELOG.md
⏰ 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 Linux
  • GitHub Check: Test NH on Darwin
  • GitHub Check: Build NH on Darwin
  • GitHub Check: treewide-checks
  • GitHub Check: Build NH on Linux
🔇 Additional comments (3)
src/generations.rs (3)

32-34: LGTM: new field is well-placed and used throughout.


61-70: Fixed widths struct looks fine.
Chosen widths are reasonable defaults.


72-84: Column metadata mapping reads cleanly.
No issues.

@cloud-yu cloud-yu requested a review from NotAShelf September 10, 2025 23:57
Copy link
Member

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last one I promise. LGTM otherwise.

Co-authored-by: raf <raf@notashelf.dev>
@NotAShelf NotAShelf merged commit a818218 into nix-community:master Sep 12, 2025
8 checks passed
@NotAShelf
Copy link
Member

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants