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 all unwrap() with expect() #1023

Closed
billy1624 opened this issue Sep 8, 2022 · 8 comments · Fixed by #1231
Closed

Replace all unwrap() with expect() #1023

billy1624 opened this issue Sep 8, 2022 · 8 comments · Fixed by #1231
Milestone

Comments

@billy1624
Copy link
Member

Motivation

We should refactor all unwrap() in SeaORM and use expect() insead. This will force us to provide a explicit message when panicking. This is far more readable than a muted panic.

@baoyachi
Copy link
Contributor

baoyachi commented Sep 9, 2022

I wish to take this into account by the way, necessary for error distinguish :#881

@giripriyadarshan
Copy link
Contributor

Can we have a new label named "Motivation" with prefixes like beginner, moderate and expert ....... Would help to filter the issues

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 14, 2022

Oh Nice I wanted to say this a long time ago.

But it's also good to think about whether we can avoid panicking on a case by case basis.

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 6, 2022

+1

@billy1624
Copy link
Member Author

billy1624 commented Nov 15, 2022

Same here.

Btw, we should document all the method that throw panic, specify under what circumstance it will panic.

Deny this linter and it will tell you which methods need to be documented
https://rust-lang.github.io/rust-clippy/master/#missing_panics_doc

Also, this linter would be helpful to deny the use of unwrap()
https://rust-lang.github.io/rust-clippy/master/#unwrap_used

@billy1624
Copy link
Member Author

I wish to take this into account by the way, necessary for error distinguish :#881

Hey @baoyachi, what do you mean?

I think in general, we should avoid panic. And should throw Err instead.
Did you mean that?

@billy1624
Copy link
Member Author

Can we have a new label named "Motivation" with prefixes like beginner, moderate and expert ....... Would help to filter the issues

I'm going to do a cleanup on the open issues and label some beginner friendly issues as "good first issue". And convert Q&A into discussion.

@baoyachi
Copy link
Contributor

baoyachi commented Nov 15, 2022

"I think in general, we should avoid panic. And should throw Err instead."

I agree. @billy1624

@billy1624 billy1624 added this to the 0.11.x milestone Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants