Designing accessors: Option<Result<T, E>> or Result<T, E>?

help
1

Hi!
I'm designing a data structure to represent CloudEvents in the new official cloudevents/sdk-rust library.

An Event could contain or not a payload, called data. This data field could have different internal representations, depending on how it comes from the wire:

pub enum Data {
    String(String),
    Binary(Vec<u8>),
    Json(serde_json::Value),
}

While accessing to data, the accessor converts it to the desired type using a TryFrom<Data> impl:

impl TryFrom<Data> for serde_json::Value {
    type Error = serde_json::Error;

    fn try_from(value: Data) -> Result<Self, Self::Error> {
        match value {
            Data::String(s) => Ok(serde_json::from_str(&s)?),
            Data::Binary(v) => Ok(serde_json::from_slice(&v)?),
            Data::Json(v) => Ok(v),
        }
    }
}

Now, the accessor now looks like:

impl Event {
    pub fn try_get_data<T: Sized + TryFrom<Data, Error = E>, E: std::error::Error>(
        &self,
    ) -> Option<Result<T, E>> {
        match self.data.as_ref() {
            Some(d) => Some(T::try_from(d.clone())),
            None => None,
        }
    }
}

Do you think it would be better, from a usability standpoint of view, to have this try_get_data return a Result<T, E>? In that case, E would be custom defined like:

enum DataError {
    NoData,
    ConversionError(Box<dyn std::error::Error>),
}
2

If there's a case where it simply returns no data, then I would return a Result<Option<T>, E>

2 Likes
3

My argument against that is: if there is no data, there is no need to conversion. The Result makes sense only when i try to convert i think...

4

I mean, if I wanted to use the function, I would probably want to bubble the error up, which I can do with the question mark, but if there is no data, I would probably want to handle that specially. Compare these:

// Result<Option<T>, E>
if let Some(item) = event.try_get_data()? {
    ...
}

// Option<Result<T, E>>
if let Some(fallible) = event.try_get_data() {
    let item = fallible?;
    ...
}

It also makes more semantic sense. The output of the function is Option<T> because it might not exist. The result should wrap the output and allow it to return an error.

6 Likes
5

Note that you can implement this rather easily with transpose

impl Event {
    pub fn try_get_data<T: Sized + TryFrom<Data>>(
        &self,
    ) -> Result<Option<T>, T::Error> {
        self.data.as_ref().map(|d| T::try_from(d.clone())).transpose()
    }
}

You also do not need the generic paramter E as you can use the T::Error syntax instead.

6

I'll go for Result<Option, E>, thanks!

7

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.