Skip to content

Replace anyhow with thiserror in libraries #1845

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

Open
emilk opened this issue Apr 14, 2023 · 2 comments · May be fixed by #9579
Open

Replace anyhow with thiserror in libraries #1845

emilk opened this issue Apr 14, 2023 · 2 comments · May be fixed by #9579
Labels
good first issue Good for newcomers 🚜 refactor Change the code, not the functionality

Comments

@emilk
Copy link
Member

emilk commented Apr 14, 2023

We have been too liberal of our use of anyhow. For any pub function in our libraries, we should be using thiserror instead of anyhow, to get more type safety and explicitness of what can go wrong.

Internal use of anyhow in a crate is less bad, but would also be good to change to thiserror.

anyhow should mostly be used in high-level crates (binaries) to easily summarize errors coming from many different places without having to create too many error enums

@emilk emilk added good first issue Good for newcomers 🚜 refactor Change the code, not the functionality labels Apr 14, 2023
Wumpf pushed a commit that referenced this issue Sep 29, 2023
…3487)

<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What
- Improves Use of `thiserror` in
crates([#1845](#1845))

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/{{
pr.number }}) (if applicable)

- [PR Build Summary](https://build.rerun.io/pr/{{ pr.number }})
- [Docs preview](https://rerun.io/preview/{{ pr.commit }}/docs)
<!--DOCS-PREVIEW-->
- [Examples preview](https://rerun.io/preview/{{ pr.commit }}/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
@Wumpf Wumpf self-assigned this Jan 9, 2024
Wumpf added a commit that referenced this issue Jan 10, 2024
…4759)

### What

* part of #1845 

Started also removing it the file resolver/reloader but it's super
tedious and makes errors worse since during include resolve we often
want to stack errors. We can revisit that if we ever move out the shader
reloading or when we tackle

* #2664 

but otherwise I don't think it's worth it.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4759/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4759/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4759/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4759)
- [Docs
preview](https://rerun.io/preview/79d6075baae6651555be0431005e07817440c3b5/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/79d6075baae6651555be0431005e07817440c3b5/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@Wumpf Wumpf removed their assignment Jan 10, 2024
@emilk
Copy link
Member Author

emilk commented Jul 14, 2024

We should do this in re_types in particular, and somehow forbid it so that it doesn't come back.

@ishakbhatt
Copy link

ishakbhatt commented Apr 10, 2025

I have replaced it in re_types and am doing a proper verification that it works before making a draft pull request. Do we want to include all of the other crates in a single pull request or would you prefer doing this with re_types and scoping it later to include other crates? Based on previous merged pull requests I can put together one for re_types @emilk

Edit: check #9579

@ishakbhatt ishakbhatt linked a pull request Apr 10, 2025 that will close this issue
emilk added a commit that referenced this issue May 19, 2025
### Related
* #1845
* #8681

### What
This problem keeps biting us in the ass: We get really bad error reports
from users because the default `Display` for `anyhow::Error` only
includes the latest context, and not the whole error 😠
jprochazk pushed a commit that referenced this issue May 24, 2025
### Related
* #1845
* #8681

### What
This problem keeps biting us in the ass: We get really bad error reports
from users because the default `Display` for `anyhow::Error` only
includes the latest context, and not the whole error 😠
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants