rust-client icon indicating copy to clipboard operation
rust-client copied to clipboard

Use a more fitting error type

Open llogiq opened this issue 2 years ago • 5 comments

Currently this crate uses anyhow, which is usually meant for applications, not libraries. This makes it very hard to match on an error, because the error is essentially opaque. As far as I can see, we mostly return ´tonic::Status`, but there may be some APIs that also can return other errors.

The classic way of dealing with this is create a newtype that wraps Status, has a From<Status> impl and allows basically the same functionality (especially matching on Code). We could also create an enum instead, but I should note that this will either make the implementation, the user interface or both needlessly complex. If we have API calls that can return other errors, either have them return their own specific error type or convert whatever error we get to our predefined error type internally.

Another way would be to make the error type opaque, but that has the downside of making it harder for users to implement From<QdrantError> for their own error types, essentially requiring a Box or other indirection.

llogiq avatar Aug 02 '23 15:08 llogiq

Is this one already taken?

nkbelov avatar Aug 04 '23 13:08 nkbelov

I believe @ffuugoo is preparing work on this. We're having some internal conversions on what the final error type would look like.

timvisee avatar Aug 04 '23 15:08 timvisee

I think the most elegant way to address it is to use thiserror for errors inside a library. Such errors would be compatible with anyhow and alternatives (like eyre etc).

daniel-abramov avatar Mar 18 '24 20:03 daniel-abramov

I don't see the great benefit of thiserror compared to a plain declarative macro, and the latter certainly compiles faster.

Btw. it's often a good idea to have specific error types for classes of operations, or even per method, so you don't get variants that don't ever exist. Makes matching easier.

llogiq avatar Mar 20 '24 09:03 llogiq

I meant that thiserror is a typical choice for libraries to use instead of anyhow. I think the issue with anyhow is not only that the error is opaque but also that it makes it hard to use a library if the app does not use anyhow e.g. if my app uses eyre I can't really call functions from qdrant client and use ? on them. Instead, I need to do something like .map_err(|err| eyre!(err))? which gets a bit cumbersome.

daniel-abramov avatar Mar 20 '24 10:03 daniel-abramov