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 error-chain for Main Error Type #149

Closed
KodrAus opened this issue May 14, 2017 · 8 comments
Closed

Use error-chain for Main Error Type #149

KodrAus opened this issue May 14, 2017 · 8 comments

Comments

@KodrAus
Copy link
Contributor

KodrAus commented May 14, 2017

Can we make tantivy's error handling better using error-chain?

error-chain gives us better backtraces thanks to backtrace, which would be great given tantivy is complex and asynchronous, but it imposes a particular open-book structure on your error type.

Currently we've got small, specialised error types in submodules that are collected into this main user-facing Error. The idea is to use error-chain for the main user-facing tantivy::Error type and keep the specialised errors as just plain std::Errors. I guess we'd chain_err those?

Does anyone have any other thoughts/recommendations/considerations on how we could hold error-chain or something else to keep tantivys error handling idiomatic and useful? Any input would be really appreciated! I've got a quick spike here that could be a good starting point for discussion.

cc: @fulmicoton @brson @BurntSushi

@fulmicoton
Copy link
Collaborator

One of the main concern is that understanding tantivy's Error type will not be accessible by
editor's go to definition nor will it be visible in the rust doc.

@BurntSushi
Copy link

My general opinion is that the problems that error-chain solves are important, but that it's too much magic for my blood. Two things to note about myself:

  1. I have a very high tolerance for code that is annoying to write, but a low tolerance for code that is hard to read.
  2. I'm probably being a bit cranky.

@fulmicoton
Copy link
Collaborator

I empathize with 1... Especially for a libraries. Library source code eventually tends to be more Write Once Read Many than higher level application in my experience.

@KodrAus
Copy link
Contributor Author

KodrAus commented May 14, 2017

In addition to this I think we could add more logging to tantivy so you can get a lot of useful debug info in verbose. There's a ticket here somewhere about slog.

If error-chain is a bit too much magic then we can use backtrace directly and tack it on to our error types like error-chain does. Reinventing it seems like a weird solution though, but would give us the flexibility to make Error a black box sometime in the future.

Alternatively if out-of-the-box logging is going to have all this detail then we could rely on that and leave the errors mostly as they are. We could have a bail! macro of our own that grabs a backtrace if configured and tacks it on to the log.

Thoughts?

@fulmicoton
Copy link
Collaborator

@KodrAus quick update... Let's give it one more week of thoughts.

@fulmicoton
Copy link
Collaborator

@KeenS corrected me. The errorkind not showing in the rustdoc was just a matter of it being publicly exposed.
I think it might be a good idea to use error-chain for tantivy's toplevel error.

@KodrAus
Copy link
Contributor Author

KodrAus commented May 22, 2017

Ok, I'll get the error-chain branch in a reviewable state and open a PR 👍

@KodrAus
Copy link
Contributor Author

KodrAus commented May 29, 2017

Closed by #177. The main Error type is now using error-chain and the directory errors implement std::error::Error.

@KodrAus KodrAus closed this as completed May 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants