-
Notifications
You must be signed in to change notification settings - Fork 429
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
Lifetime.observeEnded
and take(during:)
fix.
#229
Conversation
229e219
to
5c7e576
Compare
51c17b2
to
d3ecd73
Compare
I think I'm 👍 on |
import Result | ||
|
||
final class LifetimeSpec: QuickSpec { | ||
override func spec() { | ||
describe("Lifetime") { | ||
it("should complete its lifetime ended signal when the it deinitializes") { | ||
let object = MutableReference(TestObject()) | ||
describe("ended") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nitpick, but do you think we can make this change without reindenting the file? It makes the diff a little hard to follow, and there's no shared setup/teardown code.
I really like the simplicity of exposing If we're concerned about the safety issue you mentioned, adding the terminated events to |
I'm +1 for not exposing But I don't get why disposable is needed as an argument of Also, I don't think P.S. I think |
@inamiy When the lifetime has already terminated. |
I still prefer I can be 👍 to But I'd prefer not to add |
I don't quite see the problem here. We can just treat a fire-exactly-once |
Yeah, so I think writing |
1cfcf00
to
3b938ba
Compare
d766c06
to
3a5bafa
Compare
3a5bafa
to
b52f302
Compare
Lifetime.observeEnded
and take(during:)
fix.
The deprecation of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,6 +1,6 @@ | |||
import Quick | |||
import Nimble | |||
import ReactiveSwift | |||
@testable import ReactiveSwift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need to be @testable
?
b52f302
to
3081538
Compare
New methods and operator:
attach
,observeEnded
and.+=
ended
provides a fullSignal
interface that is overblown for the general use cases ofLifetime
.[Bugfix?] Safe observation
& deprecatingLifetime.ended
.ended.observeCompleted
does not handle the observations to a terminatedended
Signal, and the action is simply ignored.It should have been invoked regardless, like how adding a
Disposable
to disposedCompositeDisposable
would dispose of the disposable being added.Composition ofLifetime
is suggested to be done through lifetime operators instead, e.g.and
andor
, should the demand arise.