Creating `icu_experimental` crate #4564

robertbastian · 2024-01-31T13:26:04Z

Part of #4551

This contains compactdecimal, displaynames, relativetime, dimension, transliterate, and unicodeset_builder.

unitsconversion can be done once there are no outstanding PRs.

robertbastian · 2024-01-31T13:28:35Z

experimental/unitsconversion/src/provider.rs

@@ -24,13 +24,15 @@ use zerovec::{VarZeroVec, ZeroVec};
 /// </div>
 pub struct Baked;

+include!("../../../provider/datagen/tests/data/baked/macros.rs");


I made this use the stub data until we can move it into experimental. It's singleton anyway.

robertbastian · 2024-01-31T13:29:32Z

ffi/capi/Cargo.toml

@@ -45,7 +45,6 @@ buffer_provider = [
    "icu_collator?/serde",
    "icu_datetime?/serde",
    "icu_decimal?/serde",
-    "icu_displaynames?/serde",


I disabled displaynames FFI for now. We have to decide between pulling in all of experimental or not having it on FFI.

I think it should still take part in DCE and I wouldn't want to forbid a component from having FFI only because it is in icu_experimental.

Also, we said that moving the code from icu_experimental into its own crate should be a no-op PR that puts the repo into a releasable state with the crate at stable, and we can't do that if we then need to add FFI bindings. We should start drafting the FFI bindings while the code is in icu_experimental.

I lean toward including it in icu_capi and probably also in icu::experimental

We probably want to include it in icu_capi, but behind a experimental_components features (like in datagen), instead of one feature per module.

Regarding icu, I like the property that a module is either in icu or in icu_experimental, and that stabilisation moves it over. I also see value in removing the experimental feature from icu, we have too many features.

Actually I retract what I said about icu::experimental. I think we should include it in icu_capi.

robertbastian · 2024-01-31T13:30:23Z

provider/datagen/Cargo.toml

-icu_displaynames = { workspace = true, features = ["datagen"], optional = true }
-icu_relativetime = { workspace = true, features = ["datagen"], optional = true }
-icu_transliterate = { workspace = true, features = ["datagen"], optional = true }
-icu_unitsconversion = { workspace = true, features = ["datagen"], optional = true }


The repo becomes releasable with this PR

sffc

Praise: Nice work keeping the diffs to a nice reviewable state

sffc · 2024-01-31T16:35:22Z

Cargo.toml

    "experimental/unitsconversion",
+    "experimental/components",


Or components/experimental? 🤷

Soooo my thinking here is to move everything from components to the top level, and also move icu_experimental to the top level. It makes for a nicer GitHub page where our core crates are all listed out.

Let's not move things other than experimental components without first discussing with the team. I think we should land this PR and we can always move around the experimental components again later. experimental/components seems approximately as reasonable as components/experimental

yes this wasn't thinking for this pr

Soooo my thinking here is to move everything from components to the top level, and also move icu_experimental to the top level. It makes for a nicer GitHub page where our core crates are all listed out.

Hmm. I don't know if I'm a huge fan of this, I like the split we have right now.

sffc · 2024-01-31T16:38:21Z

docs/tutorials/.cargo/config.toml

-icu_relativetime = { path = "../../experimental/relativetime" }
-icu_transliterate = { path = "../../experimental/transliterate" }
-icu_unicodeset_parse = { path = "../../experimental/unicodeset_parse" }
+icu_experimental = { path = "../../experimental/experimental" }


Question: did you need to manually update this file or was it auto-generated? Is there something that enforces this file being up to date?

Manual. There's a comment on the top level Cargo toml that "enforces" this. Tutorials ci also breaks if you move or delete crates, however not if you add

experimental/components/Cargo.toml

sffc · 2024-01-31T16:56:13Z

experimental/components/Cargo.toml

+[dev-dependencies]
+icu_locid = { path = "../../components/locid" }
+
+[features]


Question: should we make each module in icu_experimental be feature-gated?

sffc · 2024-01-31T17:02:16Z

experimental/components/src/lib.rs

+pub mod transliterate;
+pub mod unicodeset_parse;
+
+#[doc(hidden)]


Nit: We don't normally put the provider module in doc(hidden), though we could consider making them internal when we implement #4467

Well we have n provider modules in this crate, but Baked needs to be in crate::provider because the constructor macro expects that. We can reexport Baked in foo::provider to have it public?

I think it's fine for crate::provider to behave as it normally does with baked data, and for the data structs to be defined in crate::foo::provider.

sffc · 2024-01-31T17:07:57Z

ffi/capi/Cargo.toml

@@ -45,7 +45,6 @@ buffer_provider = [
    "icu_collator?/serde",
    "icu_datetime?/serde",
    "icu_decimal?/serde",
-    "icu_displaynames?/serde",


Actually I retract what I said about icu::experimental. I think we should include it in icu_capi.

Co-authored-by: Shane F. Carr <[email protected]>

robertbastian · 2024-02-01T00:26:37Z

autosubmit=true

sffc · 2024-02-01T00:28:41Z

experimental/components/src/personnames/formatter.rs

@@ -2,11 +2,11 @@
 // called LICENSE at the top level of the ICU4X source tree
 // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).



I just noticed that you are moving personnames; there is a big PR which has been open for some months which I'm perpetually halfway through reviewing. (It's also there if you want to review it)

It should be a clean-enough move, and if it's been open for a few months it's not something I want to block on.

I'm gonna merge this PR and then update the personnames PR (#4050)

sffc · 2024-02-01T00:29:42Z

tools/make/tests.toml

+# icu_experimental is not currently published
+# exec --fail-on-error cargo run --release --manifest-path experimental/Cargo.toml


Tracking issue to re-enable this?

The website one, when we don't require the same test to work against release and head?

robertbastian commented Jan 31, 2024

View reviewed changes

robertbastian force-pushed the experimental branch from 3f187a0 to d5c1c5f Compare January 31, 2024 14:26

c

c0a15fa

robertbastian force-pushed the experimental branch from d5c1c5f to c0a15fa Compare January 31, 2024 14:28

robertbastian added 3 commits January 31, 2024 15:37

meta crate note

aa5d3d1

fmt

9d6e41c

fix

6987d76

robertbastian marked this pull request as ready for review January 31, 2024 17:07

robertbastian requested review from a team, Manishearth, pdogr, snktd and eggrobin as code owners January 31, 2024 17:07

robertbastian removed request for eggrobin, pdogr and snktd January 31, 2024 17:07

sffc reviewed Jan 31, 2024

View reviewed changes

robertbastian and others added 3 commits January 31, 2024 18:28

Update experimental/components/Cargo.toml

4662257

Co-authored-by: Shane F. Carr <[email protected]>

f

2baa837

tidy

02a2842

Manishearth previously approved these changes Jan 31, 2024

View reviewed changes

f

d7da2c1

robertbastian dismissed Manishearth’s stale review via d7da2c1 January 31, 2024 22:46

Manishearth previously approved these changes Jan 31, 2024

View reviewed changes

adf

f579b9b

robertbastian dismissed Manishearth’s stale review via f579b9b January 31, 2024 22:51

Manishearth previously approved these changes Jan 31, 2024

View reviewed changes

baked

3c9e038

robertbastian dismissed Manishearth’s stale review via 3c9e038 January 31, 2024 22:59

robertbastian added 4 commits February 1, 2024 00:01

d

b9fc268

s

40165a7

sadf

e03fe4d

gdhfgdh

154eb94

robertbastian requested review from sffc and Manishearth January 31, 2024 23:42

sffc reviewed Feb 1, 2024

View reviewed changes

sffc approved these changes Feb 1, 2024

View reviewed changes

sffc merged commit 7053d17 into unicode-org:main Feb 1, 2024
30 checks passed

robertbastian deleted the experimental branch February 6, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating `icu_experimental` crate #4564

Creating `icu_experimental` crate #4564

robertbastian commented Jan 31, 2024 •

edited

Loading

robertbastian Jan 31, 2024

robertbastian Jan 31, 2024

sffc Jan 31, 2024

robertbastian Jan 31, 2024

sffc Jan 31, 2024

robertbastian Jan 31, 2024

sffc left a comment

sffc Jan 31, 2024

robertbastian Jan 31, 2024

sffc Jan 31, 2024

robertbastian Jan 31, 2024

Manishearth Jan 31, 2024

sffc Jan 31, 2024

robertbastian Jan 31, 2024

sffc Jan 31, 2024

sffc Jan 31, 2024

robertbastian Jan 31, 2024

sffc Jan 31, 2024

sffc Jan 31, 2024

robertbastian commented Feb 1, 2024

sffc Feb 1, 2024

robertbastian Feb 1, 2024

sffc Feb 1, 2024

sffc Feb 1, 2024

robertbastian Feb 1, 2024

		@@ -2,11 +2,11 @@
		// called LICENSE at the top level of the ICU4X source tree
		// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

		# icu_experimental is not currently published
		# exec --fail-on-error cargo run --release --manifest-path experimental/Cargo.toml

Creating icu_experimental crate #4564

Creating icu_experimental crate #4564

Conversation

robertbastian commented Jan 31, 2024 • 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 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian commented Feb 1, 2024

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

Creating `icu_experimental` crate #4564

Creating `icu_experimental` crate #4564

robertbastian commented Jan 31, 2024 •

edited

Loading