Add ResourceKey attributes and LocaleFallbackProvider #2115
Conversation
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
|
I cleaned up the commit history so that it is reviewable commit-by-commit if you prefer. |
provider/core/src/data_provider.rs
Outdated
| @@ -37,6 +37,20 @@ impl fmt::Display for DataRequest { | |||
| } | |||
| } | |||
|
|
|||
| // XXX: DataRequest should technically not implement `Borrow<ResourceOptions>` since it has | |||
There was a problem hiding this comment.
@Manishearth I did this because I need a &DataRequest to pass down to the inner provider in the fallback provider. I can't pass ownership of the inner ResourceOptions to the iterator, because I need to build a DataRequest out of it at each step, and I can't have a local DataRequest and hand a &mut ResourceOptions to the iterator because I can't immutably borrow the DataRequest while one of its fields is on mutable loan. BorrowMut has the signature that I need, but according to docs.rs, it is not supposed to be used for borrowing a specific field, due to restrictions on Ord behavior.
Suggestions?
There was a problem hiding this comment.
We're misusing BorrowMut there already i think, we should be using AsMut
(that's what AsRefMut is called, it's confusing, legacy stuff, sorry)
There are docs tests in this PR that are effectively end-to-end. |
| while !fallback_iterator.get().options.is_empty() { | ||
| let result = self.inner.load_any(key, fallback_iterator.get()); | ||
| if !result_is_err_missing_resource_options(&result) { | ||
| return result; |
There was a problem hiding this comment.
Do we actually want to return data errors with the fallback_iterator.get() options set, or do we want to hide that from the client?
There was a problem hiding this comment.
I'm catching MissingResourceOptions because that's the soft error that we can recover from. If the error is something else more severe, like a Serde deserialization error or a type mismatch error, then we should propagate it. In languages with try/catch statements, you only catch the error you want to handle, and that's what I'm doing here
There was a problem hiding this comment.
I understand. But any error from the underlying provider that you don't catch will have the modified request as it's context. This might be confusing, so I propose that you also add the original request:
return result.map_err(|e| e.with_req(key, &base_req));
|
Notes for why I think we should keep the metadata in the string:
|
Without incrementing the version number, but because the path and hash change this will amount to the same. If we keep the metadata out of the path we can change it in code and reuse the same data. |
I'm saying that the data files may actually need to change and we can't re-use the same ones. For example, CLDR JSON pre-resolves data into all locales it supports, like en-GB. However, en-GB data may change when the fallback is changed. It would be fraught to treat the same data path as supporting both language and region fallback at the same time. |
| impl AsMut<ResourceOptions> for ResourceOptions { | ||
| fn as_mut(&mut self) -> &mut ResourceOptions { | ||
| self | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Curious what this is needed for?
There was a problem hiding this comment.
There are bounds in adapters/src/fallback/mod.rs that use AsMut<ResourceOptions> (rather than &mut ResourceOptions), and for whatever reason AsMut<T> isn't blanket-impled for all T
There was a problem hiding this comment.
Also see the conversation in #2115 (comment)
Nearing completion on #1109