Skip to content

Improving error messages and removing dead error treatment code #140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marcospb19 merged 6 commits into from
Nov 2, 2021
Merged

Improving error messages and removing dead error treatment code #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6cebf78
Simplify Error::Custom code conversions
marcospb19 Nov 2, 2021
fadc412
Improve error message when no extensions given to decompress
marcospb19 Nov 2, 2021
047102e
Remove InternalError, replaced by unreachable! macro
marcospb19 Nov 2, 2021
21134aa
Improve message when no extensions given to compress file
marcospb19 Nov 2, 2021
5c0f24f
Change alignment of error message replacement suggestion
marcospb19 Nov 2, 2021
2fcec33
Removing obsolete error variants
marcospb19 Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 30 additions & 25 deletions src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use crate::{
Extension,
},
info,
utils::{self, dir_is_empty, nice_directory_display, to_utf},
Error, Opts, QuestionPolicy, Subcommand,
utils::{self, concatenate_list_of_os_str, dir_is_empty, nice_directory_display, to_utf},
Opts, QuestionPolicy, Subcommand,
};

// Used in BufReader and BufWriter to perform less syscalls
Expand All @@ -36,24 +36,24 @@ 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
/// to decide what to do
/// Entrypoint of ouch, receives cli options and matches Subcommand to decide what to do
pub fn run(args: Opts, question_policy: QuestionPolicy) -> crate::Result<()> {
match args.cmd {
Subcommand::Compress { files, output: output_path } => {
// Formats from path extension, like "file.tar.gz.xz" -> vec![Tar, Gzip, Lzma]
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)))
.detail("You shall supply the compression format via the extension.")
.hint("Try adding something like .tar.gz or .zip to the output file.")
let error = FinalError::with_title(format!("Cannot compress to '{}'.", to_utf(&output_path)))
.detail("You shall supply the compression format")
.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(format!(" ouch compress ... {}.tar.gz", to_utf(&output_path)))
.hint(format!(" ouch compress ... {}.zip", to_utf(&output_path)));
.hint("Alternatively, you can overwrite this option by using the '--format' flag:")
.hint(format!(" ouch compress <FILES>... {} --format tar.gz", 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) {
Expand All @@ -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(());
}

Expand Down Expand Up @@ -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.");
eprintln!("Could not decompress {}.", to_utf(&files_missing_format[0]));
todo!(
"Dev note: add this error variant and pass the Vec to it, all the files \
lacking extension shall be shown: {:#?}.",
files_missing_format
);
let error = FinalError::with_title("Cannot decompress files without extensions")
.detail(format!(
"Files without supported extensions: {}",
concatenate_list_of_os_str(&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>
Expand Down
2 changes: 1 addition & 1 deletion src/dialogs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
};

Expand Down
81 changes: 8 additions & 73 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/// TODO
reason: String,
},
IoError { 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 {
/// TODO
error_title: String,
},
/// TO BE REMOVED
PermissionDenied { error_title: String },
/// From zip::result::ZipError::UnsupportedArchive
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 {
/// TODO
reason: String,
},
WalkdirError { reason: String },
/// Custom and unique errors are reported in this variant
Custom {
/// TODO
reason: FinalError,
},
Custom { reason: FinalError },
}

/// Alias to std's Result with ouch's Error
Expand Down Expand Up @@ -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("") {
Expand All @@ -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"),
Expand All @@ -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() {
Expand Down
15 changes: 15 additions & 0 deletions src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down