generations: cleanup; hide empty columns #435
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughImplements optional fields in generation metadata and adjusts output rendering to hide empty columns. Changes include making configuration_revision and specialisations optional, altering get_closure_size to return a String with “Unknown” fallback, and updating print_info to compute visible fields based on available data. Changelog updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as nh os info
participant G as generations::describe/print_info
participant FS as Filesystem
U->>CLI: Run "nh os info"
CLI->>G: describe()
G->>FS: Read generation dirs, metadata, kernel modules
FS-->>G: Data or errors
G->>G: Build GenerationInfo (Option fields)
G->>G: get_closure_size() -> String ("Unknown" fallback)
CLI->>G: print_info(--fields?)
G->>G: Compute visible_fields (hide empty columns)
G-->>U: Render table with dynamic columns
note over G: Confrev/Spec omitted if empty across all gens
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/generations.rs (1)
98-166: Closure size detection can miss due to path mismatch; add robust fallbackMatching JSON entries by path can fail depending on whether nix returns the symlink path or its target. When this happens, size becomes “Unknown” despite being present.
- Match either store_path or the input path.
- Fallback to the first array entry’s closureSize if no match.
- Optional: label “GiB” since the divisor is 2^30.
pub fn get_closure_size(generation_dir: &Path) -> std::string::String { let store_path = generation_dir .read_link() .unwrap_or_else(|_| generation_dir.to_path_buf()); + let input_path_str = generation_dir.to_string_lossy().to_string(); match process::Command::new("nix") .arg("path-info") - .arg(generation_dir) + .arg(generation_dir) .arg("-Sh") .arg("--json") .output() { Ok(output) => { let output_str = String::from_utf8_lossy(&output.stdout); match serde_json::from_str::<serde_json::Value>(&output_str) { #[allow(clippy::cast_precision_loss)] Ok(json) => { // `nix path-info --json` returns an array, we need to handle it let store_path_str = store_path.to_string_lossy(); - let closure_size = json.as_array().and_then(|arr| { + let closure_size = json.as_array().and_then(|arr| { arr.iter().find_map(|entry| { let path = entry.get("path").and_then(|v| v.as_str()); let size = entry.get("closureSize").and_then(serde_json::Value::as_u64); if let (Some(path), Some(size)) = (path, size) { - if path == store_path_str { + if path == store_path_str || path == input_path_str { return Some(size); } } None }) }); + // Fallback to the first entry if specific path wasn't found + let closure_size = closure_size.or_else(|| { + json.as_array().and_then(|arr| { + arr.get(0) + .and_then(|e| e.get("closureSize").and_then(serde_json::Value::as_u64)) + }) + }); if closure_size.is_none() { let paths: Vec<String> = json .as_array() .map(|arr| { arr .iter() .filter_map(|entry| { entry.get("path").and_then(|v| { v.as_str().map(std::string::ToString::to_string) }) }) .collect() }) .unwrap_or_default(); debug!( "get_closure_size: store_path not found or closureSize missing. \ store_path: {store_path_str}, json paths: {:?}, output: {}", paths, output_str ); } closure_size.map_or_else( - || "Unknown".to_string(), - |bytes| format!("{:.1} GB", bytes as f64 / 1_073_741_824.0), + || "Unknown".to_string(), + |bytes| format!("{:.1} GiB", bytes as f64 / 1_073_741_824.0), ) }, Err(e) => { debug!( "get_closure_size: failed to parse JSON: {e} output: {output_str}" ); "Unknown".to_string() }, } }, Err(e) => { debug!("get_closure_size: failed to run nix path-info: {e:?}"); "Unknown".to_string" }, } }
🧹 Nitpick comments (2)
CHANGELOG.md (1)
21-22: Mention the formatting fix for large generation counts tooChangelog only notes hidden empty columns. Include the fix for formatting when generation numbers exceed 999 to reflect the PR’s scope.
-- `nh os info` now hides empty columns. +- `nh os info` now hides empty columns and fixes formatting for generation numbers ≥ 1000.src/generations.rs (1)
381-386: Ensure ID column width ≥ header to keep header/rows alignedWhen generation counts are small, id width can be less than "Generation No" length, causing header to push subsequent columns. Make width the max of header name and content width.
- let widths = ColumnWidths { - id: max_generation_no + 10, // "Generation No" + let id_header_len = "Generation No".len(); + let widths = ColumnWidths { + id: (max_generation_no + 10).max(id_header_len), date: 20, // "Build Date" nver: max_nixos_version_len, kernel: max_kernel_len, confrev: 22, // "Configuration Revision" spec: 15, // "Specialisations" size: 12, // "Closure Size" };Also applies to: 388-395
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)src/generations.rs(13 hunks)
⏰ 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: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
🔇 Additional comments (3)
src/generations.rs (3)
24-28: Optional fields + conditional columns: LGTM
- Using Option for configuration_revision and specialisations, plus filtering visible_fields, cleanly hides empty columns.
- Row rendering with unwrap_or_default yields blanks where appropriate.
Looks correct and matches the PR goal.
If a user explicitly specifies --fields including Confrev/Spec, is hiding still desired when all values are empty? Confirm product intent.
Also applies to: 351-367, 422-445
206-218: Kernel module version detection fallback: LGTMmap_or_else with “Unknown” on read_dir failures improves resilience. No issues.
Also applies to: 219-231
243-247: Small correctness/style tweaks: LGTM
- Trim/filter on configuration_revision avoids blank noise.
- Specialisations as Option with empty-to-None normalization is good.
- clippy::too_many_lines expectation is fine here.
Also applies to: 253-264, 319-319
|
@coderabbitai shut up |
|
@faukah, I understand. I can pause automatic reviews for this PR if you'd like. Just let me know if you need anything in the future. ✅ Actions performedReviews paused. |
faukah
force-pushed
the
faukah/generations
branch
from
174e877 to
42cbade
Compare
Previously, `nh os info` always printed all columns, which is frankly unnecessary if you're missing things such as specializations. - Remove empty columns from the print output. - Furthermore, fix formatting for systems with more than 1000 genererations.
faukah
force-pushed
the
faukah/generations
branch
from
42cbade to
7a9055e
Compare
2 participants
Previously,
nh os infoalways printed all columns, which is frankly unnecessary if you're missingthings such as specializations.
Previous:
New:
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.
Summary by CodeRabbit
Improvements
Documentation