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 Option.all* & Result.all* helpers #7181

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

Conversation

bloodyowl
Copy link
Collaborator

No description provided.

@cknitt
Copy link
Member

cknitt commented Nov 29, 2024

Ah I see this is basically rescript-lang/rescript-core#202 which I forgot to re-review, sorry about that!

Build failed because of a code formatting issue.

Checking ReScript code formatting...
[format check] runtime/Result.res
[format check] runtime/Option.res
[format check] runtime/Option.resi
[format check] runtime/Result.resi
[format check] tests/tests/src/core/Core_ResultTests.res
The 5 files listed above need formatting
⚠️ ReScript code formatting issues found.

@glennsl
Copy link
Contributor

glennsl commented Nov 29, 2024

This is called zip in most other libraries. all can also easily be confused with every (which would have been better named forall) if generalized to any collection.

@bloodyowl
Copy link
Collaborator Author

This is called zip in most other libraries. all can also easily be confused with every (which would have been better named forall) if generalized to any collection.

my rationale here is to keep it familiar by mimicking Promise.all, which would (I believe) play a lot together in code:

let (user, status) = await Promise.all2((getUser(id), getStatus(id)))

Result.all2((user, status)).map(((user, status)) => {
  // ...
})

@glennsl
Copy link
Contributor

glennsl commented Nov 29, 2024

my rationale here is to keep it familiar by mimicking Promise.all, which would (I believe) play a lot together in code:

Yeah, I get it. Reproducing these kinds of design mistakes invites other issues though, such as confusion with every/forall and this.

@fhammerschmidt
Copy link
Member

And by the way we already have zip in Core.List.

@cometkim
Copy link
Member

cometkim commented Nov 29, 2024

Yeah this implementation is looks like zip for sure.

Promise.all is because it is not about its data, but concurrency. Like other APIs in same family allSettled / race / any / try


let all = options => {
let acc = []
let returnValue = ref(None)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't that just be

let hasNone = ref(false)

instead? I would find that clearer.

@cknitt
Copy link
Member

cknitt commented Dec 3, 2024

BTW build failed because you need to check in the updated Core_ResultTests.mjs.

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