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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add .success / .failure methods that cause sideeffects only #50

Closed

Conversation

tamamachi
Copy link

It will be handy if we could just add event handlers without changing values or types of Tasks
(now SwiftTask requires .success / .failure closures to return a new value or Task).
In particular, it's painful to use .failure when you just want to add an event handler and don't want to recover from failures, because it still requires successful return value.

This PR adds .success / .failure methods which have closures of returning type Void for their arguments.
You pass a closure returns nothing (or Void), and you get a new Task nearly identical to previous one but it keeps the closure as an event handler.

This change disables handling Tasks with Void as the Value type (eg. Task<Progress, Void, Error>), but no one might want to use such creepy Tasks 馃槈

@inamiy
Copy link
Member

inamiy commented Dec 6, 2015

Hi, thank you for pull request 馃槃

It looks good to me at first glance, but unfortunately, your implementation will break existing API because of ambiguous use of functions. Please cherry-pick d9f2191 (submodule-fix) and test if same issue will happen on your Mac.

2015-12-07 6 46 26

(Note: CircleCI seems to pass XCTest, but actually I didn鈥檛 configure this async test on purpose...)

To improve this, I鈥檓 thinking of renaming it as func on(success:failure:).
This method name is used in ReactiveCocoa and also has similarity with Scala鈥檚 future.onSuccess()/onFailure().

Also, I鈥檓 thinking of returning same task rather than creating new synchronous task.
Though this sounds not functional-programming-like, it improves overall performance by skipping instantiation.

@inamiy
Copy link
Member

inamiy commented Dec 6, 2015

I improved your idea in #51. Will you please check this?

@inamiy inamiy added this to the 4.1.0 milestone Dec 6, 2015
@tamamachi
Copy link
Author

Thank you for your review! #51 will be much greater.

@tamamachi tamamachi closed this Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants