Skip to content

Add on(success:failure:) for adding side-effects. #51

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

Merged
merged 8 commits into from
Dec 10, 2015

Conversation

inamiy
Copy link
Member

@inamiy inamiy commented Dec 6, 2015

This is a new feature and improvement on #50 to add side-effects after completion (success or failure) of task.

Please see SwiftTaskTests.swift#L425-L474 for more detail.

Note

Using Self for returning value will fail in Xcode 7.0, but not in 7.1.

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

Thanks a lot ! Looks great ✨

In my environment, "on()" and "on(&canceller)" are displayed in code completion.
I'm afraid it might be confusing a little bit. Do you have any ideas to suppress them?
I think we can use optional for args but I'm not sure.

@inamiy
Copy link
Member Author

inamiy commented Dec 7, 2015

@tamamachi
Yes, I understand your saying.
Normally, we don't usually see this mysterious &canceller inout parameter in any other Promises libraries, but it is actually a recommended implementation in order to precisely control lifetime of registered closures (although many users might think this signature awkward...)

Since I have already implemented this &canceller feature in other methods e.g. then(&canceller), I think it is reasonable to continue adding for on() as well for now. Then, next major version bump will be the time to rethink of these implementations again.

@tamamachi
Copy link

Oh, I meant following 4 candidates were shown and first 2 ones were thought to be unnecessary:

  • on() - unnecessary
  • on(&canceller) - unnecessary
  • on(success, failure) - good
  • on(&canceller, success, failure) - good

I think this is because default values are specified for args, so maybe we can change the signature
from like
on(success success: Value -> Void = { _ in }, failure: ErrorInfo -> Void = { _ in }) -> Self
to, sort of,
on(success success: (Value -> Void)?, failure:(ErrorInfo -> Void)?) -> Self

How do you think?

@inamiy
Copy link
Member Author

inamiy commented Dec 10, 2015

Yes, having &canceller parameter is always a downside, but AFAIK, it's not possible remove due to Swift's inout behavior which can't use default parameter.

Your suggestion on changing arguments to Optional is better, so I fixed it in 3ae32c6. But still, I kept using default parameters for success and failure because there will be needs to omit one of those argument, i.e. calling on(success:) and on(failure:).

I think it's important to support simple & consistent API (method signature) more than code completion. (In general, using default parameters easily get worse code completion...)

@inamiy
Copy link
Member Author

inamiy commented Dec 10, 2015

BTW, even though things may get better, I'm not having plan to rename on(success:) & on(failure:) to separate names e.g. onSuccess() and onFailure() just because it will make confusion with current success() & failure().

@tamamachi
Copy link

Now it looks all goid for me! 💯
I agree with you about not separating .on method.
I don't mind anything about cancellers 😉

@inamiy
Copy link
Member Author

inamiy commented Dec 10, 2015

Thanks for check!
I'll bump new version for this tonight ;)

@inamiy inamiy merged commit 146a7bd into swift/2.0 Dec 10, 2015
@inamiy
Copy link
Member Author

inamiy commented Dec 10, 2015

Bumped ver 4.1.0. @tamamachi Thanks again!

@inamiy inamiy deleted the on-success-failure branch December 10, 2015 14:54
@tamamachi
Copy link

Thanks! 🍻

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.

2 participants