removing unwraps (#59)

perryqh · web-flow · commit 4473bf5ecaed · 2025-05-12T13:28:26.000-05:00
diff --git a/src/cache/file.rs b/src/cache/file.rs
@@ -21,21 +21,25 @@ const DEFAULT_CACHE_CAPACITY: usize = 10000;
 
 impl Caching for GlobalCache {
     fn get_file_owner(&self, path: &Path) -> Result<Option<FileOwnerCacheEntry>, Error> {
-        if let Ok(cache) = self.file_owner_cache.as_ref().unwrap().lock() {
-            if let Some(cached_entry) = cache.get(path) {
-                let timestamp = get_file_timestamp(path)?;
-                if cached_entry.timestamp == timestamp {
-                    return Ok(Some(cached_entry.clone()));
+        if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
+            if let Ok(cache) = cache_mutex.lock() {
+                if let Some(cached_entry) = cache.get(path) {
+                    let timestamp = get_file_timestamp(path)?;
+                    if cached_entry.timestamp == timestamp {
+                        return Ok(Some(cached_entry.clone()));
+                    }
                 }
             }
         }
         Ok(None)
     }
 
     fn write_file_owner(&self, path: &Path, owner: Option<String>) {
-        if let Ok(mut cache) = self.file_owner_cache.as_ref().unwrap().lock() {
-            if let Ok(timestamp) = get_file_timestamp(path) {
-                cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner });
+        if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
+            if let Ok(mut cache) = cache_mutex.lock() {
+                if let Ok(timestamp) = get_file_timestamp(path) {
+                    cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner });
+                }
             }
         }
     }
@@ -50,8 +54,12 @@ impl Caching for GlobalCache {
             .change_context(Error::Io)?;
 
         let writer = BufWriter::new(file);
-        let cache = self.file_owner_cache.as_ref().unwrap().lock().map_err(|_| Error::Io)?;
-        serde_json::to_writer(writer, &*cache).change_context(Error::SerdeJson)
+        if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
+            let cache = cache_mutex.lock().map_err(|_| Error::Io)?;
+            serde_json::to_writer(writer, &*cache).change_context(Error::SerdeJson)
+        } else {
+            Ok(())
+        }
     }
 
     fn delete_cache(&self) -> Result<(), Error> {
@@ -91,7 +99,7 @@ impl GlobalCache {
 
     fn get_cache_path(&self) -> PathBuf {
         let cache_dir = self.base_path.join(PathBuf::from(&self.cache_directory));
-        fs::create_dir_all(&cache_dir).unwrap();
+        let _ = fs::create_dir_all(&cache_dir);
 
         cache_dir.join("project-file-cache.json")
     }
diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs
@@ -21,7 +21,10 @@ impl Mapper for DirectoryMapper {
         let team_by_name = self.project.teams_by_name.clone();
 
         for directory_codeowner_file in &self.project.directory_codeowner_files {
-            let dir_root = directory_codeowner_file.directory_root().to_string_lossy();
+            let dir_root = directory_codeowner_file
+                .directory_root()
+                .map(|p| p.to_string_lossy())
+                .unwrap_or_default();
             let team = team_by_name.get(&directory_codeowner_file.owner);
             if let Some(team) = team {
                 entries.push(Entry {
@@ -41,9 +44,12 @@ impl Mapper for DirectoryMapper {
 
         for file in &self.project.directory_codeowner_files {
             owner_matchers.push(OwnerMatcher::new_glob(
-                format!("{}/**/**", escape_brackets(&file.directory_root().to_string_lossy())),
+                format!(
+                    "{}/**/**",
+                    escape_brackets(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default())
+                ),
                 file.owner.to_owned(),
-                Source::Directory(file.directory_root().to_string_lossy().to_string()),
+                Source::Directory(file.directory_root().map(|p| p.to_string_lossy().to_string()).unwrap_or_default()),
             ));
         }
 
diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs
@@ -69,16 +69,18 @@ impl PackageMapper {
         let team_by_name = self.project.teams_by_name.clone();
 
         for package in self.project.packages.iter().filter(|package| &package.package_type == package_type) {
-            let package_root = package.package_root().to_string_lossy();
-            let team = team_by_name.get(&package.owner);
-
-            if let Some(team) = team {
-                entries.push(Entry {
-                    path: format!("{}/**/**", package_root),
-                    github_team: team.github_team.to_owned(),
-                    team_name: team.name.to_owned(),
-                    disabled: team.avoid_ownership,
-                });
+            if let Some(package_root) = package.package_root() {
+                let package_root = package_root.to_string_lossy();
+                let team = team_by_name.get(&package.owner);
+
+                if let Some(team) = team {
+                    entries.push(Entry {
+                        path: format!("{}/**/**", package_root),
+                        github_team: team.github_team.to_owned(),
+                        team_name: team.name.to_owned(),
+                        disabled: team.avoid_ownership,
+                    });
+                }
             }
         }
 
@@ -97,15 +99,17 @@ impl PackageMapper {
         let packages = remove_nested_packages(&packages);
 
         for package in packages {
-            let package_root = package.package_root().to_string_lossy();
-            let team = team_by_name.get(&package.owner);
-
-            if let Some(team) = team {
-                owner_matchers.push(OwnerMatcher::new_glob(
-                    format!("{}/**/**", package_root),
-                    team.name.to_owned(),
-                    Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)),
-                ));
+            if let Some(package_root) = package.package_root() {
+                let package_root = package_root.to_string_lossy();
+                let team = team_by_name.get(&package.owner);
+
+                if let Some(team) = team {
+                    owner_matchers.push(OwnerMatcher::new_glob(
+                        format!("{}/**/**", package_root),
+                        team.name.to_owned(),
+                        Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)),
+                    ));
+                }
             }
         }
 
@@ -118,9 +122,10 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> {
 
     for package in packages.iter().sorted_by_key(|package| package.package_root()) {
         if let Some(last_package) = top_level_packages.last() {
-            let last_package_root = last_package.package_root();
-            if !package.package_root().starts_with(last_package_root) {
-                top_level_packages.push(package);
+            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);
+                }
             }
         } else {
             top_level_packages.push(package);
diff --git a/src/project.rs b/src/project.rs
@@ -66,8 +66,8 @@ pub struct Package {
 }
 
 impl Package {
-    pub fn package_root(&self) -> &Path {
-        self.path.parent().unwrap()
+    pub fn package_root(&self) -> Option<&Path> {
+        self.path.parent()
     }
 }
 
@@ -78,8 +78,8 @@ pub struct DirectoryCodeownersFile {
 }
 
 impl DirectoryCodeownersFile {
-    pub fn directory_root(&self) -> &Path {
-        self.path.parent().unwrap()
+    pub fn directory_root(&self) -> Option<&Path> {
+        self.path.parent()
     }
 }
 
diff --git a/src/project_builder.rs b/src/project_builder.rs
@@ -78,10 +78,18 @@ impl<'a> ProjectBuilder<'a> {
             .to_lowercase();
 
         match file_name.as_str() {
-            name if name == "package.yml" && matches_globs(relative_path.parent().unwrap(), &self.config.ruby_package_paths) => {
+            name if name == "package.yml"
+                && relative_path
+                    .parent()
+                    .is_some_and(|parent| matches_globs(parent, &self.config.ruby_package_paths)) =>
+            {
                 Ok(EntryType::RubyPackage(absolute_path.to_owned(), relative_path.to_owned()))
             }
-            name if name == "package.json" && matches_globs(relative_path.parent().unwrap(), &self.config.javascript_package_paths) => {
+            name if name == "package.json"
+                && relative_path
+                    .parent()
+                    .is_some_and(|parent| matches_globs(parent, &self.config.javascript_package_paths)) =>
+            {
                 Ok(EntryType::JavascriptPackage(absolute_path.to_owned(), relative_path.to_owned()))
             }
             ".codeowner" => Ok(EntryType::CodeownerFile(absolute_path.to_owned(), relative_path.to_owned())),
@@ -124,31 +132,47 @@ impl<'a> ProjectBuilder<'a> {
                             }
                         }
                         EntryType::RubyPackage(absolute_path, relative_path) => {
-                            if let Some(owner) = ruby_package_owner(&absolute_path).unwrap() {
-                                pkgs.push(Package {
-                                    path: relative_path.clone(),
-                                    owner,
-                                    package_type: PackageType::Ruby,
-                                });
+                            match ruby_package_owner(&absolute_path) {
+                                Ok(Some(owner)) => {
+                                    pkgs.push(Package {
+                                        path: relative_path.clone(),
+                                        owner,
+                                        package_type: PackageType::Ruby,
+                                    });
+                                }
+                                Ok(None) => { /* No owner, do nothing */ }
+                                Err(e) => {
+                                    warn!("Error reading ruby package owner for {:?}: {:?}", absolute_path, e);
+                                }
                             }
                         }
                         EntryType::JavascriptPackage(absolute_path, relative_path) => {
-                            if let Some(owner) = javascript_package_owner(&absolute_path).unwrap() {
-                                pkgs.push(Package {
+                            match javascript_package_owner(&absolute_path) {
+                                Ok(Some(owner)) => {
+                                    pkgs.push(Package {
+                                        path: relative_path.clone(),
+                                        owner,
+                                        package_type: PackageType::Javascript,
+                                    });
+                                }
+                                Ok(None) => { /* No owner, do nothing */ }
+                                Err(e) => {
+                                    warn!("Error reading javascript package owner for {:?}: {:?}", absolute_path, e);
+                                }
+                            }
+                        }
+                        EntryType::CodeownerFile(absolute_path, relative_path) => match std::fs::read_to_string(&absolute_path) {
+                            Ok(owner) => {
+                                let owner = owner.trim().to_owned();
+                                codeowners.push(DirectoryCodeownersFile {
                                     path: relative_path.clone(),
                                     owner,
-                                    package_type: PackageType::Javascript,
                                 });
                             }
-                        }
-                        EntryType::CodeownerFile(absolute_path, relative_path) => {
-                            let owner = std::fs::read_to_string(absolute_path).unwrap();
-                            let owner = owner.trim().to_owned();
-                            codeowners.push(DirectoryCodeownersFile {
-                                path: relative_path.clone(),
-                                owner,
-                            });
-                        }
+                            Err(e) => {
+                                warn!("Error reading codeowner file for {:?}: {:?}", absolute_path, e);
+                            }
+                        },
                         EntryType::TeamFile(absolute_path, _relative_path) => match Team::from_team_file_path(absolute_path) {
                             Ok(team) => {
                                 team_files.push(team);

Original file line number	Diff line number	Diff line change
`@@ -66,8 +66,8 @@ pub struct Package {`
`66`	`66`	`}`
`67`	`67`
`68`	`68`	`impl Package {`
`69`		`- pub fn package_root(&self) -> &Path {`
`70`		`- self.path.parent().unwrap()`
	`69`	`+ pub fn package_root(&self) -> Option<&Path> {`
	`70`	`+ self.path.parent()`
`71`	`71`	`}`
`72`	`72`	`}`
`73`	`73`
`@@ -78,8 +78,8 @@ pub struct DirectoryCodeownersFile {`
`78`	`78`	`}`
`79`	`79`
`80`	`80`	`impl DirectoryCodeownersFile {`
`81`		`- pub fn directory_root(&self) -> &Path {`
`82`		`- self.path.parent().unwrap()`
	`81`	`+ pub fn directory_root(&self) -> Option<&Path> {`
	`82`	`+ self.path.parent()`
`83`	`83`	`}`
`84`	`84`	`}`
`85`	`85`