Add Option.all* & Result.all* helpers

[bloodyowl]

No description provided.

[cknitt]

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]

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]

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]

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]

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

[cometkim]

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

[cknitt]

Couldn't that just be

let hasNone = ref(false)

instead? I would find that clearer.

[cknitt]

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

[cknitt]

@bloodyowl Let's stay with *.all as proposed. Could you get CI build to pass (and maybe simplify Option.all according to my comment) so that we can merge?

[bloodyowl]

@cknitt should be all good now!

[cknitt]

Thanks, merged! 🙂