Improve error handling #1
Conversation
|
The Edit: Now removed |
| } | ||
|
|
||
| /// Finds all `.jpg`, `.jpeg`, `.png` and `.webp` images within a directory. | ||
| /// | ||
| /// Throws an error if: | ||
| /// - The directory is invalid or does not contain any images. | ||
| /// - The directory does not contain any jpg, jpeg, png, or webp images. | ||
| pub fn find_images(directory_path: &str) -> anyhow::Result<Vec<PathBuf>> { | ||
| let mut images: Vec<_> = read_dir(directory_path)? | ||
| pub fn find_images(directory_path: impl AsRef<Path>) -> Result<Vec<PathBuf>, ImageLoaderError> { |
There was a problem hiding this comment.
Changed this function to accept a generic parameter to make it more flexible. This means you can, for instance, provide a &PathBuf instead of a &str (useful for application code).
Note that there is a compile-time cost due to monomorphization, but if that becomes a problem, I can fix it with the non-generic inner function hack
src/stitcher/image_loader.rs
Outdated
| let mut images: Vec<_> = read_dir(directory_path)? | ||
| pub fn find_images(directory_path: impl AsRef<Path>) -> Result<Vec<PathBuf>, ImageLoaderError> { | ||
| // create pathbuf, check if path is a directory | ||
| let path = PathBuf::from(directory_path.as_ref()); |
There was a problem hiding this comment.
Check if a path is a directory from the metadata before operating on it. IO errors kinda suck in Rust's standard library, so it's easier to handle non-directory paths like this than attempt to use io::Error (additional ErrorKinds are a work-in-progress).
src/stitcher/image_loader.rs
Outdated
|
|
||
| // get images | ||
| let mut images: Vec<_> = read_dir(directory_path) | ||
| .map_err(|e| ImageLoaderError::from_io_error(e, path))? |
There was a problem hiding this comment.
All the map_err usage is required because ImageLoaderError holds additional context (the path in question) that io::Error doesn't provide, so a From<io::Error> implementation isn't possible.
| }); | ||
|
|
||
| // sort images by natural order | ||
| images.sort_by(|a, b| natord::compare(&a.display().to_string(), &b.display().to_string())); |
There was a problem hiding this comment.
Favor .display().to_string() over .file_name().unwrap().to_str().unwrap(). If there are non-textual bytes in the file name, then we should just try to gloss over it as best as possible instead of crashing the program.
src/stitcher/image_loader.rs
Outdated
| let dimensions = find_images(directory_path)? | ||
| .into_iter() | ||
| .map(|image| image_dimensions(image).map_err(|e| ImageLoaderError::from(e))) | ||
| pub fn load_images(paths: &[impl AsRef<Path>], width: Option<u32>) -> Result<RgbImage, ImageLoaderError> { |
There was a problem hiding this comment.
In my opinion, load_images should just load a given set of images, and find_images should be used instead to find the images to load.
| @@ -17,16 +17,16 @@ use thiserror::Error; | |||
| /// Errors related to loading images. | |||
| pub enum ImageLoaderError { | |||
There was a problem hiding this comment.
Removed usage of PathBuf here because I figured it's usually unnecessary to also provide the path. If necessary in the future, we can add an Option<PathBuf> instead.
Removing PathBuf in the error also allows for a From<io::Error> implementation.
| match err.kind() { | ||
| Kind::NotFound => ImageLoaderError::NotFound(path), | ||
| Kind::PermissionDenied => ImageLoaderError::PermissionDenied(path), | ||
| impl From<io::Error> for ImageLoaderError { |
There was a problem hiding this comment.
Added a From<io::Error> implementation for simpler error handling, replacing the prior from_io_error function. You can just use ? to bubble up IO errors now.
|
Also note that I ran |
y-mx-b
force-pushed
the
improve-error-handling
branch
from
dd0ddc5 to
edff0a2
Compare
|
Removed dev container config from git history. |
Remove usage of anyhow in library code, improve ImageLoaderError instead.
Cleaned up some of the image loading code and made the API a little more flexible.
Removed usages of
find_imageswithinload_images. Ideally, one should callfind_imagesbefore usingload_imagesonce instead, and then load all the resulting images.