Split DataProvider into ResourceProvider and DynProvider #1554

sffc · 2022-01-28T05:16:53Z

Fixes #570

This PR adds ResourceMarker, which extends DataMarker, adding a KEY field. There is a corresponding ResourceProvider trait which uses the ResourceMarker to auto-fill the key in requests. The DataProvider trait has been renamed to DynProvider to serve more specific use cases. Throughout components, I migrated most users over to ResourceProvider.

There does not appear to be any impact on static data slicing.

Open questions:

I left the big icu_properties crate on DynProvider. I have a stash where I migrated it to ResourceProvider, which works for plain binary properties, but it does not work very well for binary enumerated properties. Should I:
1. Leave everything in icu_properties as DynProvider, or
2. Migrate what I can to ResourceProvider, or
3. Leave it as-is for now and open a follow-up issue
Should I add a macro to generate the ResourceMarker impls? It seems that writing them by hand isn't really too bad.

Still to-do:

Migrate icu_provider_cldr and icu_provider_uprops
Add documentation for new exported symbols
Fix docs tests everywhere

experimental/list_formatter/src/list_formatter.rs

robertbastian · 2022-01-28T10:58:58Z

components/decimal/src/provider.rs

+impl ResourceMarker for DecimalSymbolsV1Marker {
+    const KEY: ResourceKey = key::SYMBOLS_V1;
+}


You're right that a macro just for this impl isn't much use. However I think we should aim for a 1-1 correspondence between ResourceMarkers and keys, because defining ResourceMarker on different structs with the same key seems like a footgun.

I'd say a ResourceKey and its single ResourceMarker impl should be generated by the same macro. Maybe something like the current #[data_struct] that also generates the keys and links them:

#[icu_provider::resource_struct] #[resource_key(KEY_NAME, "key/path@1")] #[resource_key(KEY_NAME_2, "key/path_2@1")] pub struct FooV1<'data> { ... }

or alternatively with more prominent keys:

#[icu_provider::data_struct] pub struct FooV1<'data> { ... } define_resource!(KEY_NAME, "key/path@1", FooV1); define_resource!(KEY_NAME_2, "key/path_2@1", FooV1);

i like this idea

Yeah! We should do something along these lines. I like the first version better. I would also like to get rid of the separate key constants, because you will be able to access them via FooV1Marker::KEY.

Here's another interesting proposal. We can allow only one key per struct, and for structs with multiple keys, we make them newtypes with #[serde(transparent)]. They can be cast to the inner type if needed.

Here's what I got working:

use icu_provider::prelude::*; use std::borrow::Cow; // We also need `yoke` in scope: <https://github.com/unicode-org/icu4x/issues/1557> use icu_provider::yoke; #[icu_provider::data_struct( "demo/foo@1", BarV1Marker = "demo/bar@1", BazV1Marker = "demo/baz@1" )] pub struct FooV1<'data> { message: Cow<'data, str>, }; assert_eq!(FooV1Marker::KEY.get_path(), "demo/foo@1"); assert_eq!(BarV1Marker::KEY.get_path(), "demo/bar@1"); assert_eq!(BazV1Marker::KEY.get_path(), "demo/baz@1");

Cool!

Do we need custom marker names? We could derive them from the key path: "demo/foo@1" -> DemoFooV1Marker (or maybe ignore the name space as the rust module takes that role).

Not a fan of the newtype solution as that would add quite a bit of boilerplate.

I strongly dislike the practice performed by a handful of computer frameworks that auto-generate names by munging other names into a different case convention. I would much rather have the literal symbol DemoFooV1Marker be in source code.

In that case I think it'd be neater if the default marker name wasn't generated from the struct name but was also a required param.

And if we don't use equivalent names, should we at least verify that the key's version matches the struct's version?

Good call. I changed the macro so that the marker type is explicit.

And if we don't use equivalent names, should we at least verify that the key's version matches the struct's version?

Seems unnecessary to me, but we could do it in a follow-up.

Manishearth

i like this approach so far!

provider/core/src/data_provider.rs

components/plurals/src/lib.rs

Manishearth · 2022-01-28T22:43:40Z

components/decimal/src/provider.rs

+impl ResourceMarker for DecimalSymbolsV1Marker {
+    const KEY: ResourceKey = key::SYMBOLS_V1;
+}


i like this idea

provider/core/src/data_provider.rs

Manishearth · 2022-01-28T22:46:29Z

I left the big icu_properties crate on DynProvider. I have a stash where I migrated it to ResourceProvider, which works for plain binary properties, but it does not work very well for binary enumerated properties. Should I:

Leave everything in icu_properties as DynProvider, or

Migrate what I can to ResourceProvider, or

Leave it as-is for now and open a follow-up issue

(3) seems okay to me, but my personal preference would be (2)? Is there a reason why it doesn't work well for binary enumprops?

Should I add a macro to generate the ResourceMarker impls? It seems that writing them by hand isn't really too bad.

I think it would be nicer yeah but I don't consider it mandatory

provider/blob/src/path_util.rs

provider/core/src/data_provider.rs

provider/core/src/data_provider/test.rs

provider/core/src/hello_world.rs

provider/core/src/resource.rs

robertbastian · 2022-02-02T12:49:21Z

provider/cldr/src/transform/time_zones/mod.rs

@@ -68,14 +68,17 @@ impl TryFrom<&str> for TimeZonesProvider {
 impl KeyedDataProvider for TimeZonesProvider {


Should KeyedDataProvider be KeyedDynProvider now?

Maybe. But I want to get rid of KeyedDataProvider and it is tracked in #442, and it is a private internal type to provider_cldr. So I prefer not to mess with it right now.

provider/core/src/data_provider.rs

robertbastian · 2022-02-02T12:56:32Z

provider/core/src/error.rs

@@ -203,13 +204,14 @@ impl DataError {
    ///
    /// If the "log_error_context" feature is enabled, this logs the whole request. Either way,
    /// it returns an error with the resource key portion of the request as context.
-    pub fn with_req(self, req: &DataRequest) -> Self {
+    #[cfg_attr(not(feature = "log_error_context"), allow(unused_variables))]
+    pub fn with_req(self, key: ResourceKey, req: &DataRequest) -> Self {


I think these functions should be adding one piece of context at a time. If the client wants to log both key and request they can do .with_key(k).with_options(o), but they should be able to only log options as well (we already have key-only). This is useful for

RequestOptions::try_langid, which can return DataErrorKind::NeedsLocale.with_options(self)

DataPayload::try_langid which wouldn't have to accept a context: ResourceKey, as that can be added by the caller with with_key

I wanted with_req to require both arguments because they are both important as context for the errors. This is a side-effect of splitting the key and the options into separate arguments.

robertbastian · 2022-02-02T13:03:54Z

components/decimal/src/provider.rs

+impl ResourceMarker for DecimalSymbolsV1Marker {
+    const KEY: ResourceKey = key::SYMBOLS_V1;
+}


In that case I think it'd be neater if the default marker name wasn't generated from the struct name but was also a required param.

And if we don't use equivalent names, should we at least verify that the key's version matches the struct's version?

robertbastian

I'm a bit confused by naming. You dropped the DataProvider type for ResourceProvider and DynProvider, but kept data_provider parameter names and use "data provider" in docs.

components/plurals/src/lib.rs

robertbastian · 2022-02-02T15:57:59Z

provider/core/src/resource.rs

+        }
+    }
+}
+
 impl ResourceOptions {


While we're changing the API, I think we should add constructors for ResourceOptions and make the fields private. Currently there is no instance that has a variant but no langid, and I think we should require that. Otherwise we cannot parse stringified ResourceOptions, as both

ResourceOptions { variant: Some(Cow::Borrowed("und")), langid: None}

ResourceOptions { variant: None, langid: Some(langid!("und")) }

are 'serialized' to "und".

We need that for #1092.

As discussed, I don't want to make this change right now because I am reconsidering the whole data model of ResourceOptions as part of #243.

Co-authored-by: Robert Bastian <robertbastian@users.noreply.github.com>

sffc · 2022-02-03T00:22:14Z

Code size:

Before:

-rwxr-xr-x 1 runner docker    41360 Feb  2 23:16 optim4.elf
-rwxr-xr-x 1 runner docker    31592 Feb  2 23:16 optim5.elf

After:

-rwxr-xr-x 1 runner docker    41360 Feb  3 00:11 optim4.elf
-rwxr-xr-x 1 runner docker    31560 Feb  3 00:11 optim5.elf

Manishearth · 2022-02-03T18:51:54Z

provider/core/README.md

+
+All data providers can fit into one of two classes.
+
+1. Type 1: Those whose data originates as structured Rust objects


issue: I'm not a huge fan of "type 1" and "type 2" naming, can we just call these any providers and buffered providers?

#1576 (comment)

sffc added 18 commits January 27, 2022 12:21

Initial ResourceMarker and ResourceProvider definitions

238a44c

Checkpoint

0c7f4f2

Remove redundant module annotation

b60b909

Big refactor around new traits

a9e1b8d

cargo check --all-targets --all-features in core crate

10dbe6f

icu_provider_blob building

600cfbf

icu_provider_fs building

d2ce258

Delete blanket impls

3b0b5c9

Add DataProvider::cast method

e591f9a

icu_properties via DynProvider

aaa8132

Fixing up some components

9a19c8c

Merge remote-tracking branch 'upstream/main' into resourcemarker

9098d3e

More migrations

c1af44d

More migrations

70b6821

ResourceProvider in plurals

86b1e91

Split plural rules constructors

f7412eb

Migrate call sites to type-specific PluralRules constructors

f330a8a

fmt

579179b

sffc requested a review from Manishearth January 28, 2022 05:16

robertbastian reviewed Jan 28, 2022

View reviewed changes

Manishearth reviewed Jan 28, 2022

View reviewed changes

sffc added 8 commits January 28, 2022 17:17

Add basic tests to icu_provider_macros

beaca9c

Add basic ResourceMarker to macro

4174efd

Improve test

e1ecce8

Add multi-key support

54a6a94

Make docs more consistent

7077b75

Add docs example

d72d419

Add namespace to ZeroCopyFrom in yoke derive

f858b38

Use new data_struct attribute across components

1e6b178

sffc removed request for zbraniecki, makotokato, gregtatum and nordzilla February 2, 2022 08:01

sffc added 3 commits February 2, 2022 00:34

Merge remote-tracking branch 'upstream/main' into resourcemarker

860e21f

Fix build

9772515

Remove unneeded type info

842215a

robertbastian reviewed Feb 2, 2022

View reviewed changes

robertbastian requested changes Feb 2, 2022

View reviewed changes

components/plurals/src/lib.rs Show resolved Hide resolved

robertbastian reviewed Feb 2, 2022

View reviewed changes

sffc and others added 8 commits February 2, 2022 12:33

Fix another compile error

127aaf0

try_langid -> get_langid

099ab34

Update data_struct macro to require explicit marker symbol path

b2c8f7d

Migrate call sites to new macro

8a47ffa

Make PluralRulesV1Marker public

d2902af

Remove ResourcePath and DataRequestOld

14fefcc

Remove unused file

51cf962

Update provider/macros/src/lib.rs

8f992bf

Co-authored-by: Robert Bastian <robertbastian@users.noreply.github.com>

sffc mentioned this pull request Feb 2, 2022

Implement naming convention for "data provider" #1576

Closed

4 tasks

sffc requested a review from robertbastian February 2, 2022 22:38

Merge branch 'main' into resourcemarker

1217eed

robertbastian previously approved these changes Feb 3, 2022

View reviewed changes

fmt

16120c6

sffc dismissed robertbastian’s stale review via 16120c6 February 3, 2022 08:14

robertbastian approved these changes Feb 3, 2022

View reviewed changes

Manishearth approved these changes Feb 3, 2022

View reviewed changes

sffc merged commit 20787e7 into unicode-org:main Feb 3, 2022

sffc deleted the resourcemarker branch February 3, 2022 21:49

Manishearth mentioned this pull request Mar 10, 2022

Add payload conversion and heap measurement tool #1670

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split DataProvider into ResourceProvider and DynProvider #1554

Split DataProvider into ResourceProvider and DynProvider #1554

sffc commented Jan 28, 2022 •

edited

Loading

robertbastian Jan 28, 2022

Manishearth Jan 28, 2022

sffc Jan 29, 2022

sffc Jan 29, 2022

sffc Jan 29, 2022

robertbastian Jan 29, 2022

sffc Feb 2, 2022 •

edited

Loading

robertbastian Feb 2, 2022

sffc Feb 2, 2022

Manishearth left a comment

Manishearth Jan 28, 2022

Manishearth commented Jan 28, 2022

robertbastian Feb 2, 2022

sffc Feb 2, 2022 •

edited

Loading

robertbastian Feb 2, 2022

sffc Feb 2, 2022

robertbastian Feb 2, 2022

robertbastian left a comment

robertbastian Feb 2, 2022

sffc Feb 2, 2022

sffc commented Feb 3, 2022

Manishearth Feb 3, 2022

sffc Feb 3, 2022

		@@ -68,14 +68,17 @@ impl TryFrom<&str> for TimeZonesProvider {
		impl KeyedDataProvider for TimeZonesProvider {


		All data providers can fit into one of two classes.

		1. Type 1: Those whose data originates as structured Rust objects

Split DataProvider into ResourceProvider and DynProvider #1554

Split DataProvider into ResourceProvider and DynProvider #1554

Conversation

sffc commented Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Jan 28, 2022

Choose a reason for hiding this comment

sffc Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Jan 28, 2022 •

edited

Loading

sffc Feb 2, 2022 •

edited

Loading

sffc Feb 2, 2022 •

edited

Loading