-
Notifications
You must be signed in to change notification settings - Fork 151
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
Change inputs
type to AnyObserver<Input>
#193
Conversation
Test that work factory is called with received inputs instead of testing that they are received on `inputs` subject
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.
This looks great to me! 🎉 Awesome work.
I'm just not knowledgable enough with Action
to make a call here, so let's wait for someone else here as well :)
This is awesome and |
Hey! Just realized this was merged to a branch called 4.0 :O |
* improve input tests Test that work factory is called with received inputs instead of testing that they are received on `inputs` subject * add test cases for input receiving terminating events * change `inputs` type to `AnyObserver<Input>` * drop `InputSubject` * Update changelog (cherry picked from commit d463ba9)
@freak4pc I don't think we can change the target branch after we merge – is this okay as-is? |
@ashfurrow Heya, I already cherry picked this: 5e7fb77 :-) |
Hi @freak4pc, I am not sure what are we trying to fix 🤔 This issue is part of Release-4.0 milestone Why do you need to pick it to |
I'm super confused - you've merged 3 PRs of yours lately. This was the only one aimed at a 4.0.0 branch. Everything else was part of Some of these milestone issues are 2-3 years old which also made me think it's a mistake. |
Sorry, I guess this confusion is a result of this 4.0 milestone being abandoned for a while. branch |
@mosamer We've already released 4.0.0 to provide support for RxSwift 5, so if you continue on that path we should:
|
@freak4pc I am fine with 1 & 2. As for nr. 3, given these are breaking changes to public API are we fine releasing them as a minor 4.x release. wdyt? 🤔 |
Uhm I see. That's a tad tricky so I'm not 100% sure. We could bump to 5 but if that's the case maybe we should gather some more changes? Since 4 was just released now. |
Re-opening #127, not sure what was blocking it 🤔 so rebased and created a new PR for review
This PR is implementing migration of inputs property from SubjectType ~> ObserverType as discussed on #120
It includes the following items;