Replacing datagen options by DatagenDriver
#3861
Conversation
robertbastian
force-pushed
the
driver
branch
4 times, most recently
from
abfc396
to
860986c
Compare
robertbastian
requested review from
a team,
sffc and
Manishearth
as code owners
robertbastian
changed the title
DataExportDriverDatagenDriver
| /// are excluded. This method can be used to reennable them. | ||
| /// | ||
| /// The special string `"search*"` causes all search collation tables to be included. | ||
| pub fn with_collations(self, collations: impl IntoIterator<Item = String>) -> Self { |
There was a problem hiding this comment.
should this be called with_addtional_collations or something? Because most collations are preselected and cannot be removed
There was a problem hiding this comment.
good question and I like your suggestion; let's not block this pr on the resolution but we should decide before 1.3
robertbastian
requested review from
zbraniecki,
gregtatum and
nordzilla
as code owners
| /// [`icu_datagen::all_keys`]: crate::all_keys | ||
| /// [`icu_datagen::key`]: crate::key | ||
| /// [`icu_datagen::keys_from_bin`]: crate::keys_from_bin | ||
| pub fn with_keys(self, keys: impl IntoIterator<Item = DataKey>) -> Self { |
There was a problem hiding this comment.
Not sure about impl IntoIterator -- if we need the vec or hashset then why not just take the vec or hashset? It's nice that it keeps the internal collection hidden, but if you already have a hashset then this is slower. (ok to discuss in a follow up)
There was a problem hiding this comment.
I found this to be much more ergonomic. In tests I can just pass arrays, and also keys, keys_for_bin and keys_from_file return Vec<DataKey>, so you always end up writing .into_iter().collect() at the call site.
| /// are excluded. This method can be used to reennable them. | ||
| /// | ||
| /// The special string `"search*"` causes all search collation tables to be included. | ||
| pub fn with_collations(self, collations: impl IntoIterator<Item = String>) -> Self { |
There was a problem hiding this comment.
good question and I like your suggestion; let's not block this pr on the resolution but we should decide before 1.3
There was a problem hiding this comment.
We really should either change the bounds on the export function to be stable (by loading the likely subtags data some roundabout way via ExportMarker) or make the function be named export_unstable but I'm okay doing that after landing the mammoth PR so long as it is tracked to block 1.3
|
We should load it through |
| @@ -147,12 +138,7 @@ impl DatagenDriver { | |||
| // Avoids multiple monomorphizations | |||
There was a problem hiding this comment.
Question: only sink gets a trait object; is it worth it?
There was a problem hiding this comment.
idk, but provider is not object safe so it can't
The new architecture has two main structs:
DatagenProviderandDatagenDriver.SourceDatais deprecated and put behind thelegacy_apiflag, data is now added directly to theDatagenProvider.DatagenDriveris used to configure export options, and contains the export logic such as locale selection. These two structs could in the future live in different crates.Misc fixes:
dataintotests/data. This is required as the new API does not include fallback data, so the segmentation dictionaries have to be in the correct location insidetests/data/icuexport. The legacy API thus pulls in data fromtests/data, but it does so usinginclude_str!which I hope is fine.Fixes #3795, #3800
Part of #3564