Refactor error handling

6 files changed, 82 insertions, 185 deletions

diff --git a/Cargo.lock b/Cargo.lock
index cd50817..c9f920d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1107,6 +1107,7 @@ dependencies = [
  "serde_json",
  "serde_yaml",
  "termimad",
+ "thiserror",
 ]
 
 [[package]]
diff --git a/Cargo.toml b/Cargo.toml
index 076e22a..eccd5dc 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,3 +18,4 @@ serde_yaml = "0.8"
 termimad = "0.8"
 minimad = "0.6"
 lazy_static = "1.4"
+thiserror = "1.0"
diff --git a/src/config.rs b/src/config.rs
index 0f09699..1d854b9 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -4,7 +4,7 @@ use std::fs;
 use std::fs::File;
 use std::path::PathBuf;
 
-use crate::error::{Error, Result};
+use crate::error::{Error, PermissionType, Result};
 
 #[derive(Deserialize, Serialize, Debug)]
 pub struct Config {
@@ -27,7 +27,8 @@ impl Default for Config {
 pub fn user_config() -> Result<Config> {
     let project = project_dir()?;
     let dir = project.config_dir();
-    fs::create_dir_all(&dir).map_err(|_| Error::create_dir(&dir.to_path_buf()))?;
+    fs::create_dir_all(&dir)
+        .map_err(|_| Error::Permissions(PermissionType::Create, dir.to_path_buf()))?;
     let filename = config_file_name()?;
     match File::open(&filename) {
         Err(_) => {
@@ -35,14 +36,16 @@ pub fn user_config() -> Result<Config> {
             write_config(&def)?;
             Ok(def)
         }
-        Ok(file) => serde_yaml::from_reader(file).map_err(|_| Error::malformed(&filename)),
+        Ok(file) => serde_yaml::from_reader(file).map_err(|_| Error::MalformedFile(filename)),
     }
 }
 
 fn write_config(config: &Config) -> Result<()> {
     let filename = config_file_name()?;
-    let file = File::create(&filename).map_err(|_| Error::create_file(&filename))?;
-    serde_yaml::to_writer(file, config).map_err(|_| Error::write_file(&filename))
+    let file = File::create(&filename)
+        .map_err(|_| Error::Permissions(PermissionType::Create, filename.clone()))?;
+    serde_yaml::to_writer(file, config)
+        .map_err(|_| Error::Permissions(PermissionType::Write, filename.clone()))
 }
 
 fn config_file_name() -> Result<PathBuf> {
@@ -51,12 +54,7 @@ fn config_file_name() -> Result<PathBuf> {
 
 /// Get project directory
 pub fn project_dir() -> Result<ProjectDirs> {
-    ProjectDirs::from("io", "Sam Tay", "so").ok_or_else(|| {
-        Error::os(
-            "Couldn't find a suitable project directory to store cache and configuration;\n\
-            this application may not be supported on your operating system.",
-        )
-    })
+    ProjectDirs::from("io", "Sam Tay", "so").ok_or_else(|| Error::ProjectDir)
 }
 
 pub fn set_api_key(key: String) -> Result<()> {
diff --git a/src/error.rs b/src/error.rs
index 9563359..d125fb2 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -2,95 +2,32 @@ use std::path::PathBuf;
 
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
-#[derive(Debug)]
-pub struct Error {
-    #[allow(dead_code)]
-    pub kind: ErrorKind,
-    pub error: String,
-}
-
-#[derive(Debug)]
-pub enum ErrorKind {
-    Malformed,
-    StackExchange,
-    Permissions,
-    OperatingSystem,
-    Panic,
+#[derive(thiserror::Error, Debug)]
+pub enum Error {
+    #[error("Termimad error: {0}")]
+    Termimad(#[from] termimad::Error),
+    #[error("Crossterm error: {0}")]
+    Crossterm(#[from] crossterm::ErrorKind),
+    #[error("Reqwest error: {0}")]
+    Reqwest(#[from] reqwest::Error),
+    #[error("IO error: {0}")]
+    IO(#[from] std::io::Error),
+    #[error("File `{}` is malformed; try removing it", .0.display())]
+    MalformedFile(PathBuf),
+    #[error("Lacking {0:?} permissions on `{}`", .1.display())]
+    Permissions(PermissionType, PathBuf),
+    #[error("{0}")]
+    StackExchange(String),
+    #[error("Couldn't find a suitable project directory; is your OS supported?")]
+    ProjectDir,
+    #[error("Empty sites file in cache")]
     EmptySites,
+    #[error("Sorry, couldn't find any answers for your query")]
     NoResults,
-    Termimad(termimad::Error),
 }
 
-impl From<&str> for Error {
-    fn from(err: &str) -> Self {
-        Error {
-            kind: ErrorKind::Panic,
-            error: String::from(err),
-        }
-    }
-}
-
-impl From<termimad::Error> for Error {
-    fn from(err: termimad::Error) -> Self {
-        Error {
-            kind: ErrorKind::Termimad(err),
-            error: String::from(""),
-        }
-    }
-}
-
-// TODO add others
-impl Error {
-    pub fn malformed(path: &PathBuf) -> Self {
-        Error {
-            kind: ErrorKind::Malformed,
-            error: format!("File `{}` is malformed; try removing it.", path.display()),
-        }
-    }
-    pub fn se(err: String) -> Self {
-        Error {
-            kind: ErrorKind::StackExchange,
-            error: err,
-        }
-    }
-    pub fn create_dir(path: &PathBuf) -> Self {
-        Error {
-            kind: ErrorKind::Permissions,
-            error: format!(
-                "Couldn't create directory `{}`; please check the permissions
-                on the parent directory",
-                path.display()
-            ),
-        }
-    }
-    pub fn create_file(path: &PathBuf) -> Self {
-        Error {
-            kind: ErrorKind::Permissions,
-            error: format!(
-                "Couldn't create file `{}`; please check the directory permissions",
-                path.display()
-            ),
-        }
-    }
-    pub fn write_file(path: &PathBuf) -> Self {
-        Error {
-            kind: ErrorKind::Permissions,
-            error: format!(
-                "Couldn't write to file `{}`; please check its permissions",
-                path.display()
-            ),
-        }
-    }
-    pub fn os(err: &str) -> Self {
-        Error {
-            kind: ErrorKind::OperatingSystem,
-            error: String::from(err),
-        }
-    }
-    pub fn no_results() -> Self {
-        Error {
-            kind: ErrorKind::NoResults,
-            error: String::from("Sorry, no answers found for your question. Try another query."),
-        }
-    }
+#[derive(Debug)]
+pub enum PermissionType {
+    Create,
+    Write,
 }
diff --git a/src/main.rs b/src/main.rs
index 57d561c..70ab6f4 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -5,7 +5,7 @@ mod stackexchange;
 mod term;
 
 use crossterm::style::Color;
-use error::{Error, ErrorKind};
+use error::Error;
 use lazy_static::lazy_static;
 use minimad::mad_inline;
 use stackexchange::{LocalStorage, StackExchange};
@@ -45,53 +45,28 @@ fn main() {
             return Ok(());
         }
 
-        match ls.validate_site(site) {
-            Ok(true) => (),
-            Ok(false) => {
-                print_error!(skin, "$0 is not a valid StackExchange site.\n\n", site)?;
-                // TODO what about using text wrapping feature?
-                print_notice!(
-                    skin,
-                    "If you think this is incorrect, try running\n\
+        if !ls.validate_site(site)? {
+            print_error!(skin, "$0 is not a valid StackExchange site.\n\n", site)?;
+            // TODO what about using text wrapping feature?
+            print_notice!(
+                skin,
+                "If you think this is incorrect, try running\n\
                     ```\n\
                     so --update-sites\n\
                     ```\n\
                     to update the cached site listing. You can also run `so --list-sites` \
                     to list all available sites.",
-                )?;
-                return Ok(());
-            }
-            Err(Error {
-                kind: ErrorKind::EmptySites,
-                ..
-            }) => {
-                // TODO use text wrapping feature
-                print_error!(
-                    skin,
-                    "The cached site list is empty. This can likely be fixed by\n\n\
-                    ```\n\
-                    so --update-sites\n\
-                    ```"
-                )?;
-                return Ok(());
-            }
-            Err(e) => return Err(e),
+            )?;
+            return Ok(());
         }
 
         if let Some(q) = opts.query {
             let se = StackExchange::new(config);
             let que = se.search(&q)?;
-            let ans = que
-                .first()
-                .ok_or_else(Error::no_results)?
-                .answers
-                .first()
-                .ok_or_else(|| {
-                    Error::from(
-                        "StackExchange returned a question with no answers; \
-                        this shouldn't be possible!",
-                    )
-                })?;
+            let ans = que.first().ok_or(Error::NoResults)?.answers.first().expect(
+                "StackExchange returned a question with no answers; \
+                    this shouldn't be possible!",
+            );
             // TODO eventually do this in the right place, e.g. abstract out md parser, write benches, & do within threads
             let md = ans.body.replace("<kbd>", "**[").replace("</kbd>", "]**");
             skin.print_text(&md);
@@ -99,12 +74,16 @@ fn main() {
 
         Ok(())
     })()
-    .or_else(|e| {
+    .or_else(|e: Error| {
         with_error_style(&mut skin, |err_skin, stderr| {
-            err_skin.write_text_on(stderr, &e.error)
-        })
+            err_skin.write_text_on(stderr, &e.to_string())
+        })?;
+        match e {
+            Error::EmptySites => {
+                print_notice!(skin, "This can likely be fixed by `so --update-sites`.")
+            }
+            _ => Ok(()),
+        }
     })
-    .unwrap_or_else(|e| {
-        println!("panic! {}", e.error);
-    });
+    .unwrap();
 }
diff --git a/src/stackexchange.rs b/src/stackexchange.rs
index a790a2a..dce05de 100644
--- a/src/stackexchange.rs
+++ b/src/stackexchange.rs
@@ -8,7 +8,7 @@ use std::fs::File;
 use std::path::PathBuf;
 
 use crate::config::{project_dir, Config};
-use crate::error::{Error, ErrorKind, Result};
+use crate::error::{Error, PermissionType, Result};
 
 /// StackExchange API v2.2 URL
 const SE_API_URL: &str = "http://api.stackexchange.com";
@@ -96,18 +96,11 @@ impl StackExchange {
                 ("order", "desc"),
                 ("sort", "relevance"),
             ])
-            .send()
-            .map_err(|e| {
-                // TODO explore legit errors such as not connected to internet
-                Error::se(format!(
-                    "Error encountered while querying StackExchange: {}",
-                    e
-                ))
-            })?;
+            .send()?;
 
         let gz = GzDecoder::new(resp_body);
         let wrapper: ResponseWrapper<Question> = serde_json::from_reader(gz).map_err(|e| {
-            Error::se(format!(
+            Error::StackExchange(format!(
                 "Error decoding questions from the StackExchange API: {}",
                 e
             ))
@@ -138,7 +131,8 @@ impl LocalStorage {
     pub fn new() -> Result<Self> {
         let project = project_dir()?;
         let dir = project.cache_dir();
-        fs::create_dir_all(&dir).map_err(|_| Error::create_dir(&dir.to_path_buf()))?;
+        fs::create_dir_all(&dir)
+            .map_err(|_| Error::Permissions(PermissionType::Create, dir.to_path_buf()))?;
         Ok(LocalStorage {
             sites: None,
             filename: dir.join("sites.json"),
@@ -146,19 +140,16 @@ impl LocalStorage {
     }
 
     // TODO make this async, inform user if we are downloading
-    // TODO issue EmptySites from here when appropriate
     pub fn sites(&mut self) -> Result<&Vec<Site>> {
         // Stop once Option ~ Some or Result ~ Err
-        if self.sites.is_some() {
-            return Ok(self.sites.as_ref().unwrap()); // safe
+        if self.sites.is_none() && !self.fetch_local_sites()? {
+            self.fetch_remote_sites()?;
         }
-        if self.fetch_local_sites()?.is_some() {
-            return Ok(self.sites.as_ref().unwrap()); // safe
+        match &self.sites {
+            Some(sites) if sites.is_empty() => Err(Error::EmptySites),
+            Some(sites) => Ok(sites),
+            None => panic!("Code failure in site listing retrieval"),
         }
-        self.fetch_remote_sites()?;
-        self.sites
-            .as_ref()
-            .ok_or_else(|| Error::from("Code failure in site listing retrieval"))
     }
 
     pub fn update_sites(&mut self) -> Result<()> {
@@ -166,25 +157,19 @@ impl LocalStorage {
     }
 
     pub fn validate_site(&mut self, site_code: &str) -> Result<bool> {
-        let sites = self.sites()?;
-        if sites.is_empty() {
-            return Err(Error {
-                kind: ErrorKind::EmptySites,
-                error: String::from(""),
-            });
-        }
-        Ok(sites
+        Ok(self
+            .sites()?
             .iter()
             .any(|site| site.api_site_parameter == *site_code))
     }
 
-    fn fetch_local_sites(&mut self) -> Result<Option<()>> {
+    fn fetch_local_sites(&mut self) -> Result<bool> {
         if let Ok(file) = File::open(&self.filename) {
-            self.sites =
-                serde_json::from_reader(file).map_err(|_| Error::malformed(&self.filename))?;
-            return Ok(Some(()));
+            self.sites = serde_json::from_reader(file)
+                .map_err(|_| Error::MalformedFile(self.filename.clone()))?;
+            return Ok(true);
         }
-        Ok(None)
+        Ok(false)
     }
 
     // TODO decide whether or not I should give LocalStorage an api key..
@@ -197,16 +182,10 @@ impl LocalStorage {
                 ("pagesize", SE_SITES_PAGESIZE.to_string()),
                 ("page", "1".to_string()),
             ])
-            .send()
-            .map_err(|e| {
-                Error::se(format!(
-                    "Error requesting sites from StackExchange API: {}",
-                    e
-                ))
-            })?;
+            .send()?;
         let gz = GzDecoder::new(resp_body);
         let wrapper: ResponseWrapper<Site> = serde_json::from_reader(gz).map_err(|e| {
-            Error::se(format!(
+            Error::StackExchange(format!(
                 "Error decoding sites from the StackExchange API: {}",
                 e
             ))
@@ -216,8 +195,10 @@ impl LocalStorage {
     }
 
     fn store_local_sites(&self) -> Result<()> {
-        let file = File::create(&self.filename).map_err(|_| Error::create_file(&self.filename))?;
-        serde_json::to_writer(file, &self.sites).map_err(|_| Error::write_file(&self.filename))
+        let file = File::create(&self.filename)
+            .map_err(|_| Error::Permissions(PermissionType::Create, self.filename.clone()))?;
+        serde_json::to_writer(file, &self.sites)
+            .map_err(|_| Error::Permissions(PermissionType::Write, self.filename.clone()))
     }
 }