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

Replace anyhow::Error with enums in bvgraph::load #113

Closed
wants to merge 1 commit into from

Conversation

progval
Copy link
Contributor

@progval progval commented Aug 16, 2024

This allows crates using epserde to unwrap errors and match them.

In my case, I would like to display a custom message on WrongTypeHash/VersionMismatch with instructions to regenerate files with the current epserde version. Currently this is not enough for this use case (because epserde still returns anyhow::Error), but vigna/epserde-rs#32 will take care of it once merged.

This allows crates using epserde to unwrap errors and `match` them.

In my case, I would like to display a custom message on WrongTypeHash/VersionMismatch with
instructions to regenerate files with the current epserde version.
Currently this is not enough for this use case (because epserde still
returns anyhow::Error), but a similar change will go to epserde.
@vigna
Copy link
Owner

vigna commented Aug 17, 2024

Two questions:

  • the anyhow docs show an example of match using downcasting. Why does that not work for you?

  • I remember we introduced anyhow elsewhere because it provides backtraces and context messages. Why don't we need that anymore?

@progval
Copy link
Contributor Author

progval commented Aug 17, 2024

the anyhow docs show an example of match using downcasting. Why does that not work for you?

Sorry, I missed that. It should be good enough for me. (It lacks match exhaustiveness checking, but this may actually be a blessing as it allows you to add new errors without it being a breaking change.)

I remember we introduced anyhow elsewhere because it provides backtraces and context messages. Why don't we need that anymore?

I added the context messages, but wrapping enums in other enums achieves the same result. I don't know about backtraces.

@progval progval closed this Aug 17, 2024
@vigna
Copy link
Owner

vigna commented Aug 17, 2024

Note that I'm not against the idea. We are using anyhow anywhere because that's what Tommaso was using at that time. I have read in the past year various comments suggesting that thiserror is better for libraries and anyhow is better for applications, but frankly with little support logic. There should certainly be a chapter about that in the guidelines and these PRs might be a good starting point for the discussion.

@progval
Copy link
Contributor Author

progval commented Aug 17, 2024

I'd say it's because anyhow loses the hierarchy of where an error bubbles through. For example, if you do both File::open("graph.graph").with_context("could not open graph.graph")? and File::open("graph.ef").with_context("could not open graph.ef")? then the caller can downcast to an io::Error, but can't tell which file failed to open

anyhow also has a higher overhead due to the dynamic dispatch, but we care in webgraph/swh-graph because it's only while loading, so not in any hot loop.

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