Address review comments

Author: cmyr

src/datastore.rs

@@ -8,7 +8,7 @@ use std::{
 };
 
 use crate::error::{StoreEntryError, StoreError};
-use crate::font_source::FontSource;
+use crate::font_source::{DirEntry, FontSource};
 
 /// A generic file store for UFO [data][spec_data] and [images][spec_images],
 /// mapping [`PathBuf`] keys to [`Vec<u8>`] values.
@@ -141,13 +141,14 @@ impl DataType for Data {
                 .list_dir(&dir_path)
                 .map_err(|e| StoreEntryError::new(dir_path.clone(), e.into()))?;
 
-            for (entry_name, is_dir) in entries {
-                let full_rel = dir_path.join(&entry_name);
-                if is_dir {
-                    dir_queue.push(full_rel);
-                } else {
-                    let key = full_rel.strip_prefix(source_root).unwrap().to_path_buf();
-                    paths.push(key);
+            for entry in entries {
+                match entry {
+                    DirEntry::Dir(name) => dir_queue.push(dir_path.join(name)),
+                    DirEntry::File(name) => {
+                        let full_rel = dir_path.join(name);
+                        let key = full_rel.strip_prefix(source_root).unwrap().to_path_buf();
+                        paths.push(key);
+                    }
                 }
             }
         }
@@ -190,14 +191,13 @@ impl DataType for Image {
             .list_dir(source_root)
             .map_err(|e| StoreEntryError::new(source_root.to_path_buf(), e.into()))?;
 
-        for (entry_name, is_dir) in entries {
-            let full_rel = source_root.join(&entry_name);
-            if is_dir {
+        for entry in entries {
+            match entry {
                 // The spec forbids directories.
-                return Err(StoreEntryError::new(full_rel, StoreError::Subdir));
-            } else {
-                let key = full_rel.strip_prefix(source_root).unwrap().to_path_buf();
-                paths.push(key);
+                DirEntry::Dir(name) => {
+                    return Err(StoreEntryError::new(name, StoreError::Subdir))
+                }
+                DirEntry::File(name) => paths.push(name),
             }
         }
 
@@ -806,21 +806,18 @@ mod tests {
 
     /// A FontSource backed by in-memory data, with `as_path() -> None`.
     /// This triggers the eager-loading branch in `Store::new`.
+    #[derive(Default)]
     struct MemorySource {
         files: HashMap<PathBuf, Vec<u8>>,
-        dirs: HashMap<PathBuf, Vec<(PathBuf, bool)>>,
+        dirs: HashMap<PathBuf, Vec<DirEntry>>,
     }
 
     impl MemorySource {
-        fn new() -> Self {
-            MemorySource { files: HashMap::new(), dirs: HashMap::new() }
-        }
-
         fn add_file(&mut self, path: impl Into<PathBuf>, data: Vec<u8>) {
             self.files.insert(path.into(), data);
         }
 
-        fn add_dir(&mut self, path: impl Into<PathBuf>, entries: Vec<(PathBuf, bool)>) {
+        fn add_dir(&mut self, path: impl Into<PathBuf>, entries: Vec<DirEntry>) {
             self.dirs.insert(path.into(), entries);
         }
     }
@@ -830,7 +827,7 @@ mod tests {
             self.files.get(path).cloned().map(Ok)
         }
 
-        fn list_dir(&self, path: &Path) -> Result<Vec<(PathBuf, bool)>, std::io::Error> {
+        fn list_dir(&self, path: &Path) -> Result<Vec<DirEntry>, std::io::Error> {
             self.dirs.get(path).cloned().ok_or_else(|| {
                 std::io::Error::new(std::io::ErrorKind::NotFound, format!("{path:?} not found"))
             })
@@ -841,10 +838,10 @@ mod tests {
 
     #[test]
     fn data_eager_loading_from_memory_source() {
-        let mut source = MemorySource::new();
+        let mut source = MemorySource::default();
         source.add_dir(
             "data",
-            vec![(PathBuf::from("a.txt"), false), (PathBuf::from("b.txt"), false)],
+            vec![DirEntry::File("a.txt".into()), DirEntry::File("b.txt".into())],
         );
         source.add_file("data/a.txt", b"aaa".to_vec());
         source.add_file("data/b.txt", b"bbb".to_vec());
@@ -865,8 +862,8 @@ mod tests {
     #[test]
     fn image_eager_loading_from_memory_source() {
         let img = png_data(b"test");
-        let mut source = MemorySource::new();
-        source.add_dir("images", vec![(PathBuf::from("x.png"), false)]);
+        let mut source = MemorySource::default();
+        source.add_dir("images", vec![DirEntry::File("x.png".into())]);
         source.add_file("images/x.png", img.clone());
 
         let store = ImageStore::new(&source).unwrap();
@@ -879,8 +876,8 @@ mod tests {
 
     #[test]
     fn eager_load_invalid_image_fails_at_construction() {
-        let mut source = MemorySource::new();
-        source.add_dir("images", vec![(PathBuf::from("bad.png"), false)]);
+        let mut source = MemorySource::default();
+        source.add_dir("images", vec![DirEntry::File("bad.png".into())]);
         source.add_file("images/bad.png", b"not a png".to_vec());
 
         // Eager loading validates during construction, so this should fail.
@@ -890,10 +887,9 @@ mod tests {
 
     #[test]
     fn eager_load_nested_data_from_memory_source() {
-        let mut source = MemorySource::new();
-        source
-            .add_dir("data", vec![(PathBuf::from("top.txt"), false), (PathBuf::from("sub"), true)]);
-        source.add_dir("data/sub", vec![(PathBuf::from("deep.txt"), false)]);
+        let mut source = MemorySource::default();
+        source.add_dir("data", vec![DirEntry::File("top.txt".into()), DirEntry::Dir("sub".into())]);
+        source.add_dir("data/sub", vec![DirEntry::File("deep.txt".into())]);
         source.add_file("data/top.txt", b"top".to_vec());
         source.add_file("data/sub/deep.txt", b"deep".to_vec());
 

src/error.rs

@@ -163,7 +163,7 @@ pub enum LayerLoadError {
         source: GlifLoadError,
     },
     /// An [`std::io::Error`].
-    #[error("failed to read layer data")]
+    #[error("failed to read layer data: '{0}'")]
     Io(#[from] IoError),
     /// Could not find the layer's contents.plist.
     #[error("cannot find the contents.plist file")]
@@ -192,7 +192,7 @@ pub enum FontInfoLoadError {
     #[error("fontinfo.plist contains invalid data: {0}")]
     InvalidData(FontInfoErrorKind),
     /// An [`std::io::Error`].
-    #[error("failed to read fontinfo.plist file")]
+    #[error("failed to read fontinfo.plist file: '{0}'")]
     Io(#[from] IoError),
     /// Could not parse the UFO's fontinfo.plist.
     #[error("failed to parse fontinfo.plist file")]
@@ -313,8 +313,11 @@ impl StoreEntryError {
     /// Returns `true` if the store could not be populated because the source
     /// does not support directory listing or the directory does not exist.
     pub(crate) fn is_missing_or_unsupported(&self) -> bool {
-        matches!(&self.source, StoreError::Io(e)
-            if matches!(e.kind(), std::io::ErrorKind::Unsupported | std::io::ErrorKind::NotFound))
+        if let StoreError::Io(e) = &self.source {
+            matches!(e.kind(), std::io::ErrorKind::Unsupported | std::io::ErrorKind::NotFound)
+        } else {
+            false
+        }
     }
 }
 

src/font_source.rs

@@ -12,8 +12,24 @@ use std::path::{Path, PathBuf};
 /// Paths passed to methods are always relative to the UFO root, e.g.
 /// `"metainfo.plist"`, `"glyphs/contents.plist"`, `"glyphs/A_.glif"`.
 ///
-/// A filesystem directory (a `&Path`) implements this trait directly, so you
-/// can pass a path wherever a `FontSource` is expected.
+/// Two implementations are provided out of the box:
+///
+/// - A filesystem directory (a `&Path`) implements this trait directly, so you
+///   can pass a path wherever a `FontSource` is expected.
+/// - Any closure `Fn(&Path) -> Option<Result<Vec<u8>, io::Error>>` implements it
+///   too, which is handy for a quick ad-hoc source without defining a type:
+///
+/// ```
+/// use std::io;
+/// use std::path::Path;
+/// use norad::{DataRequest, Font};
+///
+/// # fn example(lookup: impl Fn(&Path) -> Option<Vec<u8>> + Sync) -> Result<(), Box<dyn std::error::Error>> {
+/// let source = |path: &Path| lookup(path).map(Ok::<_, io::Error>);
+/// let font = Font::load_from_source(&DataRequest::all(), &source)?;
+/// # Ok(())
+/// # }
+/// ```
 ///
 /// # Implementing
 ///
@@ -60,21 +76,30 @@ pub trait FontSource: Sync {
 
     /// List entries in a directory at the given relative path.
     ///
-    /// Returns `(entry_name, is_dir)` pairs, where `entry_name` is the name
-    /// of each entry (not a full path). Callers should join with the directory
-    /// path to get the full relative path.
+    /// Returns a [`DirEntry`] for each entry, carrying the entry name (not a
+    /// full path). Callers should join with the directory path to get the full
+    /// relative path.
     ///
     /// The default implementation returns [`io::ErrorKind::Unsupported`],
     /// meaning this source does not support directory enumeration. Data and
     /// image stores will be empty for such sources.
-    fn list_dir(&self, _path: &Path) -> Result<Vec<(PathBuf, bool)>, io::Error> {
+    fn list_dir(&self, _path: &Path) -> Result<Vec<DirEntry>, io::Error> {
         Err(io::Error::new(
             io::ErrorKind::Unsupported,
             "this FontSource does not support directory listing",
         ))
     }
 }
 
+/// The name of a file or directory, relative to a parent (not a full path!)
+#[derive(Clone, Debug)]
+pub enum DirEntry {
+    /// A file, carrying its name relative to the parent directory.
+    File(PathBuf),
+    /// A subdirectory, carrying its name relative to the parent directory.
+    Dir(PathBuf),
+}
+
 /// A directory on disk implements [`FontSource`] directly.
 impl FontSource for &Path {
     fn try_read(&self, path: &Path) -> Option<Result<Vec<u8>, io::Error>> {
@@ -89,14 +114,18 @@ impl FontSource for &Path {
         Some(self)
     }
 
-    fn list_dir(&self, path: &Path) -> Result<Vec<(PathBuf, bool)>, io::Error> {
+    fn list_dir(&self, path: &Path) -> Result<Vec<DirEntry>, io::Error> {
         let full = self.join(path);
         let mut entries = Vec::new();
         for entry in std::fs::read_dir(&full)? {
             let entry = entry?;
             let metadata = entry.metadata()?;
             let name = PathBuf::from(entry.file_name());
-            entries.push((name, metadata.is_dir()));
+            entries.push(if metadata.is_dir() {
+                DirEntry::Dir(name)
+            } else {
+                DirEntry::File(name)
+            });
         }
         Ok(entries)
     }

src/fontinfo.rs

@@ -2,7 +2,6 @@
 //!
 //! [`fontinfo.plist`]: https://unifiedfontobject.org/versions/ufo3/fontinfo.plist/
 
-use std::path::Path;
 use std::{collections::HashSet, convert::TryFrom, ops::Deref};
 
 use serde::de::Deserializer;
@@ -488,24 +487,6 @@ struct FontInfoV1 {
 }
 
 impl FontInfo {
-    /// Returns [`FontInfo`] from a file, upgrading from the supplied `format_version` to the highest
-    /// internally supported version.
-    ///
-    /// The conversion follows what ufoLib and defcon are doing, e.g. various fields that were
-    /// implicitly signed integers before and are unsigned integers in the newest spec, are
-    /// converted by taking their absolute value. Fields that could be floats before and are
-    /// integers now are rounded. Fields that could be floats before and are unsigned integers
-    /// now are rounded before taking their absolute value.
-    #[allow(dead_code)]
-    pub(crate) fn from_file<P: AsRef<Path>>(
-        path: P,
-        format_version: FormatVersion,
-        lib: &mut Plist,
-    ) -> Result<Self, FontInfoLoadError> {
-        let data = std::fs::read(path.as_ref()).map_err(FontInfoLoadError::Io)?;
-        Self::from_bytes(&data, format_version, lib)
-    }
-
     pub(crate) fn from_bytes(
         data: &[u8],
         format_version: FormatVersion,

src/layer.rs

@@ -344,7 +344,7 @@ impl Layer {
                 let full_path = layer_dir.join(glyph_path);
 
                 source
-                    .read(&full_path)
+                    .read(&full_path)?
                     .map_err(GlifLoadError::Io)
                     .and_then(|data| Glyph::parse(&data))
                     .map_err(|source| LayerLoadError::Glyph {

src/lib.rs

@@ -87,7 +87,7 @@ mod write;
 
 pub use data_request::DataRequest;
 pub use font::{Font, FormatVersion, MetaInfo};
-pub use font_source::FontSource;
+pub use font_source::{DirEntry, FontSource};
 pub use fontinfo::FontInfo;
 pub use glyph::{
     AffineTransform, Anchor, Codepoints, Component, Contour, ContourPoint, Glyph, Image, PointType,