ReactiveX / RxScala

RxScala – Reactive Extensions for Scala – a library for composing asynchronous and event-based programs using observable sequences
Apache License 2.0
888 stars 110 forks source link

flatMapTry operators #221

Closed rvanheest closed 7 years ago

rvanheest commented 7 years ago

After playing around with the additions from #210 I found myself in the situation where I wanted to flatMap a Try into an Observable. The standard solution since #210 would be to write flatMap(i -> Observable.from(someTryResultingFunction(i))). This however gets tedious after a while. Given that there are flatMapIterable overloads as well, I thought I might as well add a flatMapTry operator. Now the syntax becomes flatMapTry(i => someTryResultingFunction(i)) (or flatMapTry(someTryResultingFunction) to be even more fancy).

For now I have naively implemented flatMapTry by wrapping the Try in an Observable and calling flatMap, but maybe someone can give me some pointers of how to implement it in a better way (if needed?).

dhoepelman commented 7 years ago

An alternative would be a (user defined) implicit conversion with flatMap(someTryResultingFunction(_)) (this is how I solved it in a project where I frequently flatMapped to something that produces a Future)

If this is deemed useful, flatMapFuture could be considered as well, for parity with the overloads ofObservable.from: Iterable, Try and Future

Personally I think it's a tad too niche.

samuelgruetter commented 7 years ago

I agree with @dhoepelman : If you're tired of writing Observable.from(someTryResultingFunction(i)) all the time instead of just someTryResultingFunction(i), that's a typical use case for Scala's implicit conversions. I would claim that using implicit conversions here is much more Scala-idiomatic than blowing up the already-huge API of Observable with 2 more (or even 4, with Future) methods.

rvanheest commented 7 years ago

I see what you mean, and indeed that would look nicer and all that... I did this with the latest commit by adding an implicit conversion to package.scala from Try[T] to Observable[T]. This removes the need for a separate flatMapTry(With).

However, this also extends the Try API with all the Observable operators. Now it would (for example) be possible to do Try(5).withLatestFrom(Try(6))(_ + _). The question is whether or not this is desirable? A problem with this, however, is that some API calls are not available: Try(5).flatMap(i => Observable.just(1)) is not valid, since flatMap already exists in the Try API.

I'm not sure if this is something want. LMKWYT!

samuelgruetter commented 7 years ago

I think there was a slight misunderstanding here: I believe that no change to RxScala is needed, and that users who are tired of writing Observable.from(someTryResultingFunction(i)) all the time would just put the following line into one of their own files:

implicit def tryToObservable[T](tt: Try[T]): Observable[T] = Observable.from(tt)

Putting it into package.scala is dangerous, because users who import rx.lang.scala._ will import this even if they don't want it. So if we really want RxScala to provide this, it should be inside another object which has to be explicitly imported. (Note that the implicit classes which are also in package.scala are not dangerous, because they only become active when the user explicitly writes .toObservable, whereas tryToObservable becomes active whenever the expected type is Observable, and the actual type is Try).

And for the record, there are actually three ways to write it:

flatMap(i => Observable.from(someTryResultingFunction(i)))
flatMap(i => someTryResultingFunction(i)) // with implicit def tryToObservable
flatMap(i => someTryResultingFunction(i).toObservable) // with implicit class TryToObservable

The 3rd has not mentioned yet, and is my personal favorite :wink:

rvanheest commented 7 years ago

That was exactly my feeling regarding putting the implicit conversion in package.scala. It ain't good! I guess you prefer the xxx.toObservable way more, so I'll close the PR. I even hadn't thought about that option, even though I introduced it myself...