Address review comments

Author: cmyr

src/datastore.rs

@@ -191,14 +191,11 @@ impl DataType for Image {
             .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 {
                 // 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);
+                return Err(StoreEntryError::new(entry_name, StoreError::Subdir));
             }
+            paths.push(entry_name);
         }
 
         Ok(paths)
@@ -806,16 +803,13 @@ 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)>>,
     }
 
     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);
         }
@@ -841,7 +835,7 @@ 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)],
@@ -865,7 +859,7 @@ mod tests {
     #[test]
     fn image_eager_loading_from_memory_source() {
         let img = png_data(b"test");
-        let mut source = MemorySource::new();
+        let mut source = MemorySource::default();
         source.add_dir("images", vec![(PathBuf::from("x.png"), false)]);
         source.add_file("images/x.png", img.clone());
 
@@ -879,7 +873,7 @@ mod tests {
 
     #[test]
     fn eager_load_invalid_image_fails_at_construction() {
-        let mut source = MemorySource::new();
+        let mut source = MemorySource::default();
         source.add_dir("images", vec![(PathBuf::from("bad.png"), false)]);
         source.add_file("images/bad.png", b"not a png".to_vec());
 
@@ -890,7 +884,7 @@ mod tests {
 
     #[test]
     fn eager_load_nested_data_from_memory_source() {
-        let mut source = MemorySource::new();
+        let mut source = MemorySource::default();
         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)]);

src/error.rs

@@ -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
 ///

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,