Support Twitter futures #833

mijicd · 2019-05-06T19:15:52Z

Closes #718.

LGLO

Are you able to provide some simple tests? Otherwise I could add them if you are busy.

LGLO · 2019-05-07T12:48:55Z

.travis.yml

@@ -36,7 +36,7 @@ jobs:
    - &scaladoc
      stage: microsite
      name: "Generate Scaladoc"
-      script: sbt ++$TRAVIS_SCALA_VERSION coreJVM/doc coreJS/doc interopCatsJVM/doc interopCatsJS/doc interopFutureJVM/doc interopFutureJS/doc interopScalaz7xJVM/doc interopScalaz7xJS/doc interopMonixJVM/doc interopMonixJS/doc interopJavaJVM/doc interopReactiveStreamsJVM/doc
+      script: sbt ++$TRAVIS_SCALA_VERSION coreJVM/doc coreJS/doc interopCatsJVM/doc interopCatsJS/doc interopFutureJVM/doc interopScalaz7xJVM/doc interopScalaz7xJS/doc interopMonixJVM/doc interopMonixJS/doc interopJavaJVM/doc interopReactiveStreamsJVM/doc interopTwitterJVM/doc


is interopFutureJS/doc gone by accident or for purpose? WHy?

It was on purpose, as I think there isn't (wasn't?) such interop atm. I can reintroduce it though.

LGLO · 2019-05-07T12:56:22Z

build.sbt

    interopScalaz7xJVM,
    interopScalaz7xJS,
    interopJavaJVM,
    interopReactiveStreamsJVM,
-//  benchmarks,
+    interopTwitterJVM,
+    benchmarks,


What was the reason to have benchmarks commented out?

No idea. It was commented out for a long time now. As far as I can see it's commented out together with Monix during "death of TF" PR :).

I remember it :D , I wondered why it was already commented before

mijicd · 2019-05-07T13:08:43Z

@LGLO Thanks for the review. I will add the tests.

docs/interop/twitter.md

mijicd · 2019-05-07T16:55:25Z

@LGLO added docs to sidebar.

regiskuckaertz · 2019-05-07T17:19:34Z

interop-twitter/jvm/src/main/scala/scalaz/zio/interop/twitter.scala

+package object twitter {
+  implicit class TaskObjOps(private val obj: Task.type) extends AnyVal {
+    final def fromTwitterFuture[A](future: => Future[A]): Task[A] =
+      Task.effectAsync { cb =>


You will also need to handle interruption from the outside with future.raise

Any suggestions on the approach? From what I see, interrupt handler is available only for Promise, and raising is propagated to producers. There are facilities for making it non-interruptible, but I don't think it's the desired behavior :).

I've made some change here

Interruption handler in promise is where you need to accept downstream cancellation, here you need to signal cancellation by using future.raise.

Besides that fromTwitterFuture should really accept Task[Future[A]] (similar to the scala version fromFuture).

As @yaroot did 😄

@yaroot fromFuture doesn't accept Task, as far as I can see, it accepts ExecutionContext => scala.concurrent.Future[A]. I'll take a look at the solution you suggested, or, I can retract the PR and let you submit the solution, if it's the preferred approach :).

@mijicd no worries, I should have been clearer 😅 Yes the Task[Future[_]] thing is rather hard to find. By name seems fine to me.

Great work 💯

Thanks. I wasn't aware of this, sorry @yaroot. That being said, I see there are multiple ways we are "playing" with futures: thunks, by-name, wrapping them in IO... not sure that we need all of them.

@mijicd great work. Yes they're all thunks, but IO has nice properties and easier to reason about (much easier than by-name), not to mention keeping the API in line with existing ones.

In the latest commit (https://github.com/scalaz/scalaz-zio/blob/dbc12eef5809aa34e2eae8b17e13bdcdfd7d42ed/interop-twitter/jvm/src/main/scala/scalaz/zio/interop/twitter.scala#L10-L15) the future is actually evaluated twice, if you change the variable lazy val future = ... to def future = ... or inline it like Task.fromTwitterFuture(Future.sleep(..)) the test will be broken, which would be a nightmare to debug in application code.

Agreed. Let's update this one to task then!

Instead of providing convertors, twitter-interop will be implemented as API-compatible entity backed by ZIO, as its done with scala futures.

This reverts commit e5ce90d1e0e3dfdd610b44c5860779a844f0a900.

regiskuckaertz · 2019-05-08T12:54:54Z

.scalafmt.conf

@@ -1,3 +1,4 @@
+version = "2.0.0-RC6"


Is this necessary? It's added by metals, isn't it? If possible I'd move that out of the way.

Unfortunately, it is. All hell breaks loose if you remove it :).

regiskuckaertz

It looks good to me, thank you very much for your contribution! I've not reviewed for some time so I'll let @LGLO give a final 👍

mijicd · 2019-05-08T18:56:06Z

@LGLO @regiskuckaertz in the end I adjusted the interface as suggested above by @yaroot. Take one more look, and hopefully the build will be green (this flaky reactive test is kinda annoying).

LGLO

LGTM!

mijicd requested a review from jdegoes May 6, 2019 19:15

mijicd force-pushed the twitter-futures-interop branch 2 times, most recently from 49e2221 to 3d1677f Compare May 7, 2019 11:18

LGLO reviewed May 7, 2019

View reviewed changes

mijicd requested a review from LGLO May 7, 2019 13:32

LGLO reviewed May 7, 2019

View reviewed changes

docs/interop/twitter.md Show resolved Hide resolved

mijicd requested a review from LGLO May 7, 2019 16:54

regiskuckaertz reviewed May 7, 2019

View reviewed changes

mijicd added 19 commits May 8, 2019 08:41

Create interop subproject

4ed1287

Rename monix interop package

44d6b14

Aggregate monix-interop and benchmarks

b137c02

Drop twitter-util dependency

a9aa71b

Instead of providing convertors, twitter-interop will be implemented as API-compatible entity backed by ZIO, as its done with scala futures.

Update scalafmt to 2.0.0

595d48b

Revert "Drop twitter-util dependency"

ad3c1a7

This reverts commit e5ce90d1e0e3dfdd610b44c5860779a844f0a900.

Add project template

c98e2ff

Improve monix interop naming

58d4a36

Convert twitter's future to zio Task

4a6a025

Fix errors in monix tut page

d754b82

Draft tut docs

ff1b505

Document interop usage

695aa0d

Rename to avoid collision

4ec01ef

Fix tut errors

0bb7c03

Migrate docs from to the new format

2703c56

Update scaladoc creation

9ec75e7

Test the conversion

f5fc7bc

Add Twitter section to sidebar

a7ee13a

Handle interrupts

dbc12ee

mijicd force-pushed the twitter-futures-interop branch from d59384f to dbc12ee Compare May 8, 2019 11:59

regiskuckaertz reviewed May 8, 2019

View reviewed changes

Add copyright header

99e6514

regiskuckaertz previously approved these changes May 8, 2019

View reviewed changes

Replace by-name with Task[_]

729a293

mijicd dismissed regiskuckaertz’s stale review via 729a293 May 8, 2019 13:48

LGLO previously approved these changes May 8, 2019

View reviewed changes

Decrease timeouts by factor of 10

709453f

mijicd dismissed LGLO’s stale review via 709453f May 8, 2019 20:41

LGLO approved these changes May 9, 2019

View reviewed changes

regiskuckaertz approved these changes May 9, 2019

View reviewed changes

regiskuckaertz merged commit ce37ecd into zio:master May 9, 2019

mijicd deleted the twitter-futures-interop branch May 9, 2019 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Twitter futures #833

Support Twitter futures #833

mijicd commented May 6, 2019

LGLO left a comment

LGLO May 7, 2019

mijicd May 7, 2019

LGLO May 7, 2019

mijicd May 7, 2019

wi101 May 7, 2019

mijicd commented May 7, 2019

mijicd commented May 7, 2019

regiskuckaertz May 7, 2019

mijicd May 8, 2019 •

edited

yaroot May 8, 2019 •

edited

regiskuckaertz May 8, 2019

mijicd May 8, 2019

regiskuckaertz May 8, 2019

mijicd May 8, 2019 •

edited

yaroot May 8, 2019 •

edited

mijicd May 8, 2019

mijicd May 8, 2019

regiskuckaertz May 8, 2019

mijicd May 8, 2019

regiskuckaertz May 8, 2019

regiskuckaertz left a comment

mijicd commented May 8, 2019

LGLO left a comment

Support Twitter futures #833

Support Twitter futures #833

Conversation

mijicd commented May 6, 2019

LGLO left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mijicd commented May 7, 2019

mijicd commented May 7, 2019

Choose a reason for hiding this comment

mijicd May 8, 2019 • edited

Choose a reason for hiding this comment

yaroot May 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mijicd May 8, 2019 • edited

Choose a reason for hiding this comment

yaroot May 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regiskuckaertz left a comment

Choose a reason for hiding this comment

mijicd commented May 8, 2019

LGLO left a comment

Choose a reason for hiding this comment

mijicd May 8, 2019 •

edited

yaroot May 8, 2019 •

edited

mijicd May 8, 2019 •

edited

yaroot May 8, 2019 •

edited