-
Notifications
You must be signed in to change notification settings - Fork 3
Removing unwraps
#59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing unwraps
#59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
src/cache/file.rs
Outdated
| let _ =fs::create_dir_all(&cache_dir); | ||
|
|
Copilot
AI
May 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | ||
| } |
Copilot
AI
May 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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