Refactoring icu-data-provider crate. #198
Conversation
Pull Request Test Coverage Report for Build fe8b6a6d70cd30cc22ea6276137d06646f1a2070-PR-198
💛 - Coveralls |
sffc
force-pushed
the
sffc-dp1
branch
2 times, most recently
from
b20611b to
8206f63
Compare
|
Not sure why macOS is failing. I investigated but can't find the cause. I've re-run it a few times, and sometimes it fails in data-provider, and sometimes in locale. See #201 |
|
The macOS test passed at least once in this PR: https://github.com/unicode-org/icu4x/actions/runs/198964202 The code version on that passing run (b20611b) is identical to the current head of this branch (ed79fef): https://github.com/unicode-org/icu4x/compare/b20611b..ed79fef |
There was a problem hiding this comment.
lgtm!
I also pinged @Manishearth online to get additional eyes on the DataRequest/DataResponse pair and he responded with:
manish: it seems okay, but might be nice to have a more performant set of APIs that aren't using trait objects
zibi: what would they use?
manish: yeah i'm also not sure if it's possible
but you could have a direct API that'sget_response<T>(Request) -> Result<T>
load<T>(&self, req: &DataRequest) -> Result<T, DataError>
and use that to write one that uses trait objects
I don't think this should be blocking, but putting it here for your consideration Shane. Maybe a follow-up issue?
| Some(variant.clone()) | ||
| } else { | ||
| None | ||
| }, |
There was a problem hiding this comment.
could you use &data_entry.variant.cloned() ?
There was a problem hiding this comment.
What is cloned() a method on? Here's the error when I make this change:
error[E0599]: no method named `cloned` found for enum `std::option::Option<std::borrow::Cow<'static, str>>` in the current scope
--> components/data-provider/src/data_entry.rs:81:37
|
81 | &data_entry.variant.cloned(),
| ^^^^^^ method not found in `std::option::Option<std::borrow::Cow<'static, str>>`
|
::: /home/sffc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/option.rs:156:1
|
156 | pub enum Option<T> {
| ------------------ doesn't satisfy `_: std::iter::Iterator`
|
= note: the method `cloned` exists but the following trait bounds were not satisfied:
`std::option::Option<std::borrow::Cow<'static, str>>: std::iter::Iterator`
which is required by `&mut std::option::Option<std::borrow::Cow<'static, str>>: std::iter::Iterator`
There was a problem hiding this comment.
I think they meant clone().
components: [
// Does not actually clone if the variant is borrowed
data_entry.variant.clone(),
Some(Cow::Owned(data_entry.langid.to_string())),
],
| Some(variant.clone()) | ||
| } else { | ||
| None | ||
| }, |
There was a problem hiding this comment.
What is cloned() a method on? Here's the error when I make this change:
error[E0599]: no method named `cloned` found for enum `std::option::Option<std::borrow::Cow<'static, str>>` in the current scope
--> components/data-provider/src/data_entry.rs:81:37
|
81 | &data_entry.variant.cloned(),
| ^^^^^^ method not found in `std::option::Option<std::borrow::Cow<'static, str>>`
|
::: /home/sffc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/option.rs:156:1
|
156 | pub enum Option<T> {
| ------------------ doesn't satisfy `_: std::iter::Iterator`
|
= note: the method `cloned` exists but the following trait bounds were not satisfied:
`std::option::Option<std::borrow::Cow<'static, str>>: std::iter::Iterator`
which is required by `&mut std::option::Option<std::borrow::Cow<'static, str>>: std::iter::Iterator`
Key changes in this PR:
Note: I added a dependency on erased_serde. I need it for implementing the DataExporter trait. Unfortunately that crate is not at v1 yet. I could put DataExporter (and IterableDataProvider) behind a feature, but I will need to sprinkle the code with a lot of
#[cfg]s, since the core traits are pulling in the erased_serde symbols.