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.

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

Merged
merged 5 commits into from
Apr 23, 2019

Conversation

mosamer
Copy link
Contributor

@mosamer mosamer commented Apr 22, 2019

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;

  • Change inputs type from InputSubject ~> AnyObserver
  • Drop InputSubject
  • Improved test cases for action inputs behavior

Copy link
Member

@freak4pc freak4pc left a 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 :)

Changelog.md Show resolved Hide resolved
@bobgodwinx
Copy link
Member

This is awesome and AnyObserver does the job perfectly. Thanks @mosamer

@bobgodwinx bobgodwinx merged commit d463ba9 into RxSwiftCommunity:4.0 Apr 23, 2019
@freak4pc
Copy link
Member

freak4pc commented May 1, 2019

Hey! Just realized this was merged to a branch called 4.0 :O
It doesn't exist on master. Can we fix this?

@freak4pc freak4pc mentioned this pull request May 1, 2019
freak4pc pushed a commit that referenced this pull request May 1, 2019
* 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)
@ashfurrow
Copy link
Member

@freak4pc I don't think we can change the target branch after we merge – is this okay as-is?

@freak4pc
Copy link
Member

freak4pc commented May 2, 2019

@ashfurrow Heya, I already cherry picked this: 5e7fb77

:-)

@mosamer
Copy link
Contributor Author

mosamer commented May 3, 2019

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 master

@freak4pc
Copy link
Member

freak4pc commented May 3, 2019

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 master. I think it might be better to have a develop branch if you want to have a staging thing because otherwise it makes it super confusing.

Some of these milestone issues are 2-3 years old which also made me think it's a mistake.

@mosamer
Copy link
Contributor Author

mosamer commented May 3, 2019

Sorry, I guess this confusion is a result of this 4.0 milestone being abandoned for a while. branch 4.0 is/was intended as a develop branch for 4.0 issues which are mainly major breaking changes, I am trying to get this done over the next few weeks hopefully

@freak4pc
Copy link
Member

freak4pc commented May 3, 2019

@mosamer We've already released 4.0.0 to provide support for RxSwift 5, so if you continue on that path we should:

  1. Maybe cut a new (develop) branch from masster
  2. Apply your changes over that
  3. We can apply these changes as a minor 4.x release

@mosamer
Copy link
Contributor Author

mosamer commented May 3, 2019

@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? 🤔

@freak4pc
Copy link
Member

freak4pc commented May 3, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants