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

Remove implicit LocaleFallbackProvider constructors #5183

Merged
merged 1 commit into from
Jul 9, 2024
Merged
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
6 changes: 4 additions & 2 deletions components/icu/README.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions components/icu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
//! ```no_run
//! use icu::datetime::DateTimeFormatter;
//! use icu::locale::locale;
//! use icu::locale::fallback::LocaleFallbacker;
//! use icu_provider_adapters::fallback::LocaleFallbackProvider;
//! use icu_provider_blob::BlobDataProvider;
//!
Expand All @@ -61,10 +62,11 @@
//! let provider = BlobDataProvider::try_new_from_blob(data)
//! .expect("data should be valid");
//!
//! let provider =
//! LocaleFallbackProvider::try_new_with_buffer_provider(provider)
//! let fallbacker = LocaleFallbacker::try_new_with_buffer_provider(&provider)
//! .expect("provider should include fallback data");
//!
//! let provider = LocaleFallbackProvider::new(provider, fallbacker);
//!
//! let dtf = DateTimeFormatter::try_new_with_buffer_provider(
//! &provider,
//! &locale!("es-US").into(),
Expand Down
3 changes: 0 additions & 3 deletions ffi/capi/bindings/c/ICU4XDataProvider.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions ffi/capi/bindings/cpp/ICU4XDataProvider.d.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 0 additions & 8 deletions ffi/capi/bindings/cpp/ICU4XDataProvider.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 1 addition & 23 deletions ffi/capi/bindings/dart/DataProvider.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 1 addition & 14 deletions ffi/capi/bindings/js/ICU4XDataProvider.d.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 0 additions & 17 deletions ffi/capi/bindings/js/ICU4XDataProvider.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 5 additions & 35 deletions ffi/capi/src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,39 +176,8 @@ pub mod ffi {
Ok(())
}

/// Enables locale fallbacking for data requests made to this provider.
///
/// Note that the test provider (from `create_test`) already has fallbacking enabled.
#[diplomat::rust_link(
icu_provider_adapters::fallback::LocaleFallbackProvider::try_new,
FnInStruct
)]
#[diplomat::rust_link(
icu_provider_adapters::fallback::LocaleFallbackProvider,
Struct,
compact
)]
pub fn enable_locale_fallback(&mut self) -> Result<(), ICU4XDataError> {
Copy link
Member

Choose a reason for hiding this comment

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

Not 100% sure we want to bring the change to FFI since the alternative may be less efficient (an extra allocation and FFI call), but it seems like something we could add back later if needed

Copy link
Member

Choose a reason for hiding this comment

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

Fewer mutable methods over FFI is better anyway

use ICU4XDataProviderInner::*;
*self = match core::mem::replace(&mut self.0, Destroyed) {
Destroyed => Err(icu_provider::DataError::custom(
"This provider has been destroyed",
))?,
#[cfg(feature = "compiled_data")]
Compiled => Err(icu_provider::DataError::custom(
"The compiled provider cannot be modified",
))?,
Empty => Err(icu_provider::DataErrorKind::MarkerNotFound.into_error())?,
#[cfg(feature = "buffer_provider")]
Buffer(inner) => convert_buffer_provider(
LocaleFallbackProvider::try_new_with_buffer_provider(inner)?,
),
};
Ok(())
}

#[diplomat::rust_link(
icu_provider_adapters::fallback::LocaleFallbackProvider::new_with_fallbacker,
icu_provider_adapters::fallback::LocaleFallbackProvider::new,
FnInStruct
)]
#[diplomat::rust_link(
Expand All @@ -233,9 +202,10 @@ pub mod ffi {
))?,
Empty => Err(icu_provider::DataErrorKind::MarkerNotFound.into_error())?,
#[cfg(feature = "buffer_provider")]
Buffer(inner) => convert_buffer_provider(
LocaleFallbackProvider::new_with_fallbacker(inner, fallbacker.0.clone()),
),
Buffer(inner) => convert_buffer_provider(LocaleFallbackProvider::new(
inner,
fallbacker.0.clone(),
)),
};
Ok(())
}
Expand Down
3 changes: 0 additions & 3 deletions provider/adapters/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,3 @@ serde = { workspace = true, features = ["derive", "alloc"], optional = true }
icu_provider = { path = "../../provider/core", features = ["macros", "deserialize_json"] }
icu_locale = { path = "../../components/locale" }
writeable = { path = "../../utils/writeable" }

[features]
serde = ["dep:serde", "zerovec/serde", "icu_locale/serde", "icu_provider/serde"]
89 changes: 13 additions & 76 deletions provider/adapters/src/fallback/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
//! A data provider wrapper that performs locale fallback.

use crate::helpers::result_is_err_missing_locale;
use icu_locale::provider::*;
use icu_locale::LocaleFallbacker;
#[doc(no_inline)]
pub use icu_locale::LocaleFallbacker;
use icu_provider::prelude::*;
use icu_provider::DryDataProvider;
use icu_provider::DynamicDryDataProvider;
Expand All @@ -20,13 +20,9 @@ use icu_provider::DynamicDryDataProvider;
/// use icu_locale::langid;
/// use icu_provider::prelude::*;
/// use icu_provider::hello_world::*;
/// # let provider = HelloWorldProvider;
/// # struct LocaleFallbackProvider;
/// # impl LocaleFallbackProvider {
/// # fn try_new_unstable(provider: HelloWorldProvider) -> Result<icu_provider_adapters::fallback::LocaleFallbackProvider<HelloWorldProvider>, ()> {
/// # Ok(icu_provider_adapters::fallback::LocaleFallbackProvider::new_with_fallbacker(provider, icu_locale::LocaleFallbacker::new().static_to_owned()))
/// # }
/// # }
/// use icu_provider_adapters::fallback::LocaleFallbackProvider;
///
/// let provider = HelloWorldProvider;
///
/// let id = DataIdentifierCow::from_locale(langid!("ja-JP").into());
///
Expand All @@ -37,8 +33,7 @@ use icu_provider::DynamicDryDataProvider;
/// }).expect_err("No fallback");
///
/// // But if we wrap the provider in a fallback provider...
/// let provider = LocaleFallbackProvider::try_new_unstable(provider)
/// .expect("Fallback data present");
/// let provider = LocaleFallbackProvider::new(provider, icu_locale::LocaleFallbacker::new().static_to_owned());
///
/// // ...then we can load "ja-JP" based on "ja" data
/// let response =
Expand All @@ -62,67 +57,11 @@ pub struct LocaleFallbackProvider<P> {
fallbacker: LocaleFallbacker,
}

impl<P> LocaleFallbackProvider<P>
where
P: DataProvider<LocaleFallbackLikelySubtagsV1Marker>
+ DataProvider<LocaleFallbackParentsV1Marker>
+ DataProvider<CollationFallbackSupplementV1Marker>,
{
/// Create a [`LocaleFallbackProvider`] by wrapping another data provider and then loading
/// fallback data from it.
///
/// If the data provider being wrapped does not contain fallback data, use
/// [`LocaleFallbackProvider::new_with_fallbacker`].
pub fn try_new_unstable(provider: P) -> Result<Self, DataError> {
let fallbacker = LocaleFallbacker::try_new_unstable(&provider)?;
Ok(Self {
inner: provider,
fallbacker,
})
}
}

impl<P> LocaleFallbackProvider<P>
where
P: AnyProvider,
{
/// Create a [`LocaleFallbackProvider`] by wrapping another data provider and then loading
/// fallback data from it.
///
/// If the data provider being wrapped does not contain fallback data, use
/// [`LocaleFallbackProvider::new_with_fallbacker`].
pub fn try_new_with_any_provider(provider: P) -> Result<Self, DataError> {
let fallbacker = LocaleFallbacker::try_new_with_any_provider(&provider)?;
Ok(Self {
inner: provider,
fallbacker,
})
}
}

#[cfg(feature = "serde")]
impl<P> LocaleFallbackProvider<P>
where
P: BufferProvider,
{
/// Create a [`LocaleFallbackProvider`] by wrapping another data provider and then loading
/// fallback data from it.
///
/// If the data provider being wrapped does not contain fallback data, use
/// [`LocaleFallbackProvider::new_with_fallbacker`].
pub fn try_new_with_buffer_provider(provider: P) -> Result<Self, DataError> {
let fallbacker = LocaleFallbacker::try_new_with_buffer_provider(&provider)?;
Ok(Self {
inner: provider,
fallbacker,
})
}
}

impl<P> LocaleFallbackProvider<P> {
/// Wrap a provider with an arbitrary fallback engine.
/// Wraps a provider with a provider performing fallback under the given fallbacker.
///
/// This relaxes the requirement that the wrapped provider contains its own fallback data.
/// If the underlying provider contains deduplicated data, it is important to use the
/// same fallback data that `ExportDriver` used.
///
/// # Examples
///
Expand All @@ -147,7 +86,7 @@ impl<P> LocaleFallbackProvider<P> {
/// // `HelloWorldProvider` does not contain fallback data,
/// // but we can construct a fallbacker with `icu_locale`'s
/// // compiled data.
/// let provider = LocaleFallbackProvider::new_with_fallbacker(
/// let provider = LocaleFallbackProvider::new(
/// provider,
/// LocaleFallbacker::new().static_to_owned(),
/// );
Expand All @@ -162,7 +101,7 @@ impl<P> LocaleFallbackProvider<P> {
///
/// assert_eq!("Hallo Welt", german_hello_world.payload.get().message);
/// ```
pub fn new_with_fallbacker(provider: P, fallbacker: LocaleFallbacker) -> Self {
pub fn new(provider: P, fallbacker: LocaleFallbacker) -> Self {
Self {
inner: provider,
fallbacker,
Expand Down Expand Up @@ -340,10 +279,8 @@ fn dry_test() {
}
}

let provider = LocaleFallbackProvider::new_with_fallbacker(
TestProvider,
LocaleFallbacker::new().static_to_owned(),
);
let provider =
LocaleFallbackProvider::new(TestProvider, LocaleFallbacker::new().static_to_owned());

assert_eq!(
provider
Expand Down
2 changes: 1 addition & 1 deletion provider/blob/benches/auxkey_bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ fn auxkey_bench_for_version(c: &mut Criterion, blob: &[u8], version_id: &str) {
b.iter(|| BlobDataProvider::try_new_from_blob(black_box(blob).into()).unwrap());
});

let provider = LocaleFallbackProvider::new_with_fallbacker(
let provider = LocaleFallbackProvider::new(
BlobDataProvider::try_new_from_blob(black_box(blob).into()).unwrap(),
LocaleFallbacker::new().static_to_owned(),
);
Expand Down
7 changes: 5 additions & 2 deletions provider/source/src/collator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,10 +291,13 @@ collation_provider!(
fn test_zh_non_baked() {
use core::cmp::Ordering;
use icu::collator::{Collator, CollatorOptions};
use icu::locale::fallback::LocaleFallbacker;
use icu_provider_adapters::fallback::LocaleFallbackProvider;

let provider =
LocaleFallbackProvider::try_new_unstable(SourceDataProvider::new_testing()).unwrap();
let provider = LocaleFallbackProvider::new(
SourceDataProvider::new_testing(),
LocaleFallbacker::new_without_data(),
);

// Note: ㄅ is Bopomofo.
{
Expand Down
Loading