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

Replace all occurrences of com.twitter.util.Duration with scala.concurrent.Duration #985

Closed
sergeykolbasov opened this Issue Sep 16, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@sergeykolbasov
Member

sergeykolbasov commented Sep 16, 2018

As it was raised during the discussion around #979

It should ease the use of our API since most of the people use scala duration anyway.

@vkostyukov vkostyukov added this to the Finch 1.0 milestone Sep 16, 2018

@prayagupd

This comment has been minimized.

Show comment
Hide comment
@prayagupd

prayagupd Sep 21, 2018

Contributor

Curious what's the difference between duration api? Seems following are two places where twitter.util.Duration is being used

  1. final def awaitOutput(d: Duration = Duration.Top): Option[Try[Output[A]]] = this match {

  2. def apply(req: Request, timeout: Duration = Duration.fromSeconds(10)): Response =

What about com.twitter.util.Await.result taking twitter.duration? Transform scala.Duration to twitter.Duration or use scala.Await?

case EndpointResult.Matched(_, _, out) => Some(Await.result(out.liftToTry.run, d))

I am interested to contribute, that way will also get introduced to building/testing finch bit more.

Contributor

prayagupd commented Sep 21, 2018

Curious what's the difference between duration api? Seems following are two places where twitter.util.Duration is being used

  1. final def awaitOutput(d: Duration = Duration.Top): Option[Try[Output[A]]] = this match {

  2. def apply(req: Request, timeout: Duration = Duration.fromSeconds(10)): Response =

What about com.twitter.util.Await.result taking twitter.duration? Transform scala.Duration to twitter.Duration or use scala.Await?

case EndpointResult.Matched(_, _, out) => Some(Await.result(out.liftToTry.run, d))

I am interested to contribute, that way will also get introduced to building/testing finch bit more.

@vkostyukov

This comment has been minimized.

Show comment
Hide comment
@vkostyukov

vkostyukov Sep 25, 2018

Member

Hey @prayagupd! Thanks for looking into this!

Yeah, we'd need to convert from Scala's duration to Twitter's duration before passing to Await.result. Said conversion will go away once we merge in cats-effect integration (see #979).

Member

vkostyukov commented Sep 25, 2018

Hey @prayagupd! Thanks for looking into this!

Yeah, we'd need to convert from Scala's duration to Twitter's duration before passing to Await.result. Said conversion will go away once we merge in cats-effect integration (see #979).

@vkostyukov

This comment has been minimized.

Show comment
Hide comment
@vkostyukov

vkostyukov Oct 2, 2018

Member

Also filed a ticket to get rid of c.t.u.Try: #989.

Member

vkostyukov commented Oct 2, 2018

Also filed a ticket to get rid of c.t.u.Try: #989.

@vkostyukov

This comment has been minimized.

Show comment
Hide comment
@vkostyukov

vkostyukov Oct 8, 2018

Member

Fixed in #985.

Member

vkostyukov commented Oct 8, 2018

Fixed in #985.

@vkostyukov vkostyukov closed this Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment