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

Add an example for accessing the resolved locale #3928

Merged
merged 13 commits into from
Aug 29, 2023

Conversation

sffc
Copy link
Member

@sffc sffc commented Aug 24, 2023

In relation to #3906 and #58

@sffc sffc requested a review from hsivonen August 24, 2023 00:58
@sffc sffc requested a review from a team as a code owner August 24, 2023 00:58
@sffc
Copy link
Member Author

sffc commented Aug 24, 2023

@robertbastian

---- src/lib.rs - data_provider (line 327) stdout ----
error: doctest failed, to rerun pass `-p tutorials-test --doc`
error[E0425]: cannot find function `any` in crate `icu_testdata`
  --> src/lib.rs:356:26
   |
31 |     inner: icu_testdata::any(),
   |                          ^^^ not found in `icu_testdata`
   |
error: aborting due to previous error

For more information about this error, try `rustc --explain E0[425](https://github.com/unicode-org/icu4x/actions/runs/5958142030/job/16161946606?pr=3928#step:9:426)`.
Couldn't compile the test.

failures:
    src/lib.rs - data_provider (line 327)

The local icu_testdata does not export an any() function unless you have the icu_locid_transform feature enabled. This is a breaking change. In 1.2, the required functionality lived in icu_provider_adapters which is non-optional. So I guess we should either make icu_locid_transform be non-optional, or do the crate split now (splitting icu_locid_transform into two crates, one more core than the other).

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

The local icu_testdata does not export an any() function unless you have the icu_locid_transform feature enabled. This is a breaking change. In 1.2, the required functionality lived in icu_provider_adapters which is non-optional. So I guess we should either make icu_locid_transform be non-optional, or do the crate split now (splitting icu_locid_transform into two crates, one more core than the other).

icu_locid_transform is a default feature, semver doesn't apply with default-features = false

// we consider the "resolved locale".
if key == DecimalSymbolsV1Marker::KEY {
let mut w = self.resolved_locale.write().unwrap();
*w = any_res.metadata.locale.clone();
Copy link
Member

Choose a reason for hiding this comment

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

I guess this could be take instead of clone? The FixedDecimalFormatter should not need the locale.

Copy link
Member

Choose a reason for hiding this comment

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

I'd really prefer if this would take. This would demonstrate that the locale is not actually used later, and it's only there to allow writing this specific wrapper.

@robertbastian
Copy link
Member

The tutorials test is failing because the legacy_testdata crate only exposes what is needed for tutorials, and any() hasn't been so far. In fact, could you use another provider for this demo? I'd rather not add a testdata tutorial, and I'd also rather not add an AnyProvider tutorial.

@sffc
Copy link
Member Author

sffc commented Aug 24, 2023

The tutorials test is failing because the legacy_testdata crate only exposes what is needed for tutorials, and any() hasn't been so far.

OK, so I should just go to the tutorials test Cargo.toml and enable the required feature.

In fact, could you use another provider for this demo?

Suggestion?

I'd rather not add a testdata tutorial, and I'd also rather not add an AnyProvider tutorial.

I don't know how to do this in a scalable way without using AnyProvider. And we already have an AnyProvider tutorial.

In general, this is a power user feature, so I'm okay forcing users who need this feature to use a custom manual data build.

@robertbastian
Copy link
Member

OK, so I should just go to the tutorials test Cargo.toml and enable the required feature.

You have to add the actual function, it's a custom lib.rs.

We're deprecating icu_testdata in 1.3, there should be some way to demonstrate this without it. I don't want icu_testdata in tutorials after 1.3.

@sffc
Copy link
Member Author

sffc commented Aug 24, 2023

We're deprecating icu_testdata in 1.3, there should be some way to demonstrate this without it. I don't want icu_testdata in tutorials after 1.3.

I hear you. I'm second-guessing that decision to deprecate now. We still use icu_testdata as a way to get a blob or any provider in tests, which is not a deprecated piece of functionality.

@sffc
Copy link
Member Author

sffc commented Aug 24, 2023

I guess I could just use HelloWorldProvider.

docs/tutorials/data_provider.md Outdated Show resolved Hide resolved
docs/tutorials/data_provider.md Outdated Show resolved Hide resolved
docs/tutorials/data_provider.md Outdated Show resolved Hide resolved
@sffc
Copy link
Member Author

sffc commented Aug 25, 2023

I think I need to add fallbacker data to the tutorials testdata crate

error[E0277]: the trait bound `UnstableDataProvider: DataProvider<LocaleFallbackLikelySubtagsV1Marker>` is not satisfied
   --> src/lib.rs:361:44
    |
error: doctest failed, to rerun pass `-p tutorials-test --doc`
36  |         LocaleFallbacker::try_new_unstable(&icu_testdata::unstable()).unwrap(),
    |         ---------------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `DataProvider<LocaleFallbackLikelySubtagsV1Marker>` is not implemented for `UnstableDataProvider`
    |         |
    |         required by a bound introduced by this call

robertbastian
robertbastian previously approved these changes Aug 27, 2023
Manishearth
Manishearth previously approved these changes Aug 28, 2023
@sffc sffc dismissed stale reviews from Manishearth and robertbastian via 6af777a August 29, 2023 00:19
robertbastian
robertbastian previously approved these changes Aug 29, 2023
docs/tutorials/data_provider.md Outdated Show resolved Hide resolved
docs/tutorials/data_provider.md Outdated Show resolved Hide resolved
@robertbastian robertbastian merged commit d419b46 into unicode-org:main Aug 29, 2023
25 checks passed
@sffc sffc deleted the resolved-locale branch November 21, 2023 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants