bIgBV/tokio-beanstalkd

Rework API to not include double result

bIgBV opened this issue · 6 comments

bIgBV commented

@jonhoo 's great PR (#10) moves the library to use std::Future, but this also means that it exposes the issues with the API. The PR shows how all the methods now require a doube unwrap in order to surface the two types of errors.

One way to handle this is to collapse the two error types into a single hierarchical error type and expose that, simplifying the API by a ton.

I think one solution here is to merge the PR, and then do a follow-up PR that cleans up the types further. And just don't do a release of the intermediate commit.

How would you imagine this looking? Should a new top level Error enum be introduced?

pub enum Error {
    Beanstalk(BeanstalkError),
    Put(Put),
    Consumer(Consumer),
}

Or perhaps BeanstalkError should be extended:

#[derive(Copy, Clone, Debug, Eq, PartialEq, Fail)]
pub enum BeanstalkError {
    /// The client sent a command line that was not well-formed. This can happen if the line does not
    /// end with \r\n, if non-numeric characters occur where an integer is expected, if the wrong
    /// number of arguments are present, or if the command line is mal-formed in any other way.
    ///
    /// This should not happen, if it does please file an issue.
    #[fail(display = "Client command was not well formatted")]
    BadFormat,

    // ... snip ...

    Put(Put),
    Consumer(Consumer),
}

EDIT: Also, should the proto errors go away and the main error class be returned directly?

I think the wrapping should go the other way. So:

pub enum PutError {
    TransportError(#[cause] BeanstalkError)

    // ... various Put-specific errors
}

Ah interesting, cool.

I made a late edit to my last comment, what are your thoughts for that?

Also, should the proto errors go away and the main error class be returned directly?

The reason I'd want them to go that way is so that you still only have to match on relevant errors for any given method if it errors. Rather than having to match away, say, consumer errors when your put errors.

Not sure about your follow-up. I don't know if it matters? Seems fine for them to be folded into BeanstalkError, but either way works.

Good point, thanks for the follow up.