fix(ark-cli): avoid unwrap() and add FIXMEs

Signed-off-by: Tarek <[email protected]>

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()

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)
     }
 }

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,
                 )))

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| {

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<PathBuf>,
+    // FIXME: help message should specify what metric the interval is in
     #[clap(help = "Interval to check for changes")]
     interval: Option<u64>,
 }
 
 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))
     }

ark-cli/src/commands/render.rs

@@ -6,32 +6,37 @@ 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<PathBuf>,
+    path: PathBuf,
     #[clap(help = "The quality of the rendering")]
     quality: Option<String>,
 }
 
 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()
                 .to_owned()
                 + ".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(())
     }
 }

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<P: AsRef<Path>>(
     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<P: AsRef<Path>>(
     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());
 

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);

ark-cli/src/util.rs

@@ -73,7 +73,7 @@ pub fn provide_root(root_dir: &Option<PathBuf>) -> Result<PathBuf, AppError> {
 pub fn provide_index(root_dir: &PathBuf) -> ResourceIndex<ResourceId> {
     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());