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

Naked Unwraps #52

Open
njgheorghita opened this issue May 23, 2023 · 2 comments
Open

Naked Unwraps #52

njgheorghita opened this issue May 23, 2023 · 2 comments

Comments

@njgheorghita
Copy link
Contributor

I don't have a reproducible issue that I can show, but I can conclusively say that as I go about debugging this library I do run into panicked threads due to naked unwraps. In trin we took a firm stance against the use of naked unwraps. (According to clippy) there are 31 naked unwraps in utp. Some of which have documentation explaining their use. Some of which don't contain documentation. There are cases where naked unwraps are used in methods that do not return a Result which complicates resolving these unwraps.

I guess I'm just curious about the decision to allow naked unwraps in this library. Why? What was the criteria for using an unwrap vs implementing error handling for individual cases? It seems to me like resolving these unwraps would improve the performance of this library, but would like to get some confirmation before undertaking the task.

@njgheorghita
Copy link
Contributor Author

I guess a simpler option to eliminate all the naked unwraps could be to just convert them to expect()s

@jacobkaufmann
Copy link
Collaborator

yea.. some of the unwrap instances were for expediency, and in other cases they represent assertions that should not fail. in such cases where you expect invariants to hold, I think unwrap is acceptable. I can take a look. if there are common places where the code panics, then that would be helpful.

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

2 participants