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

Would using thiserror crate be acceptable for the error handling? #638

Open
wt opened this issue Aug 25, 2023 · 2 comments · May be fixed by #640
Open

Would using thiserror crate be acceptable for the error handling? #638

wt opened this issue Aug 25, 2023 · 2 comments · May be fixed by #640
Labels
enhancement

Comments

@wt
Copy link
Contributor

wt commented Aug 25, 2023

I would like to refactor the errors crate to use the thiserror crate. The patterns implemented in the module are very similar to the crate. The crate is widely used in rust libs. I think it would probably raise the quality of the code a bit to use the crate. Would this be an interesting change?
@Mingun
Copy link
Collaborator

Mingun commented Aug 25, 2023

If you want to use it just for defining error messages via derive macro then I think this is unnecessary. My points are:

  • thiserror requires rust 1.56+, we require rust 1.52+. While I generally not against raising required version, that change also should give significant improvements
  • overhead from manually implementing necessary traits are minimal
  • new dependency increases build time. That is fine if it gives us new abilities but I prefer to avoid that if not

If you like also rework our errors to make them more ergonomic, then I'm not against. Actually, we should do that, some of our errors are cryptic. I also would like to have backtraces in our errors, but I'm unsure should we do something special to give them or we can get them automatically. Rust books is not clear about that.

The other thing that we lack in our errors is a position information (there is a dedicated issue for this -- #625). I would like to learn approaches used in other serde format libraries.

@wt
Copy link
Contributor Author

wt commented Aug 25, 2023 via email

wt added a commit to wt/quick-xml that referenced this issue Aug 27, 2023
@wt
Use thiserror crate to make error definition more ergonomic.
6c19f36 
The thiserror crate takes care of a bunch of details of implementing
errors for a library in rust. This will simplify the code around our
errors by making it follow conventions that are widely used in the rust
community. It also is much shorter in terms of lines of code.

Closes tafia#638
@wt wt linked a pull request Aug 27, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use thiserror crate to make error definition more ergonomic. wt/quick-xml
2 participants
@wt @Mingun