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

WIP. failure -> thiserror/anyhow #148

Closed
wants to merge 1 commit into from
Closed

WIP. failure -> thiserror/anyhow #148

wants to merge 1 commit into from

Conversation

cjordan
Copy link
Contributor

@cjordan cjordan commented Mar 28, 2021

This is my work-in-progress on converting rubbl's usage of failure to using thiserror and/or anyhow (anyhow is good for "catch all" scenarios, whereas thiserror is good for writing nice Rust errors that can be propagated). This is beneficial, because failure has apparently been deprecated, and by using thiserror Error types, one can use ? on functions that return result types.

The current state of this branch does not compile, but I think I'd like your input before I continue; it's getting a little subjective. Hopefully it's easy to see what's going on -- it looks like the failure API is very similar to thiserror/anyhow, but I was still struggling on a couple of little things.

You can let me know what you think, and I can have another crack at getting things fully converted.

@pkgw
Copy link
Owner

pkgw commented Mar 29, 2021

Thanks so much for taking this on @cjordan!

From my work on Tectonic and other projects, I've been getting more familiar with anyhow and thiserror, and I'm very excited to switch to using them here in Rubbl. That being said, I still feel like I haven't quite reached Rust Error Handling Total Zen Understanding — I feel like I'm still building up my sense for how to use these tools optimally in different circumstances. So I'll comment on a couple of topics here but unfortunately my guidance is going to be a little vague :-)

Part of the reason for that is that overall I think this is looking great! Nothing here that looks unwise to me.

One question that I've been thinking about in anyhow/thiserror-world is how useful enum error types are. Instead of a TableError enum, for instance, you could export individual structs for the sub-errors of interest (i.e. do what the current code does) and people could use anyhow's downcasting support to test for specific error kinds. This feels a bit cleaner to me in a certain sense ... but enum types do give callers the ability to test for "is this error any kind of Rubbl table error", which you don't get without the enum layer. So I think the decision here kind of comes down to how you expect callers to handle your errors.

Likewise, especially in the FITS code, there's a question of creating individual types or enum variants for specific format errors, vs. just using anyhow! with string values. The only real advantage of stringy anyhow! errors that I can think of to reduce the API surface area a bit — the other main advantage being ease of implementation, but you've already gone and done the work to switch things over. Extensibility is something to keep in mind but there's some Rust enum annotation (#[non_exhaustive_enum] or something?) to indicate that an enum type may have new variants added in the future. Overall I think I'd be fine with using stringy errors, especially in something like FITS parsing code where there are just oodles of random ways that things can go wrong, but I'm pretty sure it's strictly better to have types for those errors.

As a side note I'm a bit confused because the anyhow docs read to me as if they have unconditional backtrace support, but when switching to anyhow in other crates I've had to go comment out my backtrace-printing code as you've done here.

Once again, great work and thanks a bunch for taking on this project!

@pkgw
Copy link
Owner

pkgw commented Jan 20, 2023

Closing this older PR since the action is in #220 now.

@pkgw pkgw closed this Jan 20, 2023
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

Successfully merging this pull request may close these issues.

2 participants