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.

Sign up for GitHub

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

Jump to bottom

Implement PropertyType.signal. #2622

Merged
merged 6 commits into from
Jan 3, 2016
Merged

Implement PropertyType.signal. #2622

merged 6 commits into from
Jan 3, 2016

Conversation

andersio
Copy link
Member

Fixes: #2543

Signal.empty

It is a signal that completes immediately without emitting any value. Future observation to this signal would only result in an Interrupted event per the Signal.observe implementation.

ConstantProperty

ConstantProperty.signal is just a Signal.empty, since there is never a change anyway.

MutableProperty

MutableProperty.signal is a signal started from the buffering producer at the time the property is initialised.

DynamicProperty

DynamicProperty.signal and DynamicProperty.producer are backed by an underlying MutableProperty. The implementation is not thread-safe (as usual...?).

@andersio andersio force-pushed the propertySignal branch 2 times, most recently from 4f122f7 to c6d3bef Compare December 20, 2015 16:11
NachoSoto
NachoSoto reviewed Dec 20, 2015
View reviewed changes
ReactiveCocoa/Swift/Property.swift
self.producer = SignalProducer(value: value)

producer = SignalProducer(value: value)
signal = Signal { observer in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Signal.empty

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only Signal.never. :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh. Then let's add that! :)

@NachoSoto
Copy link
Member

Thanks for tackling this! Can you rebase now that #2617 is merged?

NachoSoto
NachoSoto reviewed Dec 20, 2015
View reviewed changes
ReactiveCocoa/Swift/Property.swift
@@ -135,6 +187,8 @@ public final class MutableProperty<Value>: MutablePropertyType {
private weak var object: NSObject?
private let keyPath: String

private var _property: MutableProperty<AnyObject?>? = MutableProperty<AnyObject?>(nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type here is redundant.

@andersio andersio force-pushed the propertySignal branch 2 times, most recently from 5715639 to d0fae54 Compare December 20, 2015 17:49
@andersio
Copy link
Member Author

Rebased and updated. :-)

NachoSoto
NachoSoto reviewed Dec 20, 2015
View reviewed changes
ReactiveCocoa/Swift/Property.swift

/// Initializes the property to have the given value.
public init(_ value: Value) {
self.value = value
self.producer = SignalProducer(value: value)
producer = SignalProducer(value: value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use self.xxx = ... here for consistency?

@andersio
Copy link
Member Author

Rebased and reverted to use SignalProducer.buffer.

MutableProperty.signal is now a signal started from the buffering producer. By the way, I just realised that there is no need to apply skip(1) based on the behavior of startWithSignal on a buffering producer.

I have added also a few "should yield... even if the value actually remains unchanged" test cases.

@andersio andersio force-pushed the propertySignal branch 2 times, most recently from fd9964a to c748f26 Compare December 20, 2015 19:13
@andersio andersio closed this Dec 20, 2015
@andersio andersio reopened this Dec 20, 2015
@andersio
Copy link
Member Author

The failing test case is from RACSignalSpec, hmm. All Swift specs passed.

RACSignalSpec _setKeyPath_onObject___should_keep_object_alive_over__sendNext__UserstravisbuildReactiveCocoaReactiveCocoaReactiveCocoaTestsObjectiveCRACSignalSpecm_1338

expected to equal <1.0000>, got <nil> (use beNil() to match nils)

NachoSoto
NachoSoto reviewed Dec 21, 2015
View reviewed changes
ReactiveCocoa/Swift/Property.swift
@@ -97,6 +109,16 @@ public final class MutableProperty<Value>: MutablePropertyType {
}
}

/// A signal that will send the property's changes over time,
/// then complete when the property has deinitialized.
public lazy var signal: Signal<Value, NoError> = { [unowned self] in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. Eventually I think we should actually expose this from SignalProducer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a SignalProducer.startedSignal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Feel free to extract that now, but I think we should leave it as internal for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just leave it alone then.

@NachoSoto
Copy link
Member

Looks perfect! Only thing missing is a test for Signal.empty, and then it's good to go!

NachoSoto
NachoSoto reviewed Dec 21, 2015
View reviewed changes
ReactiveCocoaTests/Swift/PropertySpec.swift
switch event {
case .Interrupted:
signalInterrupted = true
case .Next(_), .Failed(_), .Completed:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: don't need parenthesis

@NachoSoto
Copy link
Member

Awesome :shipit:

@RuiAAPeres
Copy link
Member

This is pretty nice! GJ @andersio. @NachoSoto could it be merged (I actually have use case for this)?

NachoSoto added a commit that referenced this pull request Jan 3, 2016
@NachoSoto
Merge pull request #2622 from andersio/propertySignal
f77534c 
Implement `PropertyType.signal`.
@NachoSoto NachoSoto merged commit f77534c into ReactiveCocoa:master Jan 3, 2016
@andersio andersio deleted the propertySignal branch January 4, 2016 01:22
@andersio andersio mentioned this pull request Mar 6, 2016
Closed
@ikesyo ikesyo mentioned this pull request Apr 20, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andersio @NachoSoto @RuiAAPeres