-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use DataProvider without AnyProvider in tutorial example #3948
Conversation
docs/tutorials/data_provider.md
Outdated
@@ -269,31 +269,41 @@ use tinystr::tinystr; | |||
|
|||
pub struct CustomDecimalSymbolsProvider<P>(P); | |||
|
|||
impl<P> AnyProvider for CustomDecimalSymbolsProvider<P> | |||
fn transform(any_res: AnyResponse) -> Result<AnyResponse, DataError> { | |||
let mut dec_res: DataResponse<DecimalSymbolsV1Marker> = any_res.downcast()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that for some reason when I had this function inlined below, I got the following compile error:
error[E0271]: type mismatch resolving `<DecimalSymbolsV1Marker as DataMarker>::Yokeable == <M as DataMarker>::Yokeable`
--> src/lib.rs:301:77
|
37 | let mut dec_res: DataResponse<DecimalSymbolsV1Marker> = any_res.downcast()?;
| ^^^^^^^^ expected `DecimalSymbolsV1<'_>`, found associated type
|
= note: expected struct `DecimalSymbolsV1<'static>`
found associated type `<M as DataMarker>::Yokeable`
= help: consider constraining the associated type `<M as DataMarker>::Yokeable` to `DecimalSymbolsV1<'static>`
= note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
The whole point of going through AnyResponse
is to work around that exact type of compile problem since we don't have specialization.
Moving the code into a helper function fixes the error. Compiler bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a fix. You need to be explicit about what you're downcasting to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, still seems like a bug that let foo: Foo<Bar> = foo()?
would resolve differently than let foo = foo::<Bar>()
with the signature fn foo<T>() -> Result<Foo<T>>
docs/tutorials/data_provider.md
Outdated
@@ -269,31 +269,41 @@ use tinystr::tinystr; | |||
|
|||
pub struct CustomDecimalSymbolsProvider<P>(P); | |||
|
|||
impl<P> AnyProvider for CustomDecimalSymbolsProvider<P> | |||
fn transform(any_res: AnyResponse) -> Result<AnyResponse, DataError> { | |||
let mut dec_res: DataResponse<DecimalSymbolsV1Marker> = any_res.downcast()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a fix. You need to be explicit about what you're downcasting to.
docs/tutorials/data_provider.md
Outdated
let mut res = self.0.load(req)?; | ||
if M::KEY == DecimalSymbolsV1Marker::KEY && req.locale.region() == Some(region!("CH")) { | ||
// Cast from `DataPayload<M>` to `DataPayload<DecimalSymbolsV1Marker>` | ||
let mut dec_res = res.wrap_into_any_response().downcast::<DecimalSymbolsV1Marker>()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a try_cast
to DataPayload
that does this fallible casting through Any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, something along those lines. Maybe something that returns a &mut. But still, it's handy that we can do this with existing 1.2 API!
docs/tutorials/data_provider.md
Outdated
M::Yokeable: icu_provider::MaybeSendSync + zerofrom::ZeroFrom<'static, M::Yokeable>, | ||
for<'a> yoke::trait_hack::YokeTraitHack<<M::Yokeable as yoke::Yokeable<'a>>::Output>: Clone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if these bounds where on KeyedDataMarker::Yokeable
. We require them to be Send
, Sync
, ZeroFrom
and Clone
implicitly anyway, and they all are, so we shouldn't need to repeat this.
There's a better way to do this that we've been missing. #3952 |
// Cast from `DataPayload<M>` to `DataPayload<DecimalSymbolsV1Marker>` | ||
let mut any_payload = generic_payload as &mut dyn Any; | ||
if let Some(mut decimal_payload) = any_payload.downcast_mut::<DataPayload<DecimalSymbolsV1Marker>>() { | ||
if req.locale.region() == Some(region!("CH")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put the CH
check at the top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep the region nearer the payload override for readability. I don't have reason to believe that there's a performance difference between checking the TypeId of the payload versus the region; maybe there's a small difference but this code needs to exist either way.
Do we like this better or worse?
This is one of the main use cases for AnyProvider. If we're okay with users making this type of blanket impl and saying that
_unstable
constructors are safe with blanket impls, then it's a step toward phasing out AnyProvider.See #3947