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

[refactor] join(...tasks) & cancel(...tasks) should return join/cancel descriptors #1515

Closed
Andarist opened this issue Jul 2, 2018 · 5 comments
Assignees
Milestone

Comments

@Andarist
Copy link
Member

Andarist commented Jul 2, 2018

At the moment they are internally wrapped in all but actually the effect type should get preserved so internal refactor is needed.

At the same time we might consider changing those signatures to:

join([...tasks])
cancel([...tasks])

but I'm still on the fence with that, not sure which one is better - variadic arguments or a single array argument? any fresh thoughts on this one @restrry @shinima ?

@Andarist Andarist added this to the v1 milestone Jul 2, 2018
@feichao93
Copy link
Member

feichao93 commented Jul 3, 2018

I'm inclining to single array argument.

The problem with variadic arguments I found is that join(singleTask) matches both join(...tasks) and join(task), and it is unclear that yield join(singleTask) should return taskResult or [ taskResult ].

It is clearer to use array argument: If you pass a single task object, then the result is a single result object; if you pass an array of tasks, then the result is an array too. Like in all or race effects, the result type (array or object) is determined by the argument type.

By the way, this issue had got some discussion in #1248.

@Andarist
Copy link
Member Author

Andarist commented Jul 7, 2018

You have convinced me to array argument with simplicity of input arg type to output arg type mapping 👍

@mshustov
Copy link
Contributor

@shinima would you like to work on it?

@feichao93
Copy link
Member

OK. I'll get it done in a few days.

@feichao93 feichao93 self-assigned this Jul 10, 2018
feichao93 added a commit that referenced this issue Jul 23, 2018
@feichao93
Copy link
Member

feichao93 commented Jul 23, 2018

Done in #1527

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

No branches or pull requests

3 participants