Skip to content

Move result type from Belt to built-in. #6450

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 1 commit into from
Oct 25, 2023
Merged

Move result type from Belt to built-in. #6450

merged 1 commit into from
Oct 25, 2023

Conversation

cristianoc
Copy link
Collaborator

No description provided.

@cristianoc cristianoc requested a review from cknitt October 25, 2023 11:56
@cristianoc
Copy link
Collaborator Author

@zth does this affect anything? E.g. pipe completion.

@zth
Copy link
Collaborator

zth commented Oct 25, 2023

I'd have to try it, I'm not sure really. I would assume no, and if it does it should be easy to fix.

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Why did the order in ocaml_re_test.js change?

@cristianoc
Copy link
Collaborator Author

Why did the order in ocaml_re_test.js change?

Because things change randomly whenever stuff in pervasive change.
Some long-standing nondeterminism in the compiler somewhere.
It's alway the order of mutually recursive definitions.

@cristianoc
Copy link
Collaborator Author

cristianoc commented Oct 25, 2023

As a side comment, it seems likely that most tests mentioning Belt are either tests for Belt itself, or offer unclear value at this point (in catching future regressions). Both can be just removed.
The rest, should be easy to convert to not use belt.

@cristianoc cristianoc merged commit 02f84a1 into master Oct 25, 2023
@cristianoc cristianoc deleted the belt_result branch October 25, 2023 14:16
@TheSpyder
Copy link
Contributor

This breaks code that (unnecessarily) references the constructors as Belt.Result.Error and Belt.Result.Ok:
https://github.com/dusty-phillips/rescript-zora/blob/76e08155f24d8aa8c8bbdf7553522bbc5192039f/src/Zora.res#L41-L45

I don't mind submitting a PR to rescript-zora to fix it, but can this please be listed as a breaking change? It took me a while to track down that RC 5 was when it broke, and even longer to locate exactly why - because it wasn't listed in the changelog at all.

@anmonteiro
Copy link
Contributor

Alternatively, the non-breaking change would be to alias the result type properly:

type result<'a, 'b> = result<'a, 'b> =
  | Ok('a)
  | Error('b)

@cknitt
Copy link
Member

cknitt commented Dec 7, 2023

I agree, we should alias this properly.

@TheSpyder Would you mind submitting a PR to the rescript-compiler repo to for this?

@TheSpyder
Copy link
Contributor

Ok, will do

@TheSpyder
Copy link
Contributor

@cknitt #6514

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.

5 participants