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

Add expect-like functionality to Result and Option #140

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

Conversation

gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Jun 15, 2024

Reference: https://doc.rust-lang.org/std/result/enum.Result.html#method.expect

Implemented by adding an optional message string parameter for the respective unwraps.

@slavovojacek
Copy link
Contributor

Why not also call this expect and implement it as per the Rust link you attached?

@gibbz00
Copy link
Contributor Author

gibbz00 commented Jun 18, 2024

Good question, two things that go hand in hand.

I've always found expect to be counterintuitive, for example: option.expect("it to extist") will result in a panic message containing something similar to: "program panicked, unwrapped option: 'it to exist'". In other words: you call expect when you "expect" it to work, but supply the message to be shown when it doesn't. (Granted, I'm not the first one to voice this concern.)

Rust doesn't have operator overloading, so it might be one reason for why the naming is the way it is in the first place. JS/TS does, however, and I think this is a good case for when to apply it. It reduces the number of functions exposed, and it makes unwrapping more natural, even though the latter is of corue more of a personal opinion.

@slavovojacek
Copy link
Contributor

Thanks for the explanation, makes sense. A few thoughts:

  1. I am slightly concerned unwrap(errorMessage) might easily be misinterpreted to mean unwrap(defaultValue)
  2. Given all other methods are named after their Rust equivalent I would lean towards following that convention despite the unfortunate naming - it's just one less translation step people have to go through when trying to link it back to the original implementation

Please let me know what you think!

@corbinu
Copy link

corbinu commented Aug 14, 2024

@slavovojacek I agree with keeping with Rust equivalents on this.

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