Add CLDR JSON data to testdata and remove from provider_cldr #513
Conversation
Codecov Report
@@ Coverage Diff @@
## master #513 +/- ##
==========================================
+ Coverage 73.35% 74.13% +0.78%
==========================================
Files 117 118 +1
Lines 6501 6500 -1
==========================================
+ Hits 4769 4819 +50
+ Misses 1732 1681 -51
Continue to review full report at Codecov.
|
| let json_str = std::fs::read_to_string("tests/testdata/likelySubtags.json").unwrap(); | ||
| let provider = LikelySubtagsProvider::try_from(json_str.as_str()).unwrap(); | ||
| let cldr_paths = crate::cldr_paths::for_test(); | ||
| let provider = LikelySubtagsProvider::try_from(&cldr_paths as &dyn CldrPaths).unwrap(); |
There was a problem hiding this comment.
do you need as here? I think it should auto-coerce, no?
There was a problem hiding this comment.
It seems Rust doesn't auto-convert concrete references like &CldrPathsAllInOne to dynamic references like &dyn CldrPaths. I want to change the function to take &impl (CldrPaths + ?Sized) instead of &dyn CldrPaths, but I can't do that in TryFrom; I tried removing TryFrom but that broke a bunch of stuff. I intend to follow up in the next PR, in the context of #442, to fix this.
| @@ -113,6 +126,10 @@ clap = { version = "2.33", optional = true } | |||
| icu_provider_cldr = { version = "0.1", path = "../../components/provider_cldr", optional = true, features = ["download"] } | |||
| log = { version = "0.4", optional = true } | |||
| simple_logger = { version = "1.11", optional = true } | |||
| globwalk = { version = "0.8", optional = true } | |||
There was a problem hiding this comment.
I know it's testdata, but glob crate is maintained by the Rust core team and has no dependencies. Could you use it?
There was a problem hiding this comment.
The README for globwalk lists 4 reasons why it exists, and 3 of the 4 apply to me:
- I need
{}syntax for locales - I need to give a list of multiple selectors, gitignore-style
- Support starting from a specified directory, not just current working directory rust-lang/glob#54, a fundamentally important feature request that has been open for almost 5 years
- Also, I like the API of
globwalkbetter
There was a problem hiding this comment.
Also, not only is this testdata, but this is an optional dependency for testdata that is downloaded only when gen-testdata is run. So the impact of this dependency is very minimal, to the extent that I didn't consider it when deciding to add it.
I made this change because:
FromStrcode path on the CLDR data providersI copied the data for all of our test locales. I could have limited it to only the locales we were using in provider_cldr tests, but I did all the locales because it allows the ICU4X JSON test data to be generated without hitting any remote server, which could allow us to add additional end-to-end tests on icu4x-cldr-export.