Return a result for ResourceManager::get_resource (#282)

adriantombu · web-flow · commit f2e43593dff8 · 2022-12-02T10:08:55.000-06:00
diff --git a/fluent-resmgr/Cargo.toml b/fluent-resmgr/Cargo.toml
@@ -23,6 +23,7 @@ unic-langid = "0.9"
 elsa = "1.5"
 futures = "0.3"
 rustc-hash = "1"
+thiserror = "1.0"
 
 [dev-dependencies]
 unic-langid = { version = "0.9", features = ["macros"]}
diff --git a/fluent-resmgr/examples/simple-resmgr.rs b/fluent-resmgr/examples/simple-resmgr.rs
@@ -40,15 +40,13 @@ fn get_available_locales() -> Result<Vec<LanguageIdentifier>, io::Error> {
         .join("examples")
         .join("resources");
     let res_dir = fs::read_dir(res_path)?;
-    for entry in res_dir {
-        if let Ok(entry) = entry {
-            let path = entry.path();
-            if path.is_dir() {
-                if let Some(name) = path.file_name() {
-                    if let Some(name) = name.to_str() {
-                        let langid: LanguageIdentifier = name.parse().expect("Parsing failed.");
-                        locales.push(langid);
-                    }
+    for entry in res_dir.flatten() {
+        let path = entry.path();
+        if path.is_dir() {
+            if let Some(name) = path.file_name() {
+                if let Some(name) = name.to_str() {
+                    let langid: LanguageIdentifier = name.parse().expect("Parsing failed.");
+                    locales.push(langid);
                 }
             }
         }
@@ -90,10 +88,12 @@ fn main() {
     );
 
     // 5. Get a bundle for given paths and locales.
-    let bundle = mgr.get_bundle(
-        resolved_locales.into_iter().map(|s| s.to_owned()).collect(),
-        resources,
-    );
+    let bundle = mgr
+        .get_bundle(
+            resolved_locales.into_iter().map(|s| s.to_owned()).collect(),
+            resources,
+        )
+        .expect("Could not get bundle");
 
     // 6. Check if the input is provided.
     match args.get(1) {
diff --git a/fluent-resmgr/src/resource_manager.rs b/fluent-resmgr/src/resource_manager.rs
@@ -6,9 +6,9 @@ use fluent_fallback::{
 };
 use futures::stream::Stream;
 use rustc_hash::FxHashSet;
-use std::fs;
 use std::io;
-use std::iter;
+use std::{fs, iter};
+use thiserror::Error;
 use unic_langid::LanguageIdentifier;
 
 fn read_file(path: &str) -> Result<String, io::Error> {
@@ -48,21 +48,24 @@ impl ResourceManager {
 
     /// Returns a [`FluentResource`], by either reading the file and loading it into
     /// memory, or retrieving it from an in-memory cache.
-    fn get_resource(&self, resource_id: &str, locale: &str) -> &FluentResource {
+    fn get_resource(
+        &self,
+        res_id: &str,
+        locale: &str,
+    ) -> Result<&FluentResource, ResourceManagerError> {
         let path = self
             .path_scheme
             .replace("{locale}", locale)
-            .replace("{res_id}", resource_id);
-        if let Some(res) = self.resources.get(&path) {
-            res
+            .replace("{res_id}", res_id);
+        Ok(if let Some(resource) = self.resources.get(&path) {
+            resource
         } else {
-            let string = read_file(&path).unwrap();
-            let res = match FluentResource::try_new(string) {
-                Ok(res) => res,
-                Err((res, _err)) => res,
+            let resource = match FluentResource::try_new(read_file(&path)?) {
+                Ok(resource) => resource,
+                Err((resource, _err)) => resource,
             };
-            self.resources.insert(path.to_string(), Box::new(res))
-        }
+            self.resources.insert(path.to_string(), Box::new(resource))
+        })
     }
 
     /// Gets a [`FluentBundle`] from a list of resources. The bundle will only contain the
@@ -74,14 +77,28 @@ impl ResourceManager {
         &self,
         locales: Vec<LanguageIdentifier>,
         resource_ids: Vec<String>,
-    ) -> FluentBundle<&FluentResource> {
+    ) -> Result<FluentBundle<&FluentResource>, Vec<ResourceManagerError>> {
+        let mut errors: Vec<ResourceManagerError> = vec![];
         let mut bundle = FluentBundle::new(locales.clone());
-        for res_id in &resource_ids {
-            println!("res_id {:?}", res_id);
-            let res = self.get_resource(res_id, &locales[0].to_string());
-            bundle.add_resource(res).unwrap();
+        let locale = &locales[0];
+
+        for resource_id in &resource_ids {
+            match self.get_resource(resource_id, &locale.to_string()) {
+                Ok(resource) => {
+                    if let Err(errs) = bundle.add_resource(resource) {
+                        errs.into_iter()
+                            .for_each(|error| errors.push(ResourceManagerError::Fluent(error)))
+                    }
+                }
+                Err(error) => errors.push(error),
+            };
+        }
+
+        if !errors.is_empty() {
+            Err(errors)
+        } else {
+            Ok(bundle)
         }
-        bundle
     }
 
     /// Returns an iterator for a [`FluentBundle`] for each locale provided. Each
@@ -92,24 +109,51 @@ impl ResourceManager {
         &self,
         locales: Vec<LanguageIdentifier>,
         resource_ids: Vec<String>,
-    ) -> impl Iterator<Item = FluentBundle<&FluentResource>> {
-        let res_mgr = self;
+    ) -> impl Iterator<Item = Result<FluentBundle<&FluentResource>, Vec<ResourceManagerError>>>
+    {
         let mut idx = 0;
 
         iter::from_fn(move || {
             locales.get(idx).map(|locale| {
                 idx += 1;
+                let mut errors: Vec<ResourceManagerError> = vec![];
                 let mut bundle = FluentBundle::new(vec![locale.clone()]);
+
                 for resource_id in &resource_ids {
-                    let resource = res_mgr.get_resource(resource_id, &locale.to_string());
-                    bundle.add_resource(resource).unwrap();
+                    match self.get_resource(resource_id, &locale.to_string()) {
+                        Ok(resource) => {
+                            if let Err(errs) = bundle.add_resource(resource) {
+                                errs.into_iter().for_each(|error| {
+                                    errors.push(ResourceManagerError::Fluent(error))
+                                })
+                            }
+                        }
+                        Err(error) => errors.push(error),
+                    }
+                }
+
+                if !errors.is_empty() {
+                    Err(errors)
+                } else {
+                    Ok(bundle)
                 }
-                bundle
             })
         })
     }
 }
 
+/// Errors generated during the process of retrieving the localization resources
+#[derive(Error, Debug)]
+pub enum ResourceManagerError {
+    /// Error while reading the resource file
+    #[error("{0}")]
+    Io(#[from] std::io::Error),
+
+    /// Error while trying to add a resource to the bundle
+    #[error("{0}")]
+    Fluent(#[from] fluent_bundle::FluentError),
+}
+
 // Due to limitation of trait, we need a nameable Iterator type.  Due to the
 // lack of GATs, these have to own members instead of taking slices.
 pub struct BundleIter {
@@ -184,35 +228,37 @@ mod test {
         let res_mgr = ResourceManager::new("./tests/resources/{locale}/{res_id}".into());
 
         let _bundle = res_mgr.get_bundle(vec![langid!("en-US")], vec!["test.ftl".into()]);
-        let res_1 = res_mgr.get_resource("test.ftl", "en-US");
+        let res_1 = res_mgr
+            .get_resource("test.ftl", "en-US")
+            .expect("Could not get resource");
 
         let _bundle2 = res_mgr.get_bundle(vec![langid!("en-US")], vec!["test.ftl".into()]);
-        let res_2 = res_mgr.get_resource("test.ftl", "en-US");
+        let res_2 = res_mgr
+            .get_resource("test.ftl", "en-US")
+            .expect("Could not get resource");
 
         assert!(
             std::ptr::eq(res_1, res_2),
             "The resources are cached in memory and reference the same thing."
         );
     }
 
-    // TODO - This API should return a Result instead.
-    // https://github.com/projectfluent/fluent-rs/issues/278
     #[test]
-    #[should_panic]
     fn get_resource_error() {
         let res_mgr = ResourceManager::new("./tests/resources/{locale}/{res_id}".into());
 
         let _bundle = res_mgr.get_bundle(vec![langid!("en-US")], vec!["test.ftl".into()]);
-        res_mgr.get_resource("nonexistent.ftl", "en-US");
+        let res = res_mgr.get_resource("nonexistent.ftl", "en-US");
+
+        assert!(res.is_err());
     }
 
-    // TODO - This API should return a Result instead.
-    // https://github.com/projectfluent/fluent-rs/issues/278
     #[test]
-    #[should_panic]
     fn get_bundle_error() {
         let res_mgr = ResourceManager::new("./tests/resources/{locale}/{res_id}".into());
-        let _bundle = res_mgr.get_bundle(vec![langid!("en-US")], vec!["nonexistent.ftl".into()]);
+        let bundle = res_mgr.get_bundle(vec![langid!("en-US")], vec!["nonexistent.ftl".into()]);
+
+        assert!(bundle.is_err());
     }
 
     // TODO - Syntax errors should be surfaced. This test has an invalid resource that
@@ -221,22 +267,24 @@ mod test {
     #[test]
     fn get_bundle_ignores_errors() {
         let res_mgr = ResourceManager::new("./tests/resources/{locale}/{res_id}".into());
-        let bundle = res_mgr.get_bundle(
-            vec![langid!("en-US")],
-            vec!["test.ftl".into(), "invalid.ftl".into()],
-        );
+        let bundle = res_mgr
+            .get_bundle(
+                vec![langid!("en-US")],
+                vec!["test.ftl".into(), "invalid.ftl".into()],
+            )
+            .expect("Could not retrieve bundle");
 
         let mut errors = vec![];
         let msg = bundle.get_message("hello-world").expect("Message exists");
         let pattern = msg.value().expect("Message has a value");
-        let value = bundle.format_pattern(&pattern, None, &mut errors);
+        let value = bundle.format_pattern(pattern, None, &mut errors);
         assert_eq!(value, "Hello World");
         assert!(errors.is_empty());
 
         let mut errors = vec![];
         let msg = bundle.get_message("valid-message").expect("Message exists");
         let pattern = msg.value().expect("Message has a value");
-        let value = bundle.format_pattern(&pattern, None, &mut errors);
+        let value = bundle.format_pattern(pattern, None, &mut errors);
         assert_eq!(value, "This is a valid message");
         assert!(errors.is_empty());
     }
diff --git a/fluent-resmgr/tests/localization_test.rs b/fluent-resmgr/tests/localization_test.rs
@@ -36,7 +36,9 @@ fn localization_format_value() {
 fn resmgr_get_bundle() {
     let res_mgr = ResourceManager::new("./tests/resources/{locale}/{res_id}".into());
 
-    let bundle = res_mgr.get_bundle(vec![langid!("en-US")], vec!["test.ftl".into()]);
+    let bundle = res_mgr
+        .get_bundle(vec![langid!("en-US")], vec!["test.ftl".into()])
+        .expect("Could not get bundle");
 
     let mut errors = vec![];
     let msg = bundle.get_message("hello-world").expect("Message exists");
@@ -50,25 +52,31 @@ fn resmgr_get_bundles() {
     let res_mgr = ResourceManager::new("./tests/resources/{locale}/{res_id}".into());
 
     let locales = vec![langid!("en-US"), langid!("pl")];
-    let mut bundles_iter = res_mgr.get_bundles(locales.clone(), vec!["test.ftl".into()]);
+    let mut bundles_iter = res_mgr.get_bundles(locales, vec!["test.ftl".into()]);
 
     {
-        let bundle = bundles_iter.next().expect("Failed to get en-US bundle.");
+        let bundle = bundles_iter
+            .next()
+            .unwrap()
+            .expect("Failed to get en-US bundle.");
 
         let mut errors = vec![];
         let msg = bundle.get_message("hello-world").expect("Message exists");
         let pattern = msg.value().expect("Message has a value");
-        let value = bundle.format_pattern(&pattern, None, &mut errors);
+        let value = bundle.format_pattern(pattern, None, &mut errors);
         assert_eq!(value, "Hello World");
     }
 
     {
-        let bundle = bundles_iter.next().expect("Failed to get pl bundle.");
+        let bundle = bundles_iter
+            .next()
+            .unwrap()
+            .expect("Failed to get pl bundle.");
 
         let mut errors = vec![];
         let msg = bundle.get_message("hello-world").expect("Witaj Świecie");
         let pattern = msg.value().expect("Message has a value");
-        let value = bundle.format_pattern(&pattern, None, &mut errors);
+        let value = bundle.format_pattern(pattern, None, &mut errors);
         assert_eq!(value, "Witaj Świecie");
     }
 
