Conversation
BradSwain
left a comment
There was a problem hiding this comment.
Code Review — sysinfo 0.28.2 → 0.38.2
Overall this is a clean API migration. The Option-based handling of process.exe() and process.name() returning &OsStr are correctly addressed throughout. A few items worth considering:
Medium — Duplicated RefreshKind configuration (3×)
checksec.rs/src/main.rs ~lines 879, 901, 939
The block:
RefreshKind::nothing().with_processes(
ProcessRefreshKind::nothing()
.with_cpu()
.with_exe(UpdateKind::OnlyIfNotSet),
)is repeated verbatim in the procall, procids, and procname branches. Consider extracting to a local binding or helper to keep future changes a single-site edit.
Medium — Cargo.lock not committed (pre-existing)
This is a binary crate, so committing Cargo.lock is recommended by Cargo. With a 10-minor-version jump bringing in new transitive dependencies, reproducible builds become more important. Not introduced by this PR, but worth addressing alongside it.
Medium — MSRV raised significantly
sysinfo 0.38.2 requires Rust 1.88 (up from 1.59 for 0.28.2). The project has no rust-version field in Cargo.toml and CI uses stable, so this won't break today — but it's a meaningful floor increase worth documenting. The use of Option::is_none_or() (stabilized in Rust 1.82) also independently raises the project's effective MSRV.
Low — Sentinel value in unwrap_or
checksec.rs/src/main.rs ~line 921
process.exe().unwrap_or(Path::new("<unknown>")).display()Using a sentinel string for a missing path is a minor anti-pattern. Consider handling the Option explicitly in the format string (e.g., match or map_or) so "<unknown>" can't be confused with a real path.
🤖 Generated with Claude Code
No description provided.