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

Best practice for "catch all" error? #52

Open
vmalloc opened this issue Dec 3, 2019 · 2 comments

Comments

@vmalloc
Copy link

@vmalloc vmalloc commented Dec 3, 2019

I'd like to use a single Error type for various modules inside my crate, and occasionally the need arises to propagate errors from other projects using ? for ergonomics.

It seems intuitive to just include a

    #[error("Internal Error")]
    InternalError {
        #[from]
        e: anyhow::Error,
    },

in my Error enum, but the problem is that some errors require an additional conversion into anyhow::Error, for example:

fn load_records(db: DB) -> Result<Vec<Record>, MyError> {
    db.get_connection()?.load_records(...)?
}

In the above example I'd like the possible errors from diesel/r2d2/what not to map to my InternalError variant. The problem is there is no automatic conversion from those into MyError, since it requires an additional conversion to anyhow::Error...

What is the idiomatic/recommended way to deal with these cases?

Thanks in advance

@dtolnay

This comment has been minimized.

Copy link
Owner

@dtolnay dtolnay commented Dec 3, 2019

Thanks for asking! The style I would recommend is:

pub enum Error {
    ...

    #[error(transparent)]
    Other(#[from] anyhow::Error),
}

using the #[error(transparent)] attribute from https://github.com/dtolnay/thiserror/releases/tag/1.0.7. That will forward the Display and Error::source implementations directly to the anyhow::Error so that your internal error doesn't appear as a redundant layer in the cause chain. So instead of:

Error: failed to do thing that the caller wanted

Caused by:
    - internal error
    - timed out waiting for connection

they would instead receive:

Error: failed to do thing that the caller wanted

Caused by:
    timed out waiting for connection

For ?-conversions you can either write the impls:

impl From<r2d2::Error> for Error {
    fn from(e: r2d2::Error) -> Self {
        Error::Other(anyhow::Error::new(e))
    }
}

or else use map_err to make the anyhow::Error:

use anyhow::Error as OtherError;

let conn = db.get_connection().map_err(OtherError::new)?;
let records = conn.load_records(...)?.map_err(OtherError::new)?;
@vmalloc

This comment has been minimized.

Copy link
Author

@vmalloc vmalloc commented Dec 4, 2019

Thanks for the reply!

Yeah what you wrote is what I ended up doing, but having so many cases drain into the "internal error" case makes this very repetitive and I ended up creating a macro for it, something along the lines of:

macro_rules! make_internal_error {
    ($errtype:path) => {
        impl std::convert::From<$errtype> for MyError {
            fn from(e: $errtype) -> MyError {
                anyhow::Error::from(e).into()
            }
        }
    };
}

I feel like this should have been a convenience utility inside thiserror, as mapping errors must be a pretty common case...

In any case many thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.