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

49 feature request flatmap #57

Merged
merged 3 commits into from Nov 3, 2018

Conversation

venth
Copy link
Contributor

@venth venth commented Mar 13, 2018

I've prepared flatMap operator, which is consistent with current implementation. Feature request issue: #49 Feature Request: FlatMap

@@ -69,7 +70,8 @@ func (o Observable) Subscribe(handler rx.EventHandler) <-chan subscription.Subsc
}

// OnDone only gets executed if there's no error.
if sub.Error == nil {
nilPassed := !reflect.ValueOf(sub.Error).IsValid()
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Write test to justify the problem or improvement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll add the test for it. Forgot to implement it.

Copy link
Member

Choose a reason for hiding this comment

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

Not problem, code review is for that

@venth venth force-pushed the 49_feature-request-flatmap branch from 3d93883 to 655cc70 Compare March 23, 2018 14:36
Copy link
Member

@avelino avelino left a comment

Choose a reason for hiding this comment

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

Very very good, As I had imagined did not need to use the reflect (for this reason I commented to write test), congratulations

@venth
Copy link
Contributor Author

venth commented Mar 24, 2018

Thank you :) You were right. I have idea for other operators as well like: create, merge, zip. Shall I add issue before hand?

@marcsantiago
Copy link

@avelino is this going to me merged in?

@avelino
Copy link
Member

avelino commented Oct 10, 2018

@avelino is this going to me merged in?

@marcsantiago @venth this week I will analyze, apparently we need an use example in the README (In the future on wiki), I keep you informed if I have some other point.

Copy link
Member

@avelino avelino left a comment

Choose a reason for hiding this comment

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

write example used on
https://github.com/ReactiveX/RxGo/tree/master/examples
and README pls

@venth
Copy link
Contributor Author

venth commented Oct 30, 2018

@avelino, I'll provide examples and complete the README documentation over the weekend. Thanks for reviewing the code 👍

@avelino
Copy link
Member

avelino commented Oct 30, 2018

Sorry to make review broken, I am with limited time

@venth
Copy link
Contributor Author

venth commented Nov 3, 2018

@avelino I've pushed the changes. Would you like to take a look at them?

@coveralls
Copy link

coveralls commented Nov 3, 2018

Pull Request Test Coverage Report for Build 130

  • 0 of 90 (0.0%) changed or added relevant lines in 4 files are covered.
  • 269 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-43.3%) to 36.186%

Changes Missing Coverage Covered Lines Changed/Added Lines %
observable/observable.go 0 5 0.0%
observable/create.go 0 16 0.0%
observer/observer_mock.go 0 28 0.0%
observable/flatmap.go 0 41 0.0%
Files with Coverage Reduction New Missed Lines %
observer/observer.go 1 70.0%
iterable/iterable.go 2 71.43%
subscription/subscription.go 3 78.57%
errors/errors.go 9 43.75%
handlers/handlers.go 15 0.0%
observable/observable.go 239 4.6%
Totals Coverage Status
Change from base Build 128: -43.3%
Covered Lines: 241
Relevant Lines: 666

💛 - Coveralls

@avelino avelino merged commit f4a32c6 into ReactiveX:master Nov 3, 2018
@avelino
Copy link
Member

avelino commented Nov 3, 2018

thanks @venth

@venth
Copy link
Contributor Author

venth commented Nov 3, 2018

@avelino I’ve just pushed improvements to test coverage and fix one error. How can I fix it? Next PR?

@avelino
Copy link
Member

avelino commented Nov 3, 2018

Open new PR @venth

@avelino
Copy link
Member

avelino commented Nov 4, 2018

@venth more help #72

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.

None yet

4 participants