Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes several unwraps to gracefully handle None and error cases instead of panicking. Key changes include updating pattern matching in package file checks, refactoring functions to return Options instead of unwrapping parent paths, and replacing unwrap calls with error logging.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/project_builder.rs | Replaced unwrap usage for parent paths and package owner retrieval with match statements. |
| src/project.rs | Updated package_root and directory_root to return Options instead of unwrapping. |
| src/ownership/mapper/package_mapper.rs | Updated package mapping to account for Option return values from package_root. |
| src/ownership/mapper/directory_mapper.rs | Adjusted directory path handling by mapping Options to default strings when necessary. |
| src/cache/file.rs | Removed unwrap from directory creation, silently ignoring errors now. |
| let _ =fs::create_dir_all(&cache_dir); | ||
|
|
There was a problem hiding this comment.
Consider handling or logging errors from create_dir_all rather than silently ignoring them, as failed directory creation might affect file caching.
| let _ =fs::create_dir_all(&cache_dir); | |
| if let Err(e) = fs::create_dir_all(&cache_dir) { | |
| eprintln!("Failed to create cache directory {}: {}", cache_dir.display(), e); | |
| } |
| if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) { | ||
| if !current_root.starts_with(last_root) { | ||
| top_level_packages.push(package); | ||
| } |
There was a problem hiding this comment.
[nitpick] Review the handling of packages with no parent (i.e. when package_root() returns None) to ensure they are processed as intended or explicitly handled.
| if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) { | |
| if !current_root.starts_with(last_root) { | |
| top_level_packages.push(package); | |
| } | |
| match (package.package_root(), last_package.package_root()) { | |
| (Some(current_root), Some(last_root)) if !current_root.starts_with(last_root) => { | |
| top_level_packages.push(package); | |
| } | |
| (None, _) | (_, None) => { | |
| // Explicitly handle packages with no parent | |
| top_level_packages.push(package); | |
| } | |
| _ => {} |
Handle
NoneorErrorinstead of panicking