Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a result for ResourceManager::get_resource #282

Merged
merged 7 commits into from
Dec 2, 2022
Merged

Return a result for ResourceManager::get_resource #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fb01b1d
Start using Result instead of panic
adriantombu Nov 9, 2022
6bcae47
Start using workspace dependencies
adriantombu Nov 10, 2022
33fb164
Add comments on ResourceManagerError enum
adriantombu Nov 10, 2022
1089753
Fix tests
adriantombu Nov 22, 2022
408a71f
Revert using cargo worskspace dependencies
adriantombu Nov 30, 2022
0cfeff1
Fixes review comments
adriantombu Nov 30, 2022
1ed3ad2
Clippy fix
adriantombu Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions fluent-resmgr/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]}
Expand Down
26 changes: 13 additions & 13 deletions fluent-resmgr/examples/simple-resmgr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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) {
Expand Down
126 changes: 87 additions & 39 deletions fluent-resmgr/src/resource_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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);
gregtatum marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -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
Expand All @@ -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 {
gregtatum marked this conversation as resolved.
Show resolved Hide resolved
/// 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 {
Expand Down Expand Up @@ -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
Expand All @@ -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());
}
Expand Down
20 changes: 14 additions & 6 deletions fluent-resmgr/tests/localization_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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");
}

Expand Down