Skip to content
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

Where to put DataProvider load_payload method #433

Closed
sffc opened this issue Dec 23, 2020 · 2 comments · Fixed by #431
Closed

Where to put DataProvider load_payload method #433

sffc opened this issue Dec 23, 2020 · 2 comments · Fixed by #431
Labels
A-design Area: Architecture or design C-meta Component: Relating to ICU4X as a whole

Comments

@sffc
Copy link
Member

sffc commented Dec 23, 2020

Implementors of the DataProvider trait need to implement load_to_receiver. However, load_payload is easier to think about and use. I currently implement load_payload via impl dyn, like this:

/// An abstract data provider that loads a payload given a request object.
pub trait DataProvider<'d> {
    /// Query the provider for data, loading it into a DataReceiver.
    ///
    /// Returns Ok if the request successfully loaded data. If data failed to load, returns an
    /// Error with more information.
    fn load_to_receiver(
        &self,
        req: &DataRequest,
        receiver: &mut dyn DataReceiver<'d, 'static>,
    ) -> Result<DataResponse, Error>;
}

/// A response object containing an object as payload and metadata about it.
pub struct DataResponseWithPayload<'d, T>
where
    T: Clone + Debug,
{
    /// Metadata about the returned object.
    pub response: DataResponse,

    /// The object itself; None if it was not loaded.
    pub payload: Option<Cow<'d, T>>,
}

impl<'d> dyn DataProvider<'d> + 'd {
    /// Query the provider for data, returning the result.
    ///
    /// Returns Ok if the request successfully loaded data. If data failed to load, returns an
    /// Error with more information.
    pub fn load_payload<T>(
        &self,
        req: &DataRequest,
    ) -> Result<DataResponseWithPayload<'d, T>, Error>
    where
        T: serde::Deserialize<'static> + erased_serde::Serialize + Any + Clone + Debug,
    {
        let mut receiver = DataReceiverForType::<T>::new();
        let response = self.load_to_receiver(req, &mut receiver)?;
        Ok(DataResponseWithPayload {
            response,
            payload: receiver.payload,
        })
    }
}

However, I could instead implement it in a few other ways:

  1. Function on DataProvider with a default implementation. I think this won't work, because DataProvider is trait object safe.
  2. New trait, like DataProviderPlusPlus, with a impl<S> DataProviderPlusPlus for S where S: DataProvider
  3. Standalone function, called like icu_provider::load_payload::<T>(&provider, &req)

I would ideally like to reach a decision that applies not only to this specific case of load_payload, but also more generally when we have low-level traits intended to be implemented paired with high-level traits intended to be used. I have a few other examples where this has come up already.

CC @Manishearth

@sffc sffc added C-meta Component: Relating to ICU4X as a whole A-design Area: Architecture or design discuss Discuss at a future ICU4X-SC meeting labels Dec 23, 2020
@sffc
Copy link
Member Author

sffc commented Dec 23, 2020

Closely related: #422

@Manishearth
Copy link
Member

I'm not really in favor of impl dyn, it means the method is only available on the trait object.

I really like impl TraitExt for T where T: Trait + ?Sized. It is something people have to import, but the TraitExt pattern is reasonably idiomatic rust.

I wish Rust had a way to just .. imply traits this way so that they don't need to be imported.

Standalone functions are also fine, but I prefer methods when possible.

@sffc sffc closed this as completed in #431 Jan 5, 2021
@sffc sffc removed the discuss Discuss at a future ICU4X-SC meeting label Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-design Area: Architecture or design C-meta Component: Relating to ICU4X as a whole
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants