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.

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

Making IterableResourceProvider return Locales instead of ResourceOptions #2175

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions docs/tutorials/writing_a_new_data_struct.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,16 +216,16 @@ impl ResourceProvider<FooV1Marker> for FooProvider {
// Load the data from CLDR JSON and emit it as an ICU4X data struct.
// This is the core transform operation. This step could take a lot of
// work, such as pre-parsing patterns, re-organizing the data, etc.
// This method will be called once per option returned by supported_options.
// This method will be called once per option returned by supported_locales.
// Use internal mutability (RwLock) to avoid duplicating work.
}
}

impl IterableResourceProvider<FooV1Marker> for FooProvider {
fn supported_options(
fn supported_locales(
&self,
) -> Result<Vec<ResourceOptions>, DataError> {
// This should list all supported locales, for example.
) -> Result<Vec<Locale>, DataError> {
// This should list all supported locales.
}
}

Expand Down
14 changes: 7 additions & 7 deletions provider/adapters/src/either.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,14 @@ impl<M: DataMarker, P0: datagen::IterableDynProvider<M>, P1: datagen::IterableDy
datagen::IterableDynProvider<M> for EitherProvider<P0, P1>
{
#[inline]
fn supported_options_for_key(
fn supported_locales_for_key(
&self,
key: ResourceKey,
) -> Result<alloc::vec::Vec<ResourceOptions>, DataError> {
) -> Result<alloc::vec::Vec<icu_locid::Locale>, DataError> {
use EitherProvider::*;
match self {
A(p) => p.supported_options_for_key(key),
B(p) => p.supported_options_for_key(key),
A(p) => p.supported_locales_for_key(key),
B(p) => p.supported_locales_for_key(key),
}
}
}
Expand All @@ -100,11 +100,11 @@ impl<
> datagen::IterableResourceProvider<M> for EitherProvider<P0, P1>
{
#[inline]
fn supported_options(&self) -> Result<alloc::vec::Vec<ResourceOptions>, DataError> {
fn supported_locales(&self) -> Result<alloc::vec::Vec<icu_locid::Locale>, DataError> {
use EitherProvider::*;
match self {
A(p) => p.supported_options(),
B(p) => p.supported_options(),
A(p) => p.supported_locales(),
B(p) => p.supported_locales(),
}
}
}
13 changes: 4 additions & 9 deletions provider/adapters/src/filter/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ where
///
/// ```
/// use icu_locid::LanguageIdentifier;
/// use icu_locid::{langid, subtags_language as language, locale};
/// use icu_locid::{subtags_language as language, locale};
/// use icu_provider::datagen::*;
/// use icu_provider::hello_world::*;
/// use icu_provider::prelude::*;
Expand Down Expand Up @@ -55,14 +55,9 @@ where
/// ));
///
/// // English should not appear in the iterator result:
/// let supported_langids = provider
/// .supported_options()
/// .expect("Should successfully make an iterator of supported locales")
/// .into_iter()
/// .map(|options| options.get_langid())
/// .collect::<Vec<LanguageIdentifier>>();
/// assert!(supported_langids.contains(&langid!("de")));
/// assert!(!supported_langids.contains(&langid!("en")));
/// let locales = provider.supported_locales().expect("HelloWorldProvider infallible");
/// assert!(locales.contains(&locale!("de")));
/// assert!(!locales.contains(&locale!("en")));
/// ```
pub fn filter_by_langid<'a>(
self,
Expand Down
34 changes: 16 additions & 18 deletions provider/adapters/src/filter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use icu_provider::prelude::*;
///
/// Data requests that are rejected by the filter will return a [`DataError`] with kind
/// [`FilteredResource`](DataErrorKind::FilteredResource), and they will not be returned
/// by [`datagen::IterableDynProvider::supported_options_for_key`].
/// by [`datagen::IterableDynProvider::supported_locales_for_key`].
///
/// Although this struct can be created directly, the traits in this module provide helper
/// functions for common filtering patterns.
Expand Down Expand Up @@ -147,20 +147,19 @@ where
F: Fn(&DataRequest) -> bool,
D: datagen::IterableDynProvider<M>,
{
fn supported_options_for_key(
fn supported_locales_for_key(
&self,
key: ResourceKey,
) -> Result<alloc::vec::Vec<ResourceOptions>, DataError> {
self.inner.supported_options_for_key(key).map(|vec| {
) -> Result<alloc::vec::Vec<icu_locid::Locale>, DataError> {
self.inner.supported_locales_for_key(key).map(|vec| {
// Use filter_map instead of filter to avoid cloning the options
vec.into_iter()
.filter_map(move |options| {
let request = DataRequest {
options,
.filter_map(move |locale| {
if (self.predicate)(&DataRequest {
options: (&locale).into(),
metadata: Default::default(),
};
if (self.predicate)(&request) {
Some(request.options)
}) {
Some(locale)
} else {
None
}
Expand All @@ -177,17 +176,16 @@ where
F: Fn(&DataRequest) -> bool,
D: datagen::IterableResourceProvider<M>,
{
fn supported_options(&self) -> Result<alloc::vec::Vec<ResourceOptions>, DataError> {
self.inner.supported_options().map(|vec| {
fn supported_locales(&self) -> Result<alloc::vec::Vec<icu_locid::Locale>, DataError> {
self.inner.supported_locales().map(|vec| {
// Use filter_map instead of filter to avoid cloning the options
vec.into_iter()
.filter_map(move |options| {
let request = DataRequest {
options,
.filter_map(move |locale| {
if (self.predicate)(&DataRequest {
options: (&locale).into(),
metadata: Default::default(),
};
if (self.predicate)(&request) {
Some(request.options)
}) {
Some(locale)
} else {
None
}
Expand Down
18 changes: 7 additions & 11 deletions provider/adapters/src/fork/by_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

use alloc::vec::Vec;
#[cfg(feature = "datagen")]
use icu_locid::Locale;
#[cfg(feature = "datagen")]
use icu_provider::datagen;
use icu_provider::prelude::*;

Expand Down Expand Up @@ -170,15 +172,12 @@ where
P0: datagen::IterableDynProvider<M>,
P1: datagen::IterableDynProvider<M>,
{
fn supported_options_for_key(
&self,
key: ResourceKey,
) -> Result<Vec<ResourceOptions>, DataError> {
let result = self.0.supported_options_for_key(key);
fn supported_locales_for_key(&self, key: ResourceKey) -> Result<Vec<Locale>, DataError> {
let result = self.0.supported_locales_for_key(key);
if !result_is_err_missing_resource_key(&result) {
return result;
}
self.1.supported_options_for_key(key)
self.1.supported_locales_for_key(key)
}
}

Expand Down Expand Up @@ -304,12 +303,9 @@ where
M: DataMarker,
P: datagen::IterableDynProvider<M>,
{
fn supported_options_for_key(
&self,
key: ResourceKey,
) -> Result<Vec<ResourceOptions>, DataError> {
fn supported_locales_for_key(&self, key: ResourceKey) -> Result<Vec<Locale>, DataError> {
for provider in self.providers.iter() {
let result = provider.supported_options_for_key(key);
let result = provider.supported_locales_for_key(key);
if !result_is_err_missing_resource_key(&result) {
return result;
}
Expand Down
6 changes: 3 additions & 3 deletions provider/core/src/data_provider/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl ResourceProvider<HelloWorldV1Marker> for DataWarehouse {
}

impl IterableResourceProvider<HelloWorldV1Marker> for DataWarehouse {
fn supported_options(&self) -> Result<Vec<ResourceOptions>, DataError> {
fn supported_locales(&self) -> Result<Vec<icu_locid::Locale>, DataError> {
Ok(vec![Default::default()])
}
}
Expand Down Expand Up @@ -100,7 +100,7 @@ impl ResourceProvider<HelloWorldV1Marker> for DataProvider2 {
}

impl IterableResourceProvider<HelloWorldV1Marker> for DataProvider2 {
fn supported_options(&self) -> Result<Vec<ResourceOptions>, DataError> {
fn supported_locales(&self) -> Result<Vec<icu_locid::Locale>, DataError> {
Ok(vec![Default::default()])
}
}
Expand All @@ -115,7 +115,7 @@ impl ResourceProvider<HelloAltMarker> for DataProvider2 {
}

impl IterableResourceProvider<HelloAltMarker> for DataProvider2 {
fn supported_options(&self) -> Result<Vec<ResourceOptions>, DataError> {
fn supported_locales(&self) -> Result<Vec<icu_locid::Locale>, DataError> {
Ok(vec![Default::default()])
}
}
Expand Down
23 changes: 9 additions & 14 deletions provider/core/src/datagen/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,37 +5,32 @@
//! Collection of iteration APIs for data providers.

use crate::prelude::*;
use icu_locid::Locale;

/// A [`DynProvider`] that can iterate over all supported [`ResourceOptions`] for a certain key.
/// A [`DynProvider`] that can iterate over all supported [`Locale`]s for a certain key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: I am not aligned on the motivation for this change (trying to remove ResourceOptions from public APIs).

///
/// Implementing this trait means that a data provider knows all of the data it can successfully
/// return from a load request.
pub trait IterableDynProvider<M: DataMarker>: DynProvider<M> {
/// Given a [`ResourceKey`], returns a list of [`ResourceOptions`].
fn supported_options_for_key(
&self,
key: ResourceKey,
) -> Result<Vec<ResourceOptions>, DataError>;
/// Given a [`ResourceKey`], returns a list of [`Locale`]s.
fn supported_locales_for_key(&self, key: ResourceKey) -> Result<Vec<Locale>, DataError>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: We may want to use the name supported_locales_for_key for the actual supported-locales logic proposed in #58 (comment) in the post-1.x timeframe. I would like this trait to remain datagen-specific for the time being.

The other issue here is that I think we may want to retire this mechanism in the data exporter entirely, and instead use a declarative list of locales combined with locale fallback. At least when reading CLDR data, we only need to load the list of CLDR locales one time, and then use the same list for all of the data keys. See #834 for further discussion.

}

/// A [`ResourceProvider`] that can iterate over all supported [`ResourceOptions`] for a certain key.
/// A [`ResourceProvider`] that can iterate over all supported [`Locale`]s for a certain key.
///
/// Implementing this trait means that a data provider knows all of the data it can successfully
/// return from a load request.
pub trait IterableResourceProvider<M: ResourceMarker>: ResourceProvider<M> {
/// Returns a list of [`ResourceOptions`].
fn supported_options(&self) -> Result<Vec<ResourceOptions>, DataError>;
/// Returns a list of [`Locale`]s.
fn supported_locales(&self) -> Result<Vec<Locale>, DataError>;
}

impl<M, P> IterableDynProvider<M> for Box<P>
where
M: DataMarker,
P: IterableDynProvider<M> + ?Sized,
{
fn supported_options_for_key(
&self,
key: ResourceKey,
) -> Result<Vec<ResourceOptions>, DataError> {
(**self).supported_options_for_key(key)
fn supported_locales_for_key(&self, key: ResourceKey) -> Result<Vec<Locale>, DataError> {
(**self).supported_locales_for_key(key)
}
}
4 changes: 2 additions & 2 deletions provider/core/src/datagen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ macro_rules! make_exportable_provider {
);

impl $crate::datagen::IterableDynProvider<$crate::datagen::ExportMarker> for $provider {
fn supported_options_for_key(&self, key: $crate::ResourceKey) -> Result<Vec<$crate::ResourceOptions>, $crate::DataError> {
fn supported_locales_for_key(&self, key: $crate::ResourceKey) -> Result<Vec<icu_locid::Locale>, $crate::DataError> {
#![allow(non_upper_case_globals)]
// Reusing the struct names as identifiers
$(
Expand All @@ -67,7 +67,7 @@ macro_rules! make_exportable_provider {
match key.get_hash() {
$(
$struct_m => {
$crate::datagen::IterableResourceProvider::<$struct_m>::supported_options(self)
$crate::datagen::IterableResourceProvider::<$struct_m>::supported_locales(self)
}
)+,
_ => Err($crate::DataErrorKind::MissingResourceKey.with_key(key))
Expand Down
8 changes: 4 additions & 4 deletions provider/core/src/dynutil.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ where
/// }
///
/// impl IterableResourceProvider<HelloWorldV1Marker> for MyProvider {
/// fn supported_options(
/// fn supported_locales(
/// &self,
/// ) -> Result<Vec<ResourceOptions>, DataError> {
/// ) -> Result<Vec<icu_locid::Locale>, DataError> {
/// Ok(vec![Default::default()])
/// }
/// }
Expand Down Expand Up @@ -122,8 +122,8 @@ where
/// }
///
/// impl IterableDynProvider<HelloWorldV1Marker> for MyProvider {
/// fn supported_options_for_key(&self, _: ResourceKey)
/// -> Result<Vec<ResourceOptions>, DataError> {
/// fn supported_locales_for_key(&self, _: ResourceKey)
/// -> Result<Vec<icu_locid::Locale>, DataError> {
/// Ok(vec![Default::default()])
/// }
/// }
Expand Down
47 changes: 21 additions & 26 deletions provider/core/src/hello_world.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,40 +156,35 @@ impl BufferProvider for HelloWorldJsonProvider {

#[cfg(feature = "datagen")]
impl IterableResourceProvider<HelloWorldV1Marker> for HelloWorldProvider {
fn supported_options(&self) -> Result<Vec<ResourceOptions>, DataError> {
fn supported_locales(&self) -> Result<Vec<icu_locid::Locale>, DataError> {
#[allow(clippy::unwrap_used)] // datagen
Ok(Self::DATA
.iter()
.map(|(s, _)| s.parse::<icu_locid::LanguageIdentifier>().unwrap())
.map(ResourceOptions::from)
.collect())
Ok(Self::DATA.iter().map(|(s, _)| s.parse().unwrap()).collect())
}
}

#[test]
fn test_iter() {
use icu_locid::locale;
let supported_langids: Vec<ResourceOptions> = HelloWorldProvider.supported_options().unwrap();

assert_eq!(
supported_langids,
vec![
locale!("bn").into(),
locale!("cs").into(),
locale!("de").into(),
locale!("el").into(),
locale!("en").into(),
locale!("eo").into(),
locale!("fa").into(),
locale!("fi").into(),
locale!("is").into(),
locale!("ja").into(),
locale!("la").into(),
locale!("pt").into(),
locale!("ro").into(),
locale!("ru").into(),
locale!("vi").into(),
locale!("zh").into()
]
HelloWorldProvider.supported_locales(),
Ok(vec![
locale!("bn"),
locale!("cs"),
locale!("de"),
locale!("el"),
locale!("en"),
locale!("eo"),
locale!("fa"),
locale!("fi"),
locale!("is"),
locale!("ja"),
locale!("la"),
locale!("pt"),
locale!("ro"),
locale!("ru"),
locale!("vi"),
locale!("zh"),
]),
);
}
7 changes: 4 additions & 3 deletions provider/datagen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,11 @@ pub fn datagen(

keys.into_par_iter().try_for_each(|&key| {
log::info!("Writing key: {}", key);
let options = provider
.supported_options_for_key(key)
let locales = provider
.supported_locales_for_key(key)
.map_err(|e| e.with_key(key))?;
let res = options.into_par_iter().try_for_each(|options| {
let res = locales.into_par_iter().try_for_each(|locale| {
let options = ResourceOptions::from(locale);
let req = DataRequest {
options: options.clone(),
metadata: Default::default(),
Expand Down
4 changes: 2 additions & 2 deletions provider/datagen/src/transform/cldr/calendar/japanese.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use crate::transform::cldr::cldr_serde;
use crate::SourceData;
use icu_calendar::provider::*;
use icu_locid::langid;
use icu_locid::{langid, Locale};
use icu_provider::datagen::IterableResourceProvider;
use icu_provider::prelude::*;
use std::collections::BTreeMap;
Expand Down Expand Up @@ -194,7 +194,7 @@ fn era_to_code(original: &str, year: i32) -> Result<TinyStr16, String> {
icu_provider::make_exportable_provider!(JapaneseErasProvider, [JapaneseErasV1Marker,]);

impl IterableResourceProvider<JapaneseErasV1Marker> for JapaneseErasProvider {
fn supported_options(&self) -> Result<Vec<ResourceOptions>, DataError> {
fn supported_locales(&self) -> Result<Vec<Locale>, DataError> {
Ok(vec![Default::default()])
}
}
Expand Down
Loading