ipfs-rust / ipfs-embed

A small embeddable ipfs implementation
352 stars 48 forks source link

Library crate uses anyhow #114

Closed matthiasbeyer closed 1 year ago

matthiasbeyer commented 1 year ago

This is a library crate, but uses anyhow as error library.

This makes the library hardly usable.

Please switch your error library to thiserror.

dvc94ch commented 1 year ago

I understand it's a common opinion. Thanks for bringing it up

matthiasbeyer commented 1 year ago

Can you at least please share an explanation why it is not planned?

dvc94ch commented 1 year ago

Well the burden of proof is on your end. Telling me you have an opinion does not constitute an argument.

matthiasbeyer commented 1 year ago

https://github.com/dtolnay/anyhow#comparison-to-thiserror

dvc94ch commented 1 year ago

This is a library crate, but uses anyhow as error library.

True statement

This makes the library hardly usable.

False statement, I have plenty of experience using ipfs-embed and know for a fact that it's not true.

Please switch your error library to thiserror.

A request. Not sure if it's based on your false statement.

Not sure what people learn in school these days, but if you want a change made you need to present a valid logical argument.

matthiasbeyer commented 1 year ago

This makes the library hardly usable.

False statement, I have plenty of experience using ipfs-embed and know for a fact that it's not true.

You're right, my statement was inaccurate. More accurately would have been: "This makes the error type returned by the library less usable!". anyhow::Error hides any information about the error itself, thus takes away the ability to handle specific errors in application code accordingly. As an application author, I want to know what kind of error was raised, to implement handling of the error accordingly. Without any information about the actual error, I cannot do that.

Thus, I want to ask whether it would be possible that you switch the error type implementation to something using thiserror.


That said, I think I should apologize for the way I conversed previously. My statements had (as I now understood) too few context information and were to brief to be understood as I meant them and probably came over as snarky or unpolite. That was not my intention, sorry!

dvc94ch commented 1 year ago

Can you provide more information about which error you'd like to handle in a special way? See here [0] for an example of how to handle the block not found error.

matthiasbeyer commented 1 year ago

Well first of all, finding that error type (BlockNotFound) was not possible for me. I just didn't know that it existed, because the docs do not show it. The docs only show SigningError and EncodingError, no other error types.

Second, handling errors like you mentioned above does only work if there is actually an error type. And then it will get ugly rather quickly:

match some_function() {
    Err(e) => {
        return Err(e.downcast_ref::<BlockNotFound>()
            .map(MyErrorType::from)
            .or_else(|| e.downcast_ref::<SomeOtherError>().map(MyErrorType::from))
            .or_else(|| e.downcast_ref::<EvenOtherError>().map(MyErrorType::from)));
    }
    // ...
}

That is only possible if there is actually something to downcast to. If you're just using the anyhow::anyhow!() macro internally, there's nothing a user of ipfs-embed can do to find out what the actual error was ... except maybe matching strings, which is error prone and cannot be sufficiently checked by the compiler!

If there's an error enum, or some other form of typed errors, the above would be as simple as:

some_function().map_err(MyErrorType::from)?;

(That map_err could actually be removed as well, because ? does the conversion most of the time just like this) And the conversion is still typed, all the error information from the errors below are preserved and can be used if needed.

If there's more error handling involved than just "bubbling up" the error, it would be something like this:

match some_function() {
    Err(ipfs_embed::Error::BlockNotFound) => handle_block_not_found(),
    Err(ipfs_embed::Error::SomeOtherError) => handle_some_other_err(),
    // ... and so on
}

To answer your question more directly: I would like to handle all errors in a special way! Right now I don't even know which errors exist without browsing the source of the whole crate and even if I did, I would need to re-do this every time you ship an update, because the compiler cannot tell me anything except that there's a String!

dvc94ch commented 1 year ago

Well first of all, finding that error type (BlockNotFound) was not possible for me. I just didn't know that it existed, because the docs do not show it. The docs only show SigningError and EncodingError, no other error types.

As to your first point, you'll know you need to handle it when you encounter it. In general I'd say it's better to return a Result<Option<Block>>, but experience showed that in this case an explicit error type was more convenient.

That is only possible if there is actually something to downcast to.

true and that's why I generally think errors should be structs that implement Debug Display and Error. But again making a true statement that is completely irrelevant doesn't strengthen your argument. the sky is blue and grass is green and fire is hot.

I would like to handle all errors in a special way!

can you link me to some code where you handled all errors in a special way? so I can see this special error handling? In all my years writing rust I've never seen anything like this.

one problem with your suggested enum is where do you suggest storing the backtrace.

matthiasbeyer commented 1 year ago

In general I'd say it's better to return a Result<Option>, but experience showed that in this case an explicit error type was more convenient.

Yes, explicit error types is what I am asking for here! But right now there is none, as anyhow::Result (or rather anyhow::Error) hides the explicit error type!

I generally think errors should be structs that implement Debug Display and Error.

I totally agree! Debug can be derived and Display and Error would also be implemented in my suggestion, as the thiserror crates does this appropriately.

In all my years writing rust I've never seen anything like [special error handling]

So you never saw code that handled errors? You never wrote code that could recover from errors but just crashed into the users face even though it might be able to recover? I doubt that, given you claim to have years of experience with Rust!

one problem with your suggested enum is where do you suggest storing the backtrace.

You store it within the enum of course (Ctrl-F Backtrace)!

dvc94ch commented 1 year ago

Yes, explicit error types is what I am asking for here! But right now there is none, as anyhow::Result (or rather anyhow::Error) hides the explicit error type!

I know what you're asking. And I know that it has more to do with you than with ipfs-embed or good error handling than anything else.

So you never saw code that handled errors? You never wrote code that could recover from errors but just crashed into the users face even though it might be able to recover? I doubt that, given you claim to have years of experience with Rust!

now you're being disingenuous. show me any open source project that has this special error recovery. errors are logged and ignored or maybe retried a few times. but don't pretend like you can do anything else to handle transient or fatal errors.

You store it within the enum of course (Ctrl-F Backtrace)!

I know it's possible. show me one project that actually does this.

dvc94ch commented 1 year ago

It is very clear to me that you can do poor error handling with anyhow and with thiserror. I don't think that using thiserror magically makes things better. I like thiserror to derive display, it's quite convenient. Most people virtue signaling and claiming thiserror to be a magically superior solution while clearly lacking the ability to defend their statement in a coherent way are just a waste of my time.