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

Find unwrap() and expect()calls and eliminate them #21

Open
jhelovuo opened this issue Feb 14, 2021 · 4 comments
Open

Find unwrap() and expect()calls and eliminate them #21

jhelovuo opened this issue Feb 14, 2021 · 4 comments

Comments

@jhelovuo
Copy link
Owner

Either replace with more graceful error-handling or write comments to justify why panic cannot occur.

@xrl
Copy link

xrl commented Mar 5, 2022

I highly recommend the eyre error library for applications that want rich error support. But the eyre says that thiserror is a good choice for errors you want to match against (common for libraries).

https://github.com/yaahc/eyre#comparison-to-thiserror
https://docs.rs/thiserror/latest/thiserror/

This will give you a useful path forward when you are chasing out all the unwraps. Also, remember that ? is very powerful for short-circuiting out of error situations.

@LebranceBW
Copy link

Is this still a problem? I'm willing to fix them.

@jhelovuo
Copy link
Owner Author

Yes, sort of. Please go ahead and submit a PR.

Some cases may be fixable by writing comments why a panic is not possible. In some cases, such as taking locks, refactoring .lock() into a separate function would at least clean up and provide a place for an error message.

@LebranceBW
Copy link

PR
I made some small attempts. Advice wanted.

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

No branches or pull requests

3 participants