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

Kill "sink", reify "Observer", kill send* free functions #2442

Merged
merged 10 commits into from Oct 16, 2015

Conversation

Projects
None yet
9 participants
@andymatuschak
Contributor

andymatuschak commented Oct 2, 2015

When we were introducing teammates to RAC 4, there were two related sticking points that seemed like incidental complexity: the conflation of “sink” and “observer”, and the fact that sendNext and friends were free functions (didn’t autocomplete, looked weird, etc).

Here, I’ve done the following:

  • Event no longer knows about sinks or observers.
  • Observer is now its own thing akin to SinkOf but specific to Events. There is no such thing as a sink anymore.
  • Event.sink() becomes a convenience initializer for Observer.
  • send* become methods on Observer
  • Added some convenience overloads where raw observer/sink functions were arguments so that they can continue to be used that way for less line noise.

If you folks like this approach, I’ll update docs etc.

Checkpoint
Test plan: Crossed fingers
@@ -164,23 +143,3 @@ extension Event: EventType {
return self
}
}

This comment has been minimized.

@NachoSoto
@@ -15,7 +15,7 @@ import Result
/// Signals do not need to be retained. A Signal will be automatically kept
/// alive until the event stream has terminated.
public final class Signal<Value, Error: ErrorType> {
public typealias Observer = Event<Value, Error>.Sink
public typealias Observer = ReactiveCocoa.Observer<Value, Error>

This comment has been minimized.

@NachoSoto

NachoSoto Oct 2, 2015

Member

Is ReactiveCocoa. needed here?

This comment has been minimized.

@andymatuschak

andymatuschak Oct 2, 2015

Contributor

Yeah, to avoid an error about an infinitely self-referencing type. :/

This comment has been minimized.

@NachoSoto

NachoSoto Oct 2, 2015

Member

Oooh right

let onError = { sendError(observer, $0) }
let onBothCompleted = { sendCompleted(observer) }
let onInterrupted = { sendInterrupted(observer) }
let onError = { observer.sendError($0) }

This comment has been minimized.

@NachoSoto

NachoSoto Oct 2, 2015

Member

I wonder if you can just write let onError = observer.sendError here and everywhere else.

This comment has been minimized.

@andymatuschak

andymatuschak Oct 2, 2015

Contributor

Great idea; done.

This comment has been minimized.

@ghost

ghost Oct 12, 2015

Is this going to change to let onError = observer.sendError anymore?

This comment has been minimized.

@andymatuschak

andymatuschak Oct 12, 2015

Contributor

Thanks; I'd hallucinated that there was an error associated with doing that, but not so!

@NachoSoto

This comment has been minimized.

Member

NachoSoto commented Oct 2, 2015

Looks amazing :D

@neilpa: I was telling @andymatuschak that this was probably overlooked since these used to be free functions in RAC 3, because we couldn't extend SinkType before, but it seems like a natural transition for RAC 4. Let us know what you think! :)

@NachoSoto NachoSoto added this to the 4.0 milestone Oct 2, 2015

@raylillywhite

This comment has been minimized.

Contributor

raylillywhite commented Oct 2, 2015

@andymatuschak Looks like you forgot to add Observer.swift

Build's failing with:

:0: error: no such file or directory: '/Users/rayll/dev/github/ReactiveCocoa/ReactiveCocoa/Swift/Observer.swift'

andymatuschak added some commits Oct 2, 2015

Add Observer.swift you big dummy
Test plan: crossed fingers
Point-free sweetness
Test plan: crossed fingers
@andymatuschak

This comment has been minimized.

Contributor

andymatuschak commented Oct 2, 2015

@raylillywhite I'm a big dummy. Fixed!

@NachoSoto

This comment has been minimized.

Member

NachoSoto commented on ReactiveCocoa/Swift/Signal.swift in eb07996 Oct 2, 2015

Hmm this one isn't equivalent, it would have to be observer.sendError, but then you probably need to specify the type of onError, so maybe just leave this one the same?

@NachoSoto

This comment has been minimized.

Member

NachoSoto commented on ReactiveCocoa/Swift/Signal.swift in eb07996 Oct 2, 2015

Oops, you probably didn't mean to call them :) let onBothCompleted = observed.sendCompleted

Fix point-free sweetness.
Test plan: crossed fingers
@andymatuschak

This comment has been minimized.

Contributor

andymatuschak commented Oct 3, 2015

@nacho Yikes. Had the wrong scheme selected when I hid Cmd+U, wasn't paying attention. Thanks.

/// Convenience to avoid lots of extra parentheses everywhere people call observe(). TODO(andy): Document me.
public func observe(action: Observer.Action) -> Disposable? {
return observe(Observer(action))
}

This comment has been minimized.

@ghost

ghost Oct 3, 2015

This method is already implemented in the protocol extension here.

This comment has been minimized.

@andymatuschak

andymatuschak Oct 5, 2015

Contributor

Right! The protocol extension is enough.

@NachoSoto

This comment has been minimized.

Member

NachoSoto commented Oct 6, 2015

This looks good to me now!
@andymatuschak can you rebase it?

@andymatuschak

This comment has been minimized.

Contributor

andymatuschak commented Oct 6, 2015

Done! This is a pretty significant API change, so would appreciate further opinions from @ReactiveCocoa/reactivecocoa. @neilpa, I know you touched this recently—what do you think?

@robrix

This comment has been minimized.

Contributor

robrix commented Oct 7, 2015

I’m very much in favour.

Is there other documentation (in .md files, say) that will need updating?

@andymatuschak

This comment has been minimized.

Contributor

andymatuschak commented Oct 7, 2015

Yeah, certainly. I’ll do that if I don’t hear opposition here.

On Oct 6, 2015, at 5:17 PM, Rob Rix notifications@github.com wrote:

I’m very much in favour.

Is there other documentation (in .md files, say) that will need updating?


Reply to this email directly or view it on GitHub #2442 (comment).

@jspahrsummers

This comment has been minimized.

Member

jspahrsummers commented Oct 7, 2015

@andymatuschak

This comment has been minimized.

Contributor

andymatuschak commented Oct 8, 2015

OK, "sink" is now fully eradicated and all docs updated; ready for final review and merge.

@NachoSoto

This comment has been minimized.

Member

NachoSoto commented Oct 8, 2015

ship it

@bencochran

This comment has been minimized.

Contributor

bencochran commented Oct 10, 2015

Stepping in as a slight outsider to say that I’m very much a fan.

@alanjrogers

This comment has been minimized.

Member

alanjrogers commented Oct 12, 2015

👏🏼

Remove unnecessary closure literal
Test plan: Build and run the tests.
@NachoSoto

This comment has been minimized.

Member

NachoSoto commented Oct 16, 2015

This will need rebasing now that #2449 is merged. @andymatuschak I'm happy to do it for you if you'd like!

@neilpa

This comment has been minimized.

Member

neilpa commented Oct 16, 2015

Been AFK for the last few weeks and catching up but I'm 💯 ✖️ 💯

@andymatuschak

This comment has been minimized.

Contributor

andymatuschak commented Oct 16, 2015

Ready to go! (thanks for the kind offer, @NachoSoto ❤️)

@NachoSoto

This comment has been minimized.

Member

NachoSoto commented Oct 16, 2015

✈️

NachoSoto added a commit that referenced this pull request Oct 16, 2015

Merge pull request #2442 from ReactiveCocoa/observer-api
Kill "sink", reify "Observer", kill send* free functions

@NachoSoto NachoSoto merged commit 0d34f1f into swift2 Oct 16, 2015

@paulyoung

This comment has been minimized.

Contributor

paulyoung commented Oct 16, 2015

So great.

@robrix robrix deleted the observer-api branch Oct 16, 2015

@neilpa

This comment has been minimized.

Member

neilpa commented Oct 17, 2015

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment