Merge pull request #140 from ouch-org/improving-error-treatment

Improving error messages and removing dead error treatment code

src/commands.rs

@@ -19,8 +19,8 @@ use crate::{
         Extension,
     },
     info,
-    utils::{self, dir_is_empty, nice_directory_display, to_utf},
+    utils::{self, concatenate_list_of_os_str, dir_is_empty, nice_directory_display, to_utf},
-    Error, Opts, QuestionPolicy, Subcommand,
+    Opts, QuestionPolicy, Subcommand,
 };
 
 // Used in BufReader and BufWriter to perform less syscalls
@@ -36,8 +36,7 @@ fn represents_several_files(files: &[PathBuf]) -> bool {
     files.iter().any(is_non_empty_dir) || files.len() > 1
 }
 
-/// Entrypoint of ouch, receives cli options and matches Subcommand
+/// Entrypoint of ouch, receives cli options and matches Subcommand to decide what to do
-/// to decide what to do
 pub fn run(args: Opts, question_policy: QuestionPolicy) -> crate::Result<()> {
     match args.cmd {
         Subcommand::Compress { files, output: output_path } => {
@@ -45,15 +44,16 @@ pub fn run(args: Opts, question_policy: QuestionPolicy) -> crate::Result<()> {
             let mut formats = extension::extensions_from_path(&output_path);
 
             if formats.is_empty() {
-                let reason = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
+                let error = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
-                    .detail("You shall supply the compression format via the extension.")
+                    .detail("You shall supply the compression format")
-                    .hint("Try adding something like .tar.gz or .zip to the output file.")
+                    .hint("Try adding supported extensions (see --help):")
+                    .hint(format!("  ouch compress <FILES>... {}.tar.gz", to_utf(&output_path)))
+                    .hint(format!("  ouch compress <FILES>... {}.zip", to_utf(&output_path)))
                     .hint("")
-                    .hint("Examples:")
+                    .hint("Alternatively, you can overwrite this option by using the '--format' flag:")
-                    .hint(format!("  ouch compress ... {}.tar.gz", to_utf(&output_path)))
+                    .hint(format!("  ouch compress <FILES>... {} --format tar.gz", to_utf(&output_path)));
-                    .hint(format!("  ouch compress ... {}.zip", to_utf(&output_path)));
 
-                return Err(Error::with_reason(reason));
+                return Err(error.into());
             }
 
             if !formats.get(0).map(Extension::is_archive).unwrap_or(false) && represents_several_files(&files) {
@@ -73,29 +73,29 @@ pub fn run(args: Opts, question_policy: QuestionPolicy) -> crate::Result<()> {
                 let mut suggested_output_path = output_path.clone();
                 suggested_output_path.replace_range(empty_range, ".tar");
 
-                let reason = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
+                let error = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
                     .detail("You are trying to compress multiple files.")
                     .detail(format!("The compression format '{}' cannot receive multiple files.", &formats[0]))
                     .detail("The only supported formats that archive files into an archive are .tar and .zip.")
                     .hint(format!("Try inserting '.tar' or '.zip' before '{}'.", &formats[0]))
                     .hint(format!("From: {}", output_path))
-                    .hint(format!(" To : {}", suggested_output_path));
+                    .hint(format!("To:   {}", suggested_output_path));
 
-                return Err(Error::with_reason(reason));
+                return Err(error.into());
             }
 
             if let Some(format) = formats.iter().skip(1).find(|format| format.is_archive()) {
-                let reason = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
+                let error = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
                     .detail(format!("Found the format '{}' in an incorrect position.", format))
                     .detail(format!("'{}' can only be used at the start of the file extension.", format))
                     .hint(format!("If you wish to compress multiple files, start the extension with '{}'.", format))
                     .hint(format!("Otherwise, remove the last '{}' from '{}'.", format, to_utf(&output_path)));
 
-                return Err(Error::with_reason(reason));
+                return Err(error.into());
             }
 
             if output_path.exists() && !utils::user_wants_to_overwrite(&output_path, question_policy)? {
-                // User does not want to overwrite this file
+                // User does not want to overwrite this file, skip and return without any errors
                 return Ok(());
             }
 
@@ -176,15 +176,20 @@ pub fn run(args: Opts, question_policy: QuestionPolicy) -> crate::Result<()> {
                 .map(|(input_path, _)| PathBuf::from(input_path))
                 .collect();
 
-            // Error
             if !files_missing_format.is_empty() {
-                eprintln!("Some file you asked ouch to decompress lacks a supported extension.");
+                let error = FinalError::with_title("Cannot decompress files without extensions")
-                eprintln!("Could not decompress {}.", to_utf(&files_missing_format[0]));
+                    .detail(format!(
-                todo!(
+                        "Files without supported extensions: {}",
-                    "Dev note: add this error variant and pass the Vec to it, all the files \
+                        concatenate_list_of_os_str(&files_missing_format)
-                     lacking extension shall be shown: {:#?}.",
+                    ))
-                    files_missing_format
+                    .detail("Decompression formats are detected automatically by the file extension")
-                );
+                    .hint("Provide a file with a supported extension:")
+                    .hint("  ouch decompress example.tar.gz")
+                    .hint("")
+                    .hint("Or overwrite this option with the '--format' flag:")
+                    .hint(format!("  ouch decompress {} --format tar.gz", to_utf(&files_missing_format[0])));
+
+                return Err(error.into());
             }
 
             // From Option<PathBuf> to Option<&Path>

src/dialogs.rs

@@ -33,7 +33,7 @@ impl<'a> Confirmation<'a> {
     pub fn ask(&self, substitute: Option<&'a str>) -> crate::Result<bool> {
         let message = match (self.placeholder, substitute) {
             (None, _) => Cow::Borrowed(self.prompt),
-            (Some(_), None) => return Err(crate::Error::InternalError),
+            (Some(_), None) => unreachable!("dev error, should be reported, we checked this won't happen"),
             (Some(placeholder), Some(subs)) => Cow::Owned(self.prompt.replace(placeholder, subs)),
         };
 

src/error.rs

@@ -9,51 +9,28 @@ use std::{
 
 use crate::utils::colors::*;
 
+#[allow(missing_docs)]
 /// All errors that can be generated by `ouch`
 #[derive(Debug, PartialEq)]
 pub enum Error {
-    /// Extension found is not supported and known to ouch
-    UnknownExtensionError(String),
-    /// TO BE REMOVED
-    MissingExtensionError(PathBuf),
     /// Not every IoError, some of them get filtered by `From<io::Error>` into other variants
-    IoError {
+    IoError { reason: String },
-        /// TODO
-        reason: String,
-    },
     /// Detected from io::Error if .kind() is io::ErrorKind::NotFound
     FileNotFound(PathBuf),
-    /// TO BE REMOVED
+    /// NEEDS MORE CONTEXT
     AlreadyExists,
-    /// TO BE REMOVED
+    /// From zip::result::ZipError::InvalidArchive
     InvalidZipArchive(&'static str),
     /// Detected from io::Error if .kind() is io::ErrorKind::PermissionDenied
-    PermissionDenied {
+    PermissionDenied { error_title: String },
-        /// TODO
+    /// From zip::result::ZipError::UnsupportedArchive
-        error_title: String,
-    },
-    /// TO BE REMOVED
     UnsupportedZipArchive(&'static str),
     /// TO BE REMOVED
-    InternalError,
-    /// TO BE REMOVED
     CompressingRootFolder,
-    /// TO BE REMOVED
-    MissingArgumentsForCompression,
-    /// TO BE REMOVED
-    MissingArgumentsForDecompression,
-    /// TO BE REMOVED
-    CompressionTypo,
     /// Specialized walkdir's io::Error wrapper with additional information on the error
-    WalkdirError {
+    WalkdirError { reason: String },
-        /// TODO
-        reason: String,
-    },
     /// Custom and unique errors are reported in this variant
-    Custom {
+    Custom { reason: FinalError },
-        /// TODO
-        reason: FinalError,
-    },
 }
 
 /// Alias to std's Result with ouch's Error
@@ -115,12 +92,6 @@ impl FinalError {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let err = match self {
-            Error::MissingExtensionError(filename) => {
-                FinalError::with_title(format!("Cannot compress to {:?}", filename))
-                    .detail("Ouch could not detect the compression format")
-                    .hint("Use a supported format extension, like '.zip' or '.tar.gz'")
-                    .hint("Check https://github.com/ouch-org/ouch for a full list of supported formats")
-            }
             Error::WalkdirError { reason } => FinalError::with_title(reason),
             Error::FileNotFound(file) => {
                 if file == Path::new("") {
@@ -134,36 +105,7 @@ impl fmt::Display for Error {
                     .detail("This is unadvisable since ouch does compressions in-memory.")
                     .hint("Use a more appropriate tool for this, such as rsync.")
             }
-            Error::MissingArgumentsForCompression => {
-                FinalError::with_title("Could not compress")
-                    .detail("The compress command requires at least 2 arguments")
-                    .hint("You must provide:")
-                    .hint("  - At least one input argument.")
-                    .hint("  - The output argument.")
-                    .hint("")
-                    .hint("Example: `ouch compress image.png img.zip`")
-            }
-            Error::MissingArgumentsForDecompression => {
-                FinalError::with_title("Could not decompress")
-                    .detail("The compress command requires at least one argument")
-                    .hint("You must provide:")
-                    .hint("  - At least one input argument.")
-                    .hint("")
-                    .hint("Example: `ouch decompress imgs.tar.gz`")
-            }
-            Error::InternalError => {
-                FinalError::with_title("InternalError :(")
-                    .detail("This should not have happened")
-                    .detail("It's probably our fault")
-                    .detail("Please help us improve by reporting the issue at:")
-                    .detail(format!("    {}https://github.com/ouch-org/ouch/issues ", *CYAN))
-            }
             Error::IoError { reason } => FinalError::with_title(reason),
-            Error::CompressionTypo => {
-                FinalError::with_title("Possible typo detected")
-                    .hint(format!("Did you mean '{}ouch compress{}'?", *MAGENTA, *RESET))
-            }
-            Error::UnknownExtensionError(_) => todo!(),
             Error::AlreadyExists => todo!(),
             Error::InvalidZipArchive(_) => todo!(),
             Error::PermissionDenied { error_title } => FinalError::with_title(error_title).detail("Permission denied"),
@@ -175,13 +117,6 @@ impl fmt::Display for Error {
     }
 }
 
-impl Error {
-    /// TO BE REMOVED
-    pub fn with_reason(reason: FinalError) -> Self {
-        Self::Custom { reason }
-    }
-}
-
 impl From<std::io::Error> for Error {
     fn from(err: std::io::Error) -> Self {
         match err.kind() {

src/utils.rs

@@ -72,6 +72,21 @@ pub fn to_utf(os_str: impl AsRef<OsStr>) -> String {
     text.trim_matches('"').to_string()
 }
 
+/// Converts a slice of AsRef<OsStr> to comma separated String
+///
+/// Panics if the slice is empty.
+pub fn concatenate_list_of_os_str(os_strs: &[impl AsRef<OsStr>]) -> String {
+    let mut iter = os_strs.iter().map(AsRef::as_ref);
+
+    let mut string = to_utf(iter.next().unwrap()); // May panic
+
+    for os_str in iter {
+        string += ", ";
+        string += &to_utf(os_str);
+    }
+    string
+}
+
 /// Display the directory name, but change to "current directory" when necessary.
 pub fn nice_directory_display(os_str: impl AsRef<OsStr>) -> String {
     let text = to_utf(os_str);