From 964c415d9e65ffa8be7dacc313aabc2653ebf49f Mon Sep 17 00:00:00 2001 From: Tarek Date: Fri, 24 May 2024 23:38:33 +0300 Subject: [PATCH] fix(ark-cli): avoid unwrap() and add FIXMEs Signed-off-by: Tarek --- ark-cli/src/commands/backup.rs | 2 ++ ark-cli/src/commands/collisions.rs | 1 + ark-cli/src/commands/link/utils.rs | 1 + ark-cli/src/commands/list.rs | 3 +++ ark-cli/src/commands/monitor.rs | 3 +++ ark-cli/src/commands/render.rs | 13 +++++++++---- ark-cli/src/index_registrar.rs | 8 +++++--- ark-cli/src/main.rs | 2 ++ ark-cli/src/util.rs | 14 +++++++++++--- 9 files changed, 37 insertions(+), 10 deletions(-) diff --git a/ark-cli/src/commands/backup.rs b/ark-cli/src/commands/backup.rs index 398220bd..abaf54ed 100644 --- a/ark-cli/src/commands/backup.rs +++ b/ark-cli/src/commands/backup.rs @@ -15,6 +15,8 @@ pub struct Backup { } impl Backup { + // FIXME: The logic for backup should be moved out of `ark-cli` + // so that it can be used by other applications pub fn run(&self) -> Result<(), AppError> { let timestamp = timestamp().as_secs(); let backup_dir = home_dir() diff --git a/ark-cli/src/commands/collisions.rs b/ark-cli/src/commands/collisions.rs index 69c26c17..ea3c320a 100644 --- a/ark-cli/src/commands/collisions.rs +++ b/ark-cli/src/commands/collisions.rs @@ -14,6 +14,7 @@ pub struct Collisions { impl Collisions { pub fn run(&self) -> Result<(), AppError> { + // FIXME: How does `monitor_index` handle `ark-cli collisions`? monitor_index(&self.root_dir, None) } } diff --git a/ark-cli/src/commands/link/utils.rs b/ark-cli/src/commands/link/utils.rs index 122f7f1c..4830219b 100644 --- a/ark-cli/src/commands/link/utils.rs +++ b/ark-cli/src/commands/link/utils.rs @@ -38,6 +38,7 @@ pub fn load_link( Err(AppError::LinkLoadError(format!( "Path {:?} was requested. But id {} maps to path {:?}", path, + // FIXME: If `load_link` needs id, then it shouldn't be an optional parameter id.clone().unwrap(), path2, ))) diff --git a/ark-cli/src/commands/list.rs b/ark-cli/src/commands/list.rs index acafc92c..8dc70661 100644 --- a/ark-cli/src/commands/list.rs +++ b/ark-cli/src/commands/list.rs @@ -179,6 +179,9 @@ impl List { let no_tags = "NO_TAGS"; let no_scores = "NO_SCORE"; + // FIXME: We should use a table library to print this. + // probably `comfy-table` + let longest_path = storage_entries .iter() .map(|entry| { diff --git a/ark-cli/src/commands/monitor.rs b/ark-cli/src/commands/monitor.rs index 1ad1c3df..21cda1ef 100644 --- a/ark-cli/src/commands/monitor.rs +++ b/ark-cli/src/commands/monitor.rs @@ -7,12 +7,15 @@ use crate::{monitor_index, AppError}; pub struct Monitor { #[clap(value_parser, help = "Path to the root directory")] root_dir: Option, + // FIXME: help message should specify what metric the interval is in #[clap(help = "Interval to check for changes")] interval: Option, } impl Monitor { pub fn run(&self) -> Result<(), AppError> { + // FIXME: 1000 should be the default value in clap configuration + // so users know let millis = self.interval.unwrap_or(1000); monitor_index(&self.root_dir, Some(millis)) } diff --git a/ark-cli/src/commands/render.rs b/ark-cli/src/commands/render.rs index cba8bbd5..7f3fa9e6 100644 --- a/ark-cli/src/commands/render.rs +++ b/ark-cli/src/commands/render.rs @@ -6,24 +6,27 @@ use crate::{render_preview_page, AppError, File, PDFQuality}; #[clap(name = "render", about = "Render a PDF file to an image")] pub struct Render { #[clap(value_parser, help = "The path to the file to render")] - path: Option, + path: PathBuf, #[clap(help = "The quality of the rendering")] quality: Option, } impl Render { pub fn run(&self) -> Result<(), AppError> { - let filepath = self.path.to_owned().unwrap(); + let filepath = self.path.to_owned(); let quality = match self.quality.to_owned().unwrap().as_str() { "high" => Ok(PDFQuality::High), "medium" => Ok(PDFQuality::Medium), "low" => Ok(PDFQuality::Low), _ => Err(AppError::InvalidRenderOption), }?; - let buf = File::open(&filepath).unwrap(); + let buf = File::open(&filepath).map_err(|e| { + AppError::FileOperationError(format!("Failed to open file: {}", e)) + })?; let dest_path = filepath.with_file_name( filepath .file_stem() + // SAFETY: we know that the file stem is valid UTF-8 because it is a file name .unwrap() .to_str() .unwrap() @@ -31,7 +34,9 @@ impl Render { + ".png", ); let img = render_preview_page(buf, quality); - img.save(dest_path).unwrap(); + img.save(dest_path).map_err(|e| { + AppError::FileOperationError(format!("Failed to save image: {}", e)) + })?; Ok(()) } } diff --git a/ark-cli/src/index_registrar.rs b/ark-cli/src/index_registrar.rs index 0ab5678a..fc6a2e5b 100644 --- a/ark-cli/src/index_registrar.rs +++ b/ark-cli/src/index_registrar.rs @@ -1,7 +1,7 @@ use lazy_static::lazy_static; extern crate canonical_path; -use data_error::Result; +use data_error::{ArklibError, Result}; use fs_index::ResourceIndex; use std::collections::HashMap; @@ -25,7 +25,7 @@ pub fn provide_index>( let root_path = CanonicalPathBuf::canonicalize(root_path)?; { - let registrar = REGISTRAR.read().unwrap(); + let registrar = REGISTRAR.read().map_err(|_| ArklibError::Parse)?; if let Some(index) = registrar.get(&root_path) { log::info!("Index has been registered before"); @@ -36,7 +36,9 @@ pub fn provide_index>( log::info!("Index has not been registered before"); match ResourceIndex::provide(&root_path) { Ok(index) => { - let mut registrar = REGISTRAR.write().unwrap(); + let mut registrar = REGISTRAR.write().map_err(|_| { + ArklibError::Other(anyhow::anyhow!("Failed to lock registrar")) + })?; let arc = Arc::new(RwLock::new(index)); registrar.insert(root_path, arc.clone()); diff --git a/ark-cli/src/main.rs b/ark-cli/src/main.rs index 3248585c..c8c718ca 100644 --- a/ark-cli/src/main.rs +++ b/ark-cli/src/main.rs @@ -105,6 +105,8 @@ async fn main() -> anyhow::Result<()> { let _ = app_id::load(ark_dir) .map_err(|e| AppError::AppIdLoadError(e.to_string()))?; + // Having a separate function for the main logic allows for easier + // error handling and testing. if let Err(err) = run().await { eprintln!("Error: {:#}", err); std::process::exit(1); diff --git a/ark-cli/src/util.rs b/ark-cli/src/util.rs index 2b1a6d16..9a370167 100644 --- a/ark-cli/src/util.rs +++ b/ark-cli/src/util.rs @@ -73,7 +73,7 @@ pub fn provide_root(root_dir: &Option) -> Result { pub fn provide_index(root_dir: &PathBuf) -> ResourceIndex { let rwlock = crate::provide_index(root_dir).expect("Failed to retrieve index"); - let index = &*rwlock.read().unwrap(); + let index = &*rwlock.read().expect("Failed to lock index"); index.clone() } @@ -94,7 +94,11 @@ pub fn monitor_index( println!("Build succeeded in {:?}\n", duration); if let Some(millis) = interval { - let mut index = rwlock.write().unwrap(); + let mut index = rwlock.write().map_err(|_| { + AppError::StorageCreationError( + "Failed to write lock index".to_owned(), + ) + })?; loop { let pause = Duration::from_millis(millis); thread::sleep(pause); @@ -117,7 +121,11 @@ pub fn monitor_index( } } } else { - let index = rwlock.read().unwrap(); + let index = rwlock.read().map_err(|_| { + AppError::StorageCreationError( + "Failed to read lock index".to_owned(), + ) + })?; println!("Here are {} entries in the index", index.size());