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

Merged
merged 9 commits into from
Jul 20, 2025

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
Contributor Author

@mediremi mediremi Jul 17, 2025

Choose a reason for hiding this comment

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

I've added a note to the changelog about this breaking change 👍

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.

Copy link

pkg-pr-new bot commented Jul 15, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7630

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7630

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7630

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7630

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7630

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7630

commit: daea31f

@nojaf
Copy link
Collaborator

nojaf commented Jul 20, 2025

@cknitt can we merge this? (after conflict resolve).
@mediremi doesn't have permissions I believe.

@cknitt
Copy link
Member

cknitt commented Jul 20, 2025

Yes, please rebase @mediremi.

@mediremi
Copy link
Contributor Author

@cknitt I've rebased and moved the changelog entries to be under 12.0.0-beta.3

@nojaf nojaf merged commit 249cb40 into rescript-lang:master Jul 20, 2025
27 checks passed
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.

4 participants