Error cleanup: standard pattern for I/O errors and enum naming

components/cldr-json-data-provider/src/download/cldr_paths_download.rs

@@ -1,6 +1,5 @@
-use super::error::DownloadError;
+use super::error::Error;
 use super::io_util;
-use crate::error::Error;
 use crate::CldrPaths;
 use std::path::PathBuf;
 
@@ -53,10 +52,10 @@ pub struct CldrPathsDownload {
 
 // TODO(#297): Implement this async.
 impl CldrPaths for CldrPathsDownload {
-    fn cldr_core(&self) -> Result<PathBuf, Error> {
+    fn cldr_core(&self) -> Result<PathBuf, crate::error::Error> {
         self.cldr_core.download_and_unzip(&self)
     }
-    fn cldr_dates(&self) -> Result<PathBuf, Error> {
+    fn cldr_dates(&self) -> Result<PathBuf, crate::error::Error> {
         self.cldr_dates.download_and_unzip(&self)
     }
 }
@@ -67,10 +66,10 @@ impl CldrPathsDownload {
     ///
     /// github_tag should be a tag in the CLDR JSON repositories, such as "36.0.0":
     /// https://github.com/unicode-cldr/cldr-core/tags
-    pub fn try_from_github_tag(github_tag: &str) -> Result<Self, DownloadError> {
+    pub fn try_from_github_tag(github_tag: &str) -> Result<Self, Error> {
         Ok(Self {
             cache_dir: dirs::cache_dir()
-                .ok_or(DownloadError::NoCacheDir)?
+                .ok_or(Error::NoCacheDir)?
                 .join("icu4x")
                 .join("cldr"),
             cldr_core: CldrZipFileInfo {
@@ -100,7 +99,10 @@ pub struct CldrZipFileInfo {
 }
 
 impl CldrZipFileInfo {
-    fn download_and_unzip(&self, parent: &CldrPathsDownload) -> Result<PathBuf, Error> {
+    fn download_and_unzip(
+        &self,
+        parent: &CldrPathsDownload,
+    ) -> Result<PathBuf, crate::error::Error> {
         io_util::download_and_unzip(&self.url, &parent.cache_dir)
             .map(|p| p.join(&self.top_dir))
             .map_err(|e| e.into())

components/cldr-json-data-provider/src/download/error.rs

@@ -1,41 +1,43 @@
 use std::error;
 use std::fmt;
 use std::io;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 
 #[derive(Debug)]
-pub enum DownloadError {
-    Io(io::Error, PathBuf),
+pub enum Error {
+    Io(io::Error, Option<PathBuf>),
     Reqwest(reqwest::Error),
     HttpStatus(reqwest::StatusCode, String),
     NoCacheDir,
 }
 
-impl From<io::Error> for DownloadError {
-    /// Note: Prefer adding the path to Error::Io instead of using this conversion.
-    fn from(err: io::Error) -> Self {
-        Self::Io(err, PathBuf::new())
+/// To help with debugging, I/O errors should be paired with a file path.
+/// If a path is unavailable, create the error directly: Error::Io(err, None)
+impl<P: AsRef<Path>> From<(std::io::Error, P)> for Error {
+    fn from(pieces: (std::io::Error, P)) -> Self {
+        Self::Io(pieces.0, Some(pieces.1.as_ref().to_path_buf()))
     }
 }
 
-impl From<reqwest::Error> for DownloadError {
+impl From<reqwest::Error> for Error {
     fn from(err: reqwest::Error) -> Self {
         Self::Reqwest(err)
     }
 }
 
-impl fmt::Display for DownloadError {
+impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            Self::Io(err, path) => write!(f, "{}: {}", err, path.to_string_lossy()),
+            Self::Io(err, Some(path)) => write!(f, "{}: {:?}", err, path),
+            Self::Io(err, None) => err.fmt(f),
             Self::Reqwest(err) => err.fmt(f),
             Self::HttpStatus(status, url) => write!(f, "HTTP request failed: {}: {}", status, url),
             Self::NoCacheDir => write!(f, "dirs::cache_dir() returned None"),
         }
     }
 }
 
-impl error::Error for DownloadError {
+impl error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {
             Self::Io(err, _) => Some(err),

components/cldr-json-data-provider/src/download/io_util.rs

@@ -1,15 +1,9 @@
-use super::error::DownloadError;
+use super::error::Error;
 use std::fs::{self, File};
 use std::path::{Path, PathBuf};
 use std::time::Instant;
 use unzip::Unzipper;
 
-macro_rules! map_io_err {
-    ($path_ref:ident) => {
-        |err| DownloadError::Io(err, $path_ref.to_owned())
-    };
-}
-
 #[cfg(test)]
 fn assert_files_eq(expected_file_path: &Path, actual_file_path: &Path) {
     use std::io::Read;
@@ -28,26 +22,23 @@ fn assert_files_eq(expected_file_path: &Path, actual_file_path: &Path) {
 
 // Synchronously download url and save it to destination.
 // TODO(#297): Implement this async.
-fn download_sync(url: &str, destination: &Path) -> Result<(), DownloadError> {
+fn download_sync(url: &str, destination: &Path) -> Result<(), Error> {
     log::info!("Downloading: {}", url);
     let start = Instant::now();
     let mut response = reqwest::blocking::get(url)?;
     if !response.status().is_success() {
-        return Err(DownloadError::HttpStatus(
-            response.status(),
-            url.to_string(),
-        ));
+        return Err(Error::HttpStatus(response.status(), url.to_string()));
     }
     log::info!("Status: {}", response.status());
-    let mut file = File::create(destination).map_err(map_io_err!(destination))?;
+    let mut file = File::create(destination).map_err(|e| (e, destination))?;
     response.copy_to(&mut file)?;
     log::info!("Finished in {:.2} seconds", start.elapsed().as_secs_f64());
     Ok(())
 }
 
 #[test]
-fn test_download_sync() -> Result<(), DownloadError> {
-    let temp_file = mktemp::Temp::new_file()?;
+fn test_download_sync() -> Result<(), Error> {
+    let temp_file = mktemp::Temp::new_file().map_err(|e| Error::Io(e, None))?;
     download_sync(
         "https://www.w3.org/WAI/ER/tests/xhtml/testfiles/resources/pdf/dummy.pdf",
         &temp_file,
@@ -58,20 +49,20 @@ fn test_download_sync() -> Result<(), DownloadError> {
 
 /// Synchronously unpack a zip file into a destination directory.
 // TODO(#297): Implement this async.
-fn unzip_sync(zip_path: &Path, dir_path: &Path) -> Result<(), DownloadError> {
-    let reader = File::open(zip_path).map_err(map_io_err!(zip_path))?;
+fn unzip_sync(zip_path: &Path, dir_path: &Path) -> Result<(), Error> {
+    let reader = File::open(zip_path).map_err(|e| (e, zip_path))?;
     log::info!("Unzipping...");
     let start = Instant::now();
     Unzipper::new(reader, dir_path)
         .unzip()
-        .map_err(map_io_err!(dir_path))?;
+        .map_err(|e| (e, dir_path))?;
     log::info!("Unzipped in {:.2} seconds", start.elapsed().as_secs_f64());
     Ok(())
 }
 
 #[test]
-fn test_unzip_sync() -> Result<(), DownloadError> {
-    let temp_dir = mktemp::Temp::new_dir()?;
+fn test_unzip_sync() -> Result<(), Error> {
+    let temp_dir = mktemp::Temp::new_dir().map_err(|e| Error::Io(e, None))?;
     unzip_sync(&PathBuf::from("./tests/testdata/dummy.zip"), &temp_dir)?;
     assert_files_eq(
         &PathBuf::from("./tests/testdata/dummy.pdf"),
@@ -84,14 +75,14 @@ fn test_unzip_sync() -> Result<(), DownloadError> {
 ///
 /// `cache_dir` is a directory where both the zip file and the unpacked directory will be
 /// saved. If the zip file has already been downloaded, it will not be downloaded again.
-pub fn download_and_unzip(zip_file_url: &str, cache_dir: &Path) -> Result<PathBuf, DownloadError> {
-    fs::create_dir_all(cache_dir).map_err(map_io_err!(cache_dir))?;
+pub fn download_and_unzip(zip_file_url: &str, cache_dir: &Path) -> Result<PathBuf, Error> {
+    fs::create_dir_all(cache_dir).map_err(|e| (e, cache_dir))?;
 
     let zip_dir = cache_dir.to_path_buf().join("zips");
-    fs::create_dir_all(&zip_dir).map_err(map_io_err!(zip_dir))?;
+    fs::create_dir_all(&zip_dir).map_err(|e| (e, &zip_dir))?;
 
     let data_dir = cache_dir.to_path_buf().join("data");
-    fs::create_dir_all(&data_dir).map_err(map_io_err!(data_dir))?;
+    fs::create_dir_all(&data_dir).map_err(|e| (e, &data_dir))?;
 
     let basename = urlencoding::encode(zip_file_url);
     let mut zip_path = zip_dir.join(&basename);

components/cldr-json-data-provider/src/download/mod.rs

@@ -3,4 +3,4 @@ mod error;
 mod io_util;
 
 pub use cldr_paths_download::CldrPathsDownload;
-pub use error::DownloadError;
+pub use error::Error;

components/cldr-json-data-provider/src/error.rs

@@ -1,18 +1,18 @@
 use std::error;
 use std::fmt;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 
 #[cfg(feature = "download")]
-use crate::download::DownloadError;
+use crate::download;
 
 #[non_exhaustive]
 #[derive(Debug)]
 pub enum Error {
-    JsonError(serde_json::error::Error, Option<PathBuf>),
-    IoError(std::io::Error, std::path::PathBuf),
+    Io(std::io::Error, Option<PathBuf>),
+    Json(serde_json::error::Error, Option<PathBuf>),
     MissingSource(MissingSourceError),
     #[cfg(feature = "download")]
-    Download(DownloadError),
+    Download(download::Error),
     PoisonError,
 }
 
@@ -27,9 +27,19 @@ impl fmt::Display for MissingSourceError {
     }
 }
 
-impl From<(serde_json::error::Error, Option<PathBuf>)> for Error {
-    fn from(pieces: (serde_json::error::Error, Option<PathBuf>)) -> Self {
-        Self::JsonError(pieces.0, pieces.1)
+/// To help with debugging, I/O errors should be paired with a file path.
+/// If a path is unavailable, create the error directly: Error::Io(err, None)
+impl<P: AsRef<Path>> From<(std::io::Error, P)> for Error {
+    fn from(pieces: (std::io::Error, P)) -> Self {
+        Self::Io(pieces.0, Some(pieces.1.as_ref().to_path_buf()))
+    }
+}
+
+/// To help with debugging, JSON errors should be paired with a file path.
+/// If a path is unavailable, create the error directly: Error::Json(err, None)
+impl<P: AsRef<Path>> From<(serde_json::error::Error, P)> for Error {
+    fn from(pieces: (serde_json::error::Error, P)) -> Self {
+        Self::Json(pieces.0, Some(pieces.1.as_ref().to_path_buf()))
     }
 }
 
@@ -40,10 +50,10 @@ impl From<MissingSourceError> for Error {
 }
 
 #[cfg(feature = "download")]
-impl From<DownloadError> for Error {
-    fn from(err: DownloadError) -> Error {
+impl From<download::Error> for Error {
+    fn from(err: download::Error) -> Error {
         match err {
-            DownloadError::Io(err, path) => Error::IoError(err, path),
+            download::Error::Io(err, path) => Error::Io(err, path),
             _ => Error::Download(err),
         }
     }
@@ -52,11 +62,12 @@ impl From<DownloadError> for Error {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            Error::JsonError(err, Some(filename)) => {
+            Error::Io(err, Some(path)) => write!(f, "{}: {:?}", err, path),
+            Error::Io(err, None) => err.fmt(f),
+            Error::Json(err, Some(filename)) => {
                 write!(f, "JSON parse error: {}: {:?}", err, filename)
             }
-            Error::JsonError(err, None) => write!(f, "JSON parse error: {}", err),
-            Error::IoError(err, path) => write!(f, "{}: {}", err, path.to_string_lossy()),
+            Error::Json(err, None) => write!(f, "JSON parse error: {}", err),
             Error::MissingSource(err) => err.fmt(f),
             #[cfg(feature = "download")]
             Error::Download(err) => err.fmt(f),
@@ -68,8 +79,8 @@ impl fmt::Display for Error {
 impl error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {
-            Error::JsonError(err, _) => Some(err),
-            Error::IoError(err, _) => Some(err),
+            Error::Io(err, _) => Some(err),
+            Error::Json(err, _) => Some(err),
             #[cfg(feature = "download")]
             Error::Download(err) => Some(err),
             _ => None,

components/cldr-json-data-provider/src/reader.rs

@@ -10,14 +10,14 @@ pub fn open_reader(path: &Path) -> Result<BufReader<File>, Error> {
     log::trace!("Reading: {:?}", path);
     File::open(&path)
         .map(BufReader::new)
-        .map_err(|e| Error::IoError(e, path.to_path_buf()))
+        .map_err(|e| (e, path).into())
 }
 
 /// Helper function which returns a sorted list of subdirectories.
 pub fn get_subdirectories(root: &Path) -> Result<Vec<PathBuf>, Error> {
     let mut result = vec![];
-    for entry in fs::read_dir(root).map_err(|e| Error::IoError(e, root.to_path_buf()))? {
-        let entry = entry.map_err(|e| Error::IoError(e, root.to_path_buf()))?;
+    for entry in fs::read_dir(root).map_err(|e| (e, root))? {
+        let entry = entry.map_err(|e| (e, root))?;
         let path = entry.path();
         result.push(path);
     }

components/cldr-json-data-provider/src/transform/dates.rs

@@ -29,7 +29,7 @@ impl TryFrom<&dyn CldrPaths> for DatesProvider<'_> {
             let path = dir.join("ca-gregorian.json");
 
             let mut resource: cldr_json::Resource =
-                serde_json::from_reader(open_reader(&path)?).map_err(|e| (e, Some(path)))?;
+                serde_json::from_reader(open_reader(&path)?).map_err(|e| (e, path))?;
             data.append(&mut resource.main.0);
         }
 
@@ -46,7 +46,7 @@ impl TryFrom<&str> for DatesProvider<'_> {
         let mut data = vec![];
 
         let mut resource: cldr_json::Resource =
-            serde_json::from_str(input).map_err(|e| (e, None))?;
+            serde_json::from_str(input).map_err(|e| Error::Json(e, None))?;
         data.append(&mut resource.main.0);
 
         Ok(Self {

components/cldr-json-data-provider/src/transform/plurals.rs

@@ -27,7 +27,7 @@ impl TryFrom<&dyn CldrPaths> for PluralsProvider<'_> {
                 .join("supplemental")
                 .join("plurals.json");
             let data: cldr_json::Resource =
-                serde_json::from_reader(open_reader(&path)?).map_err(|e| (e, Some(path)))?;
+                serde_json::from_reader(open_reader(&path)?).map_err(|e| (e, path))?;
             data.supplemental.plurals_type_cardinal
         };
         let ordinal_rules = {
@@ -36,7 +36,7 @@ impl TryFrom<&dyn CldrPaths> for PluralsProvider<'_> {
                 .join("supplemental")
                 .join("ordinals.json");
             let data: cldr_json::Resource =
-                serde_json::from_reader(open_reader(&path)?).map_err(|e| (e, Some(path)))?;
+                serde_json::from_reader(open_reader(&path)?).map_err(|e| (e, path))?;
             data.supplemental.plurals_type_ordinal
         };
         Ok(PluralsProvider {

components/fs-data-provider/src/bin/icu4x-cldr-export.rs

@@ -55,8 +55,8 @@ impl From<icu_data_provider::DataError> for Error {
     }
 }
 
-impl From<icu_cldr_json_data_provider::download::DownloadError> for Error {
-    fn from(err: icu_cldr_json_data_provider::download::DownloadError) -> Error {
+impl From<icu_cldr_json_data_provider::download::Error> for Error {
+    fn from(err: icu_cldr_json_data_provider::download::Error) -> Error {
         Error::Setup(Box::from(err))
     }
 }