Read/write graph nodes with error handling

code review
1

I'm trying to make the API for a graph data crate. I'm a Rust beginner and would appreciate any feedback. You can read a graph by providing a function that accepts graph::Result<&Tray> and returns graph::Result<T>. The graph could fail and the user must handle the error in their read function. I am asking for a function/closure because I want the graph to do some things after a read or write.

I feel it might be unnecessarily restrictive to force the use of graph::Result for the output of the user-defined read function (check_text in this case). Or is this general okay or expected in this scenario? Should I find a way to allow the user to return their own Result type or no result at all?

Feel free to point out other problems with this setup.

Here I test reading a leaf node:

use super::*;

const TEXT: &str = "Turtles are precious.";

fn check_text(tray: graph::Result<&Tray>) -> graph::Result<()> {
    match tray? {
        Tray::String(value) => assert_eq!(value, TEXT),
        tray => Err(tray.wrong_variant("String"))?,
    }
    Ok(())
}

#[test]
fn read_leaf() -> graph::Result<()> {
    let leaf = TEXT.leaf();
    leaf.read(check_text)?;
    Ok(())
}

The graph Result and Error types (work in progress based on thiserror and anyhow crates):

/// Graph Result
pub type Result<T> = std::result::Result<T, Error>;

/// Graph Error
#[derive(Error, Debug)]
pub enum Error {
    #[error("read graph part failed ({0})")]
    Read(String),
    #[error("write graph part failed ({0})")]
    Write(String),
    #[error(transparent)]
    Adapt(#[from] adapt::Error),
    #[error(transparent)]
    Solve(#[from] solve::Error),
    #[error(transparent)]
    Apex(#[from] apex::Error),
    #[error(transparent)]
    Io(#[from] std::io::Error),
    #[error(transparent)]
    SerdeJson(#[from] serde_json::Error),
    #[error(transparent)]
    Any(#[from] anyhow::Error),
}
2

It's pretty unusual to require that a Result be passed as an input. What's the point of that? You unwrap-or-bubble it immediately using the ? operator anyway. You should just accept a &Tray.

1 Like
3

Does the code after calling the closure or function depend on its outcome? If so, you’ll need a defined way to communicate these dependencies. A Result might be a good solution. However, I would strongly recommend using std::result::Result in this case.

Personally, I prefer a generic return type for a callback, allowing the callback to do whatever it needs to do, without restrictions:

fn use_callback<T>(&mut self, callback: impl FnOnce(Self) -> T) -> T {
    self.do_something_before();
    let result = callback(self);
    self.do_something_after();
    result
}

But if your code does depend on the outcome of the callback, something like this should work well.

fn use_callback<R, E>(&mut self, callback: impl FnOnce(Self) -> Result<R, E>) -> Result<R,E> {
    self.do_something_before();
    let result = callback(self);
    if result.is_ok() {
        self.do_something_after();
    } else {
        self.do_something_different();
    }
    result
}
1 Like
4

I love the idea of just providing &Tray. To do so, I would have to not call the callback if &Tray is not available. That does feel like the better way.

5

I think your first snippet is what I want. So my graph part would implement this trait to match what you are saying except notice that I do need the read function to return graph::Result<T> because the graph might fail. But the callback FnOnce(&Self::Payload) -> T can simply return T. If Self::Payload (Leaf Payload is Tray) is not available due to some error. The callback will not be called and the read function would return an error. I think this fix lines up with what has been suggested.

pub trait Read {
    type Payload;
    /// Read the payload of the graph part.
    fn read<T, F>(&self, read: F) -> graph::Result<T>
    where
        F: FnOnce(&Self::Payload) -> T;
}
6

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.