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 unwrap() with expect("unique message") #681

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kvc0
Copy link

@kvc0 kvc0 commented Jan 13, 2023

unwrap() gives no clues where to look when a bug arises. This change
replaces common tdigest unwrap() calls with expect("message").

This does the same thing as unwrap(), it just provides more context
for users to report with bugs.

Fwiw, I find that explaining an expectation in rust can sometimes
show me that the expectation is unreasonable. I can't come up with
good explanations for some of these, but I gave an effort to at
least provide a first-approximation of what the expectation at work
is. Any refinements to what these expectations are actually trying
to assert would be welcome!

It would be nice to be able to provide more information for #680

I've only got a couple years of experience with Rust, but I'd strongly
recommend taking a "no unwrap() outside of tests" policy!

unwrap() gives no clues where to look when a bug arises. This change
replaces common tdigest unwrap() calls with expect("message").

This does the same thing as unwrap(), it just provides more context
for users to report with bugs.

Fwiw, I find that explaining an expectation in rust can sometimes
show me that the expectation is unreasonable. I can't come up with
good explanations for some of these, but I gave an effort to at
least provide a first-approximation of what the expectation at work
is. Any refinements to what these expectations are actually trying
to assert would be welcome!
@CLAassistant
Copy link

CLAassistant commented Jan 13, 2023

CLA assistant check
All committers have signed the CLA.

@epgts epgts self-assigned this Jan 17, 2023
@syvb syvb unassigned epgts Jun 19, 2023
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.

3 participants