PromiseKit icon indicating copy to clipboard operation
PromiseKit copied to clipboard

when(resolved:) tuple overloads

Open Skoti opened this issue 6 years ago • 14 comments

This PR introduces tuple overloads for when(resolved:) (which make use of Thenable protocol) and additional tuple overloads for when(fulfilled:) up to 9 arguments. This PR also introduces an associated type R representing the result type in Thenable protocol which improves a return type in when(resolved:) when using Guarantees, so this is now allowed:

let p1 = Promise.value(true)
let p2 = Promise.value(2)
let g1 = Guarantee.value("abc")

when(resolved: p1, p2, g1).done { r1, r2, r3 in
    // r1 is Result<Bool>, r2 is Result<Int>, r3 is String
}

In the future it might be worth considering changing the pipe declaration to use R instead of Result<T>, this would however impact all the other operators so I didn't want to introduce such a big change in this PR.

Skoti avatar Aug 24 '19 11:08 Skoti

Seems cool. Can we make tests pass?

mxcl avatar Sep 01 '19 20:09 mxcl

Done :)

Skoti avatar Sep 02 '19 18:09 Skoti

still interested in merging? :)

Skoti avatar Oct 08 '19 21:10 Skoti

This would be great - is there a timeline on this or is PMK7 incompatible/already implementing this?

IgnusG avatar Dec 06 '19 09:12 IgnusG

If we can base this onto v7 we can get it in.

mxcl avatar Jun 02 '21 13:06 mxcl

If we can base this onto v7 we can get it in.

Ok great! Give me some time :)

Skoti avatar Jun 02 '21 21:06 Skoti

  1. I've rebased this PR and added tuple arity functions for cancellable thenables up to 9 arguments.
  2. I've also improved when(resolved:) for array and when(fulfilled:) with generator to work for any CancellableThenable and not only CancellablePromise.
  3. I've added when(resolved:) with generator.
  4. I've renamed cancellableWhen functions to just when to match the naming convention.
  5. In order to keep the tests more readable and clean I've added a shared TestError and TestCancelError types.

Skoti avatar Jun 05 '21 20:06 Skoti

Merge conflicts.

mxcl avatar Jun 07 '21 14:06 mxcl

The PR was still set to merge to master. I've just changed the base branch to v7.

Skoti avatar Jun 07 '21 15:06 Skoti

CI did not run (GitHub Actions bug perhaps) we need that to verify that the changes do not cause ambiguity. Even then I am concerned about the change of cancellableWhen to when being ambiguous to the compiler.

I'd like to see this PR without all the infrastructure changes since it bloats it substantially.

mxcl avatar Jun 07 '21 15:06 mxcl

The ambiguity usages were due to the variadic when functions.

I agree this is better, but breaking API is not allowed, API breakage can only happen for major version bumps, so this may have to be a v7 PR if we cannot solve it.

But we agreed that it is better to have tuple arity functions, as variadic is easy to fix as you just add [ and ] around the arguments. From the calling point of view it is almost no difference. Not sure what is more convenient with variadic arguments that an array couldn't handle.

By infrastructure you mean adding a common module?

Skoti avatar Jun 11 '21 15:06 Skoti

@mxcl I got rid of the infrastructure changes.

Skoti avatar Jul 14 '21 16:07 Skoti