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 to AnyObserver #127

Closed
wants to merge 4 commits into from
Closed

Conversation

mosamer
Copy link
Contributor

@mosamer mosamer commented Nov 22, 2017

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<Input> ~> AnyObserver<Input>
  • Drop InputSubject
  • Improved test cases for action inputs behavior

Test that work factory is called with received inputs instead of testing that they are received on `inputs` subject
@mosamer
Copy link
Contributor Author

mosamer commented Nov 22, 2017

I'd appreciate @RxSwiftCommunity/contributors input on the following items;

@freak4pc
Copy link
Member

@mosamer I couldn't find the time to dig into all the tests but overall that seems sufficient to me. We can always add more tests later :)

If your tests correctly deprecate the need for InputSubject, I'm all for getting rid of it.

One gotcha I have is the naming of the tests. Usually Quick-style tests "read" like english self-descriptions of the tests.
I would expect the tests to read like:

  • describe Inputs
    • it should execute on .next events
    • it should ignore .error events
    • it should ignore .completed events
    • it should accept multiple .next events
    • it should not terminate after an .error event
    • it should not terminate after a .completed event

You could also go for "it executes on .next events, it ignores .error events" etc ...
Just a minor nit :)

@bobgodwinx
Copy link
Member

I am closing this PR because it's been hanging there for more than 1 year. It someone want to still work on it please feel free to complete it. Thanks

@bobgodwinx bobgodwinx closed this Mar 16, 2019
@rxswiftcommunity
Copy link

Warnings
⚠️

It looks like code was changed without adding anything to the Changelog. If this is a trivial PR that doesn't need a changelog, add #trivial to the PR title or body.

Generated by 🚫 dangerJS

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

Successfully merging this pull request may close these issues.

3 participants