Skip to content

Add mapConcat to Observable#1106

Merged
Avasil merged 2 commits into
monix:masterfrom
fchaillou:observable-mapConcat
Jan 14, 2020
Merged

Add mapConcat to Observable#1106
Avasil merged 2 commits into
monix:masterfrom
fchaillou:observable-mapConcat

Conversation

@fchaillou
Copy link
Copy Markdown
Contributor

@fchaillou fchaillou commented Jan 9, 2020 •
edited
Loading

Hello this PR adds

def [B] mapConcat(f: A: => Seq[B]) : Observable[B]

on Observable
which is a shorter version of

obs.flatMap(a => Observable.fromIterable(f(a))

I had to wait on the ack in the onComplete to make sure all elements from the last upstream onNext are properly propagated before completing our downstream.

Thank you

@fchaillou fchaillou force-pushed the observable-mapConcat branch from d118add to 845a480 Compare January 10, 2020 16:10
Avasil
Avasil approved these changes Jan 10, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Avasil Avasil left a comment

Choose a reason for hiding this comment

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

Fantastic job @fchaillou - I really appreciate a scaladoc with example.

I've left some comments but I can merge it as is

Comment thread monix-reactive/shared/src/main/scala/monix/reactive/Observable.scala Outdated
Comment thread monix-reactive/shared/src/main/scala/monix/reactive/Observable.scala Outdated
Comment thread monix-reactive/shared/src/test/scala/monix/reactive/internal/operators/MapConcatSuite.scala
import scala.concurrent.duration._
import scala.concurrent.{Future, Promise}

object MapConcatSuite extends BaseOperatorSuite {
Copy link
Copy Markdown
Collaborator

@Avasil Avasil Jan 10, 2020

Choose a reason for hiding this comment

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

It feels like we could add property based test mapConcat(f) <-> flatMap(x => fromIterable(f(x))

I'm also curious about performance difference but I can check myself later :D

Copy link
Copy Markdown
Contributor Author

@fchaillou fchaillou Jan 10, 2020

Choose a reason for hiding this comment

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

Good idea.
i can take care of that next week if you don't mind waiting

Copy link
Copy Markdown
Contributor Author

@fchaillou fchaillou Jan 13, 2020

Choose a reason for hiding this comment

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

Do you have any pointers on doing that with minitest ?
thanks

Copy link
Copy Markdown
Collaborator

@Avasil Avasil Jan 13, 2020

Choose a reason for hiding this comment

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

Take a look at ReduceSuite

Copy link
Copy Markdown
Contributor Author

@fchaillou fchaillou Jan 13, 2020

Choose a reason for hiding this comment

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

Thanks, just pushed a test, let me know if that makes sense for you ?

Copy link
Copy Markdown
Collaborator

@Avasil Avasil Jan 13, 2020

Choose a reason for hiding this comment

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

I think the test looks great :)

@fchaillou fchaillou force-pushed the observable-mapConcat branch from c39c6b6 to 44b3816 Compare January 13, 2020 16:50
@Avasil
Copy link
Copy Markdown
Collaborator

Avasil commented Jan 13, 2020

Thank you a lot, there are some compile errors after changing Seq to Iterable - once you fix it it's good to merge :)

@fchaillou fchaillou force-pushed the observable-mapConcat branch from 4d4986f to 9735f20 Compare January 13, 2020 20:26
@fchaillou
Copy link
Copy Markdown
Contributor Author

fchaillou commented Jan 13, 2020

Oh, sbt kept the cache versions locally so i missed those.
This should hopefully be good now :)

Avasil
Avasil reviewed Jan 13, 2020
View reviewed changes
Comment thread monix-reactive/shared/src/main/scala/monix/reactive/Observable.scala Outdated
@fchaillou @Avasil
Update monix-reactive/shared/src/main/scala/monix/reactive/Observable…
eb38891 
….scala

Co-Authored-By: Piotr Gawryś <pgawrys2@gmail.com>
@Avasil Avasil merged commit dfde19d into monix:master Jan 14, 2020
@allantl
Copy link
Copy Markdown
Contributor

allantl commented Jan 29, 2020

@Avasil @fchaillou FYI, flatMap is also aliased as concatMap

@fchaillou
Copy link
Copy Markdown
Contributor Author

fchaillou commented Jan 29, 2020 •
edited
Loading

@allantl yes @Avasil mentionned that earlier in the PR, hopefully the type signature and the documentation will prevent confusion between the two !

@Avasil
Copy link
Copy Markdown
Collaborator

Avasil commented Jan 29, 2020

@allantl We discussed it here: #1106 (comment)

Nothing has been released yet so it's not too late if you have an idea for a better name but my head is empty

@allantl
Copy link
Copy Markdown
Contributor

allantl commented Jan 29, 2020

In RxJava, its named as concatMapIterable link. There's also flatMapIterable. What do you guys think about that?

@Avasil
Copy link
Copy Markdown
Collaborator

Avasil commented Jan 29, 2020

Ah, I missed it. Thank you

If it already has a name in RX then we should be consistent and have an alias at the very least
I kinda like mapConcat more than longer names but the confusion is a concern

@fchaillou
Copy link
Copy Markdown
Contributor Author

fchaillou commented Jan 31, 2020

Hey @Avasil,
I feel like i forgot about the eval end evalF version !
I will look into them sometime next week as well.
Fabien

mdedetrich pushed a commit to mdedetrich/monix that referenced this pull request Mar 28, 2020
@fchaillou @Avasil @mdedetrich
Add mapConcat to Observable (monix#1106)
bf5dc7a 
* Add new mapConcat operator on Observable

* Update monix-reactive/shared/src/main/scala/monix/reactive/Observable.scala

Co-Authored-By: Piotr Gawryś <pgawrys2@gmail.com>

Co-authored-by: Piotr Gawryś <pgawrys2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Avasil Avasil Avasil approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fchaillou @Avasil @allantl