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.

Sign up for GitHub

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

Jump to bottom

Move to thiserror for errors #115

Closed
cpcloud opened this issue Mar 28, 2020 · 5 comments · Fixed by #135
Closed

Move to thiserror for errors #115

cpcloud opened this issue Mar 28, 2020 · 5 comments · Fixed by #135
Assignees
@poros
Labels
enhancement

Comments

@cpcloud
Copy link
Contributor

cpcloud commented Mar 28, 2020

failure is leaking into the public API and forcing downstream consumers to either Box up the failure error or depend on failure if they want to explicitly handle the error.

Moreover, error types do not have any precise information in the type about what went wrong. thiserror would allow us to provide very specific error variants, as well as avoiding leaking types into the public API and letting downstream consumers handle the variants as they wish.
@poros
Copy link
Collaborator

poros commented Apr 1, 2020

I have used failures too much to like it, but at the same time I became kinda skeptical of any new rust error-handling library :/
At a first glance it looks like to be a good one, but before embarking in such a big change, maybe I would wait to see if the Rust community converges on something ;)

@cpcloud
Copy link
Contributor Author

cpcloud commented Apr 2, 2020

The main issues here IMO are the inability for dependents to get more specific error messages, and leaking the failure dependency if you don't want to box everywhere. The convenience of error reporting the way its currently done in avro rs is really for applications, whereas library errors should be as specific and efficient as possible.

@athre0z
Copy link
Contributor

athre0z commented Jun 8, 2020

FWIW, failure is now officially deprecated:
rust-lang-deprecated/failure#347

With all new error handling libs being built around std::error::Error, it's not insanely important what library the community converges towards. Whether you use thiserror or any other of the new libs, other than with failure, the errors can just be converted into a common Box<dyn std::error::Error>.

@cpcloud
Copy link
Contributor Author

cpcloud commented Jun 10, 2020

The specific library doesn't matter. What matters is the way that errors are presented to dependents IMO. The way in which failure is used in avro-rs precludes dependents handling errors in a way that is appropriate for their use.

@poros
Copy link
Collaborator

poros commented Jul 3, 2020

I have been following a bunch of discussions in the community and it does indeed seem that anyhow and thiserror don't have a clear path to obsolescence in front of them for now, neither there are other "big" enough players to choose from (mark my words, a new revolutionary crate will be announced the minute after I hit "Comment" 😛 ).

Agreed, let's move to thiserror and this time let's make error handling right, as @cpcloud pointed it out.

@poros poros self-assigned this Jul 3, 2020
poros added a commit to poros/avro-rs that referenced this issue Jul 3, 2020
@poros
Move from failure to thiserror
33348da 
Closes flavray#115

This is still a WIP branch, with lots of TODOs and some things about
thiserror I still can't wrap my head around. However, the heavy-lifting
is done, the failure crate has been removed from the list of
dependencies and compilation, tests, benchmarks and linting are all
green.

The two biggest things I have yet to figure out are:
1. How to deal with the errors manually defined in ser.rs and de.rs:
   they are publicly available and as soon as I touch anything I hit
   cryptic serde errors
2. How to make errors like TryFromIntError part of more abstract ones
   like ParseSchemaError, which could have a source error or just a
   String description.
@poros poros mentioned this issue Jul 3, 2020
Merged
poros added a commit to poros/avro-rs that referenced this issue Jul 5, 2020
@poros
Move from failure to thiserror
ea9917e 
Closes flavray#115

This is still a WIP branch, with lots of TODOs and some things about
thiserror I still can't wrap my head around. However, the heavy-lifting
is done, the failure crate has been removed from the list of
dependencies and compilation, tests, benchmarks and linting are all
green.

The two biggest things I have yet to figure out are:
1. How to deal with the errors manually defined in ser.rs and de.rs:
   they are publicly available and as soon as I touch anything I hit
   cryptic serde errors
2. How to make errors like TryFromIntError part of more abstract ones
   like ParseSchemaError, which could have a source error or just a
   String description.
poros added a commit that referenced this issue Jul 10, 2020
@poros @athre0z
Move from failure to thiserror (#135)
9592038 
* Move from failure to thiserror

Closes #115

This is still a WIP branch, with lots of TODOs and some things about
thiserror I still can't wrap my head around. However, the heavy-lifting
is done, the failure crate has been removed from the list of
dependencies and compilation, tests, benchmarks and linting are all
green.

The two biggest things I have yet to figure out are:
1. How to deal with the errors manually defined in ser.rs and de.rs:
   they are publicly available and as soon as I touch anything I hit
   cryptic serde errors
2. How to make errors like TryFromIntError part of more abstract ones
   like ParseSchemaError, which could have a source error or just a
   String description.

* Update tests/io.rs

Co-authored-by: Joel Höner <joel@zyantific.com>

* Renaming errors + apply clippy consistently

 Rename AvroError to Error
 Removed redundant Error suffix from variants
 Introduce AvroResult shorthand alias with crate visibility
 Align clippy invocation in tests with the one in pre-commits

* Stop stressing about generic errors and add a couple more sprecific ones

* Centralize Ser and De errors into Error

The trick was implementing the ser::Error and de::Error trait for
crate::errors::Error and return Error::Ser and Error::De variants
in the implementation of the custom() method.

* SnappyCdcError as struct for consistency

* Update CHANGELOG

* Update CHANGELOG, README and add a Migration Guide page

Co-authored-by: Joel Höner <joel@zyantific.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@poros poros

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move from failure to thiserror poros/avro-rs
3 participants
@cpcloud @poros @athre0z