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

Use a more fitting error type #64

Open
llogiq opened this issue Aug 2, 2023 · 5 comments
Open

Use a more fitting error type #64

llogiq opened this issue Aug 2, 2023 · 5 comments

Comments

@llogiq
Copy link
Contributor

llogiq commented Aug 2, 2023

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.

@nkbelov
Copy link
Contributor

nkbelov commented Aug 4, 2023

Is this one already taken?

@timvisee
Copy link
Member

timvisee commented Aug 4, 2023

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

@daniel-abramov
Copy link

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).

@llogiq
Copy link
Contributor Author

llogiq commented Mar 20, 2024

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.

@daniel-abramov
Copy link

daniel-abramov commented Mar 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants