Closed nordzilla closed 2 years ago
@zbraniecki
Thank you for taking a look and approving even though this was in draft.
I believe this is fully ready for review now.
I do want to ask specifically about the changes I made to the Cargo.toml
files. I largely changed these to refer to paths within this crate workspace, but some of them were previously pulling exact versions from crates.io.
Is there a reason for this? I feel like crates in this workspace should largely use the latest versions of each other unless there is a good reason not to. But I don't want to make this change lightly.
@zbraniecki
I have added an explainer section as you requested in https://github.com/mozilla/l10nregistry-rs/pull/19#pullrequestreview-825433903
I also explored the option of using impl<S: Into<String>> From<(S, ResourceType)> for ResourceId
instead of a ToResouceId
trait, and I'm fairly certain that I like the ToResourceId
trait more for the following reasons:
These two blanket implementations cannot coexist:
impl<S: Into<String>> From<S> for ResouceId { ... }
impl<S: Into<String>> From<(S, ResourceType)> for ResourceId { ... }
Because someone else could implement
struct NewType {}
impl Fom<NewType> for String { ... }
impl From<(NewType, ResourceType)> for String { ... }
And this would create an ambiguous cycle with the above two implementations.
Lastly, regarding
"One major bit that I'd like to clarify, is that you added a
ToResourceId
trait, but you don't generalize public method callers to be over<R: ToResoureID>(res_ids: Vec<R>)
. Why?"
I did respond to this in the original comment, but I also want to respond here. Generalizing the public trait methods over a generic parameter would make the trait no longer object-safe, which I believe to be a big decision that I would prefer to make later only if needed.
The current expecation that types are converted to ResourceId
explicitly is forward-compatible if we decide to generalize over the trait at a later time, because of the blanket implementation. Since the blanket implementation impl<T> From<T> for T
exists, then any old code that explicitly converts to ResourceId
before passing it in will still work.
I'm getting approvals on the Gecko side of things, and I believe that I am ready to start publishing the crates and doing the vendoring.
ResourceId
.ResourceID
s can be optional or required.ResourceId
instead ofString
.