Skip to content

Add optional message argument to Result.getOrThrow and improve default error message #7630

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mediremi
Copy link
Contributor

This change:

  • Improves the default error message of Not_found thrown by Result.getOrThrow
  • Improves consistency between Option and Result by adding an optional message argument to Result.getOrThrow

@mediremi mediremi marked this pull request as ready for review July 11, 2025 15:20
@mediremi mediremi force-pushed the result-get-or-throw-message branch from c36f3e7 to 013b234 Compare July 11, 2025 15:27
@mediremi mediremi marked this pull request as draft July 11, 2025 15:39
@mediremi mediremi force-pushed the result-get-or-throw-message branch from 013b234 to e4c07a3 Compare July 11, 2025 15:47
@mediremi mediremi marked this pull request as ready for review July 11, 2025 15:51
Comment on lines -29 to +35
| Error(_) => throw(Not_found)
| Error(_) =>
Stdlib_JsError.panic(
switch message {
| None => "Result.getOrThrow called for Error value"
| Some(message) => message
},
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess technically this is a breaking change. But I don't think we've stopped because of this before (going from exception to general JS error).

Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment from my side. But would be good if someone more took a look at it.

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.

2 participants