linkedin / parseq

Asynchronous Java made easier
Apache License 2.0
1.17k stars 266 forks source link

andThen overloading is confusing #287

Closed junchuanwang closed 3 years ago

junchuanwang commented 4 years ago

We have two andThen function available. One takes a consumer and second takes a Task. They have different semantics and behavior.

https://github.com/linkedin/parseq/wiki/User's-Guide#sequential-composition

A misuse case is that user might use the variant that takes a Consumer function to return the task and want that task to be run, because any object can be cast to void, so the compiler won't issue warning of the consumer function returns the task

Solution can be (1) add a new interface, e.g. something like andThenTask (2) add additional logic to see if the return value from the Consumer function is of Task type and execute it (this requires another overloading andThen to take a Function<T, Task>

Linked Issue: https://github.com/linkedin/parseq/issues/207

aliciatang commented 3 years ago

+1 andThen use be used to indicate order of execution. And the fact that when it takes a Task but does not pass on the result from the previous task is unexpected.

junchuanwang commented 3 years ago

https://github.com/linkedin/parseq/pull/291 can be complementary solution to #287 and #207

BrianPin commented 3 years ago

Good on identifying the misuse. +1 for slight preference on the resolution (2) But I feel there is a common understanding among what consumer means, so it can return a task sounds a bit not common and different, so I suggest never expect consumer return anything

junchuanwang commented 3 years ago

option (2) is not viable: the Consumer and Function when they apply to the lambda function, they have same erased type. So the lambda function will find two ambiguous overloading.

A better options is to change the signature to default Task<T> andThen(final String desc, final Function1<? super T, ?> function) and check the return valuke of the function call.

Why I changed my mind: But same behavior is achievable using flatMap. So it is hard to say if want to run the task if the return type found to be a task, because the user got this confusion just because they probably not aware of FlatMap.

I think we should educate the users instead.