Moving data provider structs into their own crates

[sffc]

Fixes #415

I put the structs into a pub mod provider, which is a convention we can adopt across all components.

Note that there was already a file named "provider.rs" in datetime; I renamed that file to "provider_internal.rs" before moving the structs file into "provider.rs". Please keep this in mind when reviewing!

I made the serde feature optional on the struct, which means we can use the struct without serde (e.g. Static DataProvider).

[coveralls]

Pull Request Test Coverage Report for Build b4f699c028cb2f087ba3e5b95d90e91408cd178d-PR-451

• 11 of 68 (16.18%) changed or added relevant lines in 6 files are covered.
• 5 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.05%) to 74.581%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/plurals/src/provider.rs	1	2	50.0%
components/uniset/src/provider.rs	0	1	0.0%
components/locale_canonicalizer/src/provider.rs	0	2	0.0%
components/provider/src/hello_world.rs	7	14	50.0%
components/datetime/src/provider/mod.rs	0	46	0.0%

Files with Coverage Reduction	New Missed Lines	%
components/provider/src/error.rs	1	0%
components/provider/src/inv.rs	4	50.0%

Totals
Change from base Build f5915f19bfd775c930e31dc4a600d771efad746c:	0.05%
Covered Lines:	4677
Relevant Lines:	6271

---

💛 - Coveralls

[zbraniecki]

lgtm!

I'd only suggest moving provider.rs and provider_internal.rs to provider/structs.rs and provider/mod.rs.

[sffc]

lgtm!

I'd only suggest moving provider.rs and provider_internal.rs to provider/structs.rs and provider/mod.rs.

Reasons why I'd rather not use that particular naming scheme:

1. I'd like it if, by convention, data provider structs were directly inside the provider module of all components
2. The stuff inside internal_provider.rs was and still is crate-private
3. This file may be largely re-written as part of my work on Split date time data into smaller data keys? #257 and Finalize input data model for DateTimeFormat #355..

I would be okay with some other name than internal_provider, but I do feel strongly that provider should not be redefined.

[zbraniecki]

lgtm!
I'd only suggest moving provider.rs and provider_internal.rs to provider/structs.rs and provider/mod.rs.

Reasons why I'd rather not use that particular naming scheme:

1. I'd like it if, by convention, data provider structs were directly inside the `provider` module of all components

That seems like a very subjective preference, boiling down to use crate::provider::structs::X; vs use crate::provider::X.
But sure, you can always have in provider/mod.rs pub use structs::*; and you have your goal met.

2. The stuff inside internal_provider.rs was and still is crate-private

That's accomplished by the model I proposed.

I would be okay with some other name than internal_provider, but I do feel strongly that provider should not be redefined.

The file structure I proposed can provide what you're looking for. I'm very fond of clustering analogous mental spaces in folders.
I find it vastly easier to reason about the code when there's src/provider dir with modules inside, than foo_provider.rs, bar_provider.rs, provider.rs and baz_provider.rs in a src directory mixed with 5-10 other files.

How you expose those is flexible, but I also prefer use crate::provider::foo; and use crate::provider::bar over use crate::internal_provider; and use crate::provider;.

[sffc]

Are you okay with provider/mod.rs and provider/helpers.rs?

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[sffc]

The merge directly from master is confusing, so I'm squashing and rebasing instead. ^ that was the squash. Rebase coming up next.

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• Cargo.lock is different
• components/datetime/Cargo.toml is different
• components/datetime/src/format.rs is different
• components/locale_canonicalizer/Cargo.toml is now changed in the branch
• components/locale_canonicalizer/src/lib.rs is now changed in the branch
• components/locale_canonicalizer/src/locale_canonicalizer.rs is now changed in the branch
• components/locale_canonicalizer/src/provider.rs is now changed in the branch
• components/provider_cldr/Cargo.toml is different
• components/provider_cldr/src/transform/likelysubtags.rs is now changed in the branch
• components/provider/Cargo.toml is different
• components/provider/src/resource.rs is different
• components/provider/src/structs/mod.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[sffc]

This is done except for the features test breakage.

The problem is that icu_locale_canonicalizer has an optional serde dependency, but that dependency requires that the icu_locid/serde feature is also enabled. As far as I can tell, it's not possible to make optional dependencies enable features of other dependencies.

With a normal feature (not an optional dependency), you can do:

[features]
foo = ["bar/baz"]

@Manishearth Suggestions?

[sffc]

I'm leaning toward adding a new feature that can pull in serde and whatever else is needed, and using this same feature name across all the ICU4X components.

What should we call the new feature?

• serialize
• deserialize
• serialize_deserialize
• icu4x_serde
• provider_serde
• enable_serde
• other?

[zbraniecki]

+1 for "serialize"

[Manishearth]

serialize seems fine to me. Not happy about having to do this, but all we can do is wait for namespaced features I guess

[codecov-io]

Codecov Report

Merging #451 (a8ef303) into master (f5915f1) will increase coverage by 0.18%.
The diff coverage is 31.57%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
+ Coverage   75.22%   75.41%   +0.18%     
==========================================
  Files         114      113       -1     
  Lines        6124     6068      -56     
==========================================
- Hits         4607     4576      -31     
+ Misses       1517     1492      -25     

Impacted Files	Coverage Δ
components/datetime/src/format.rs	100.00% <ø> (ø)
components/datetime/src/lib.rs	100.00% <ø> (ø)
components/datetime/src/provider/helpers.rs	67.02% <ø> (ø)
components/datetime/src/provider/mod.rs	0.00% <0.00%> (ø)
components/ecma402/src/pluralrules.rs	92.05% <ø> (ø)
components/locale_canonicalizer/src/lib.rs	100.00% <ø> (ø)
...s/locale_canonicalizer/src/locale_canonicalizer.rs	71.76% <ø> (ø)
components/locale_canonicalizer/src/provider.rs	0.00% <0.00%> (ø)
components/plurals/src/data.rs	79.54% <ø> (ø)
components/provider/src/erased.rs	47.61% <ø> (+11.65%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5915f1...a8ef303. Read the comment docs.

[sffc]

I went with provider_serde because I realized that we may want the data provider serialization to have a separate toggle switch from serialization in other parts of the library (such as LanguageIdentifier).

Looks like the tests are all passing, including the feature permutation test! This is ready for re-review, and I'm eager to check it in. I may wait to hit the merge button until tomorrow's meeting.

[echeran]

Looks good for provider_ppucd and uniset components, but of course not much needed coincidentally.

[zbraniecki]

lgtm! Thank you Shane!

[sffc]

Clippy seems to be failing due to the known clippy bug:

error: field assignment outside of initializer for an instance created with Default::default()
   --> components/provider_fs/src/bin/icu4x-cldr-export.rs:273:5
    |
273 |     options.root = output_path;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::field-reassign-with-default` implied by `-D warnings`
note: consider initializing the variable with `icu_provider_fs::export::fs_exporter::ExporterOptions { root: output_path, ..Default::default() }` and removing relevant reassignments
   --> components/provider_fs/src/bin/icu4x-cldr-export.rs:272:5
    |
272 |     let mut options = fs_exporter::ExporterOptions::default();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#field_reassign_with_default

So I believe this PR is ready to land as soon as I get the final approvals.

[sffc]

@filmil told me offline that he's unable to review this PR, so I will proceed to merge with the approvals from @zbraniecki, @nciric, and @echeran.