Replace start/observe closure syntax with distinct functions #2318

sebreh · 2015-09-06T16:23:20Z

Issues #2311 contains the background discussion for this change.

In Swift 2 (and the swift2 branch of this repo), the SinkOf<T> type is gone and instead a normal closure T -> () is used. This is great but it introduces a potential risk due to the behaviour change when calling SignalProducer.start() with a trailing closure. In Swift 1.x, the trailing closure would default to the next argument, and in order to provide a sink you would need to pass in a SinkOf<Event<T>>. In Swift 2 however, the trailing closure will match the new sink type T -> (). From the call site, the call looks identical after migrating to the swift2 branch but the behaviour is different. This could potentially introduce issues in cases where the argument to start is ignored (_), as the type checker will not catch it and the closure will be called on next/interrupted/error/completed and not just on next.

Since Box is no longer needed with Swift 2, this PR replaces the start(next:_:completed:_:error:_:interrupted) syntax with only one start and observe that takes a sink when handling multiple cases, as well as separate functions startNext(), startCompleted(), startError(), startInterrupted() for handling a single case.

Since Box is not needed anymore in Swift 2, we can encourage the use of Event<T, E> in start/observe calls to handle multiple cases. In cases where only one case is needed, startNext/startCompleted/ startError/startInterrupted replaces start(next:_:completed:_:error:_:interrupted:_) on SignalProducer, and observeNext/observeCompleted/observeInterrupted replaces observe(next:_:completed:_:error:_:interrupted:_) on Signal.

sebreh · 2015-09-06T16:41:14Z

You could also make the case that startNext should also allow optional completion and error closures, i.e. make it startNext(next: T -> (), completion: (() -> ())? = nil, error: (E -> ())? = nil, interrupted: (() -> ())? = nil), but I am not sure how heavily the RAC team wants to encourage the use of the Event<T, E> type in handlers.

NachoSoto · 2015-09-06T17:30:04Z

ReactiveCocoa/Swift/ObjectiveCBridging.swift

+				subscriber.sendError(error as NSError)
+			case .Completed:
+				subscriber.sendCompleted()
+			default:


I like this PR for this line right here: users will be aware of their need to explicitly ignore events, which wasn't the case before due to the default arguments!

It is definitely not as compact on the caller side, but its probably worth it to be more explicit.

neilpa · 2015-09-06T18:46:13Z

You could also make the case that startNext should also allow optional completion and error closures

Interesting idea. If you only provide a next block do trailing closures still work? If so, I'm not even sure we'd want the other startError, etc. methods.

And for the sake of comparison, the different ways for dealing with 2 event cases:

// startNext:completed:
producer.startNext(next: { value in
    print("value: \(value)")
}, completed: { _ in
    print("completed")
})

// start switch
producer.start {
    switch $0 {
    case let .Next(value):
        print("value: \(value)")
    case let .Completed:
        print("completed")
    default:
        break
    }
}

// start if case
producer.start { event in
    if case let .Next(value) = event {
        print("value: \(value)")
    } else if case .Completed = event {
        print("completed")
    }
}

neilpa · 2015-09-06T19:03:04Z

ReactiveCocoa/Swift/ObjectiveCBridging.swift

-		})
+		let selfDisposable = producer.start { event in
+			switch event {
+			case .Next(let value):


I think most of the RAC code does case let .Next(value): instead of moving the let inside the case name.

Good catch, I'll change it.

sebreh · 2015-09-06T19:49:41Z

Yes, the trailing closure works. It would make it basically identical to the previous closure based start behavior, but without the naming ambiguity. I personally like having the option of only providing closures as I think it makes for cleaner code at the call site, but I was unsure if you guys wanted to force people to use the event based start() to make sure all cases are explicitly handled.

neilpa · 2015-09-06T20:36:38Z

I tend to agree after looking at the different approaches side by side. My proposal would be to have 3 convenience methods (don't think interrupted is needed).

startWithNext(next: T -> (), error: (E -> ())? = nil, completed: (() -> ())? = nil, interrupted: (() -> ())? = nil)
startWithError(error: E -> (), completed: (() -> ())? = nil, interrupted: (() -> ())? = nil)
startWithCompleted(completed: () -> (), error: (E -> ())? = nil, interrupted: (() -> ())? = nil)
// observeNext, etc. too

The error param could be removed from startWithCompleted (or vice-versa) but having them both enables either order at the call-site.

was unsure if you guys wanted to force people to use the event based start()

Nope, since you can still use switch with default or if case.

sebreh · 2015-09-06T20:57:50Z

I agree that API would make the most sense 👍

neilpa · 2015-09-06T22:22:49Z

ReactiveCocoa/Swift/SignalProducer.swift

@@ -316,8 +316,28 @@ extension SignalProducerType {
 	///
 	/// Returns a Disposable which can be used to interrupt the work associated
 	/// with the Signal, and prevent any future callbacks from being invoked.
-	public func start(error error: (E -> ())? = nil, completed: (() -> ())? = nil, interrupted: (() -> ())? = nil, next: (T -> ())? = nil) -> Disposable {
-		return start(Event.sink(next: next, error: error, completed: completed, interrupted: interrupted))
+	public func startWithNext(error error: (E -> ())? = nil, completed: (() -> ())? = nil, interrupted: (() -> ())? = nil, next: (T -> ())? = nil) -> Disposable {


I didn't realize that the next param was going to be optional and have to be last. This is what I meant if the trailing closure syntax would still work in the case where it was first but the only non-optional param. If that's not possible, I don't think this is the right change and is potentially more confusing. At this point it may be back to the individual startWithNext, startWithError, etc. approach and no optional params.

Although, I would sacrifice the trailing closure syntax to require the associated closure be the first param and while still supporting the remaining optional closures

// this still seems reasonable to me ... producer.startWithNext({ println("Next \($0)") })

Hmm. The next-closure doesn't have to be optional but then the ordering will matter (e.g. completed must precede next), so that is not an option IMO. It does have to be the last argument to support the trailing closure.

I'm not sure. I think keeping it clean at the call site could be worth an somewhat un-intuitive argument ordering. This was the same style used with the previous overloading of start, but I agree it gets more confusing with the new startWithNext naming.

Looks like we have the following options:

producer.startWithNext({ println("Next \($0)") }) producer.startWithNext({ println("Next \($0)") }, error: { println("Error \($0)") })

producer.startWithNext { println("Next \($0)") } producer.startWithNext( next: { println("Next \($0)") }, error: { println("Error \($0)") })

producer.startWithNext { println("Next \($0)") } producer.startWithError { println("Error \($0)") }

I like (2) the best, but would also be ok with (1). The problem with (1) is that it looks like it should support trailing closure syntax, but doesn't.

I'm not sure. I think keeping it clean at the call site could be worth an somewhat un-intuitive argument ordering.

This is really why I don't like (2) because it doesn't work when you have more than one closure argument. I think I'm back to the (3) camp with single closure argument and use start if you want to handle multiple.

After thinking about it a bit more, I tend to agree it might be the least confusing option. Time will tell if it would be too cumbersome to use.

So if we agree on this, I'll go ahead and revert my last commit and go back to the original suggestion.

sebreh · 2015-09-07T21:04:42Z

@neilpa that should be it. Also changed the "case let" style to be consistent across the code base.

neilpa · 2015-09-08T05:00:55Z

Awesome! Thanks for putting all the time/discussion into this one.

Replace start/observe closure syntax with distinct functions

robertjpayne · 2015-09-10T12:01:30Z

I didn't reallse see this convo but I'd like to say the new API in my opinion is inferior to the old API because there is no way to chain the subscriptions and now if we ever have to listen in on more than one event type we have to run a ugly as switch statement.

I'd much much much prefer to have the optional closures back.

robertjpayne · 2015-09-10T12:04:14Z

Actually maybe it's not too bad having the switch statement… guess it depends on how much logic has to go into these blocks and suppose it should probably be minimal.

sebreh added 2 commits September 6, 2015 17:57

Updated specs to use new start/observe naming

75c2654

NachoSoto reviewed Sep 6, 2015
View reviewed changes

neilpa added this to the Swift 2 milestone Sep 6, 2015

neilpa reviewed Sep 6, 2015
View reviewed changes

startWithX/observeX with optional closures

100d93f

neilpa reviewed Sep 6, 2015
View reviewed changes

sebreh added 2 commits September 7, 2015 22:42

Remove optional closures from startWithX/observeX

6304325

Consistent use of "case let" in switch cases

6753042

neilpa added a commit that referenced this pull request Sep 8, 2015

Merge pull request #2318 from sebreh/swift2

03ab047

Replace start/observe closure syntax with distinct functions

neilpa merged commit 03ab047 into ReactiveCocoa:swift2 Sep 8, 2015

neilpa mentioned this pull request Sep 9, 2015

Swift 2 sink type changes behavior of calling start with trailing closure #2311

Closed

ikesyo mentioned this pull request Apr 23, 2016

Swift 2: start with trailing closure does not default to next #2294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace start/observe closure syntax with distinct functions #2318

Replace start/observe closure syntax with distinct functions #2318

sebreh commented Sep 6, 2015

sebreh commented Sep 6, 2015

NachoSoto Sep 6, 2015

sebreh Sep 6, 2015

neilpa commented Sep 6, 2015

neilpa Sep 6, 2015

sebreh Sep 6, 2015

sebreh commented Sep 6, 2015

neilpa commented Sep 6, 2015

sebreh commented Sep 6, 2015

neilpa Sep 6, 2015

neilpa Sep 6, 2015

sebreh Sep 7, 2015

neilpa Sep 7, 2015

sebreh Sep 7, 2015

neilpa Sep 7, 2015

sebreh commented Sep 7, 2015

neilpa commented Sep 8, 2015

robertjpayne commented Sep 10, 2015

robertjpayne commented Sep 10, 2015

Replace start/observe closure syntax with distinct functions #2318

Replace start/observe closure syntax with distinct functions #2318

Conversation

sebreh commented Sep 6, 2015

sebreh commented Sep 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilpa commented Sep 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebreh commented Sep 6, 2015

neilpa commented Sep 6, 2015

sebreh commented Sep 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebreh commented Sep 7, 2015

neilpa commented Sep 8, 2015

robertjpayne commented Sep 10, 2015

robertjpayne commented Sep 10, 2015