-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove usages of rxscala and replace with Java based code. #4744
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4744 +/- ##
===========================================
+ Coverage 42.38% 78.31% +35.92%
===========================================
Files 191 198 +7
Lines 8564 8830 +266
Branches 609 623 +14
===========================================
+ Hits 3630 6915 +3285
+ Misses 4934 1915 -3019
Continue to review full report at Codecov.
|
dec6c2e
to
ed34677
Compare
@@ -34,8 +34,12 @@ private[cosmosdb] trait RxObservableImplicits { | |||
* @return the head result of the [[Observable]]. | |||
*/ | |||
def head(): Future[T] = { | |||
def toHandler[T](f: (T) => Unit): Action1[T] = new Action1[T] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be simplify it further with SAM. Also using P
instead of T
as Intellij warned of shadowing here
def toHandler[P](f: (P) => Unit): Action1[P] = (t: P) => f(t)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! You can tell my scala is a little on the rusty side 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to get rid of a third party dependency which is not maintained. Thanks @markusthoemmes !
@chetanmeh told me to merge myself so here we go! |
Why not using https://github.com/reactor/reactor-scala-extensions? |
@sinwe Not sure if that matches up with our usage of reactivex here. I mostly wanted to get rid of the one dependency I removed here without changing the code too much, so this was the quickest way. |
@markusthoemmes if your usage of rxscala is not that much, it's not worth to add additional dependencies. However if you use rxscala extensively and you want to keep track as it progress, e.g: scala 2.13, etc, then it's better you invest some time to replace the dependency completely with another one. There are many libraries similar to rxscala, e.g: Monix, Reactor, etc. |
The code here is purely adaptor code for the CosmosDB lib though, so we have no choice other than using what they use. |
Then you've done the right thing :) |
Description
RxScala has been abandoned: ReactiveX/RxScala#244. To unblock the move to Scala 2.13, this removes all usages of it and replaces it with Java interoperable code.
Related issue and scope
Ref #4741
Checklist: