Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add methods to get finite duration from duration #8832

Closed
wants to merge 2 commits into from
Closed

add methods to get finite duration from duration #8832

wants to merge 2 commits into from
+5 −6

Conversation

NavidJalali
Copy link
Contributor

@NavidJalali NavidJalali commented May 9, 2024 •
edited by jdegoes

Fix #8797

/claim #8797

@kyri-petrou
Copy link
Contributor

@NavidJalali just FYI, you'll need to add /claim #8797 to the PR description if you want to receive payment for bounty

@algora-pbc algora-pbc bot mentioned this pull request May 12, 2024
Open
@algora-pbc Algora PBC
Copy link

algora-pbc bot commented May 12, 2024

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay.

jdegoes
jdegoes reviewed May 12, 2024
View reviewed changes
core/shared/src/main/scala/zio/Duration.scala Outdated
case _ => Some(ScalaFiniteDuration(duration.toNanos, TimeUnit.NANOSECONDS))
}

def asFiniteDurationUnsafe: ScalaFiniteDuration = duration match {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to throw, because if you look at "Infinity", it's just a "really big" (biggest possible) finite duration. So I think this can and should be a total function.

Copy link
Member

Choose a reason for hiding this comment

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

Btw I would also consider simply changing def asScala: ScalaDuration to def asScala: ScalaFiniteDuration. This way no new methods are necessary and it should be backward compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

hearnadam
hearnadam reviewed May 20, 2024
View reviewed changes
core-tests/shared/src/test/scala/zio/DurationSpec.scala
@@ -154,7 +154,7 @@ object DurationSpec extends ZIOBaseSpec {
assert(Duration.Infinity.isZero)(equalTo(false))
},
test("It converts into the infinite s.c.d.Duration") {
assert(Duration.Infinity.asScala)(equalTo(ScalaDuration.Inf: ScalaDuration))
assert(Duration.Infinity.asScala)(equalTo(ScalaFiniteDuration(Long.MaxValue, TimeUnit.NANOSECONDS)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I will note this is likely not preferable if users are using APIs which special case for Duration.Inf...

@jdegoes
Copy link
Member

jdegoes commented Jun 4, 2024

Mima is failing with:

[error] zio: Failed binary compatibility check against dev.zio:zio_2.12:2.0.22! Found 3 potential problems (filtered 37)
[error]  * extension static method asScala$extension(java.time.Duration)scala.concurrent.duration.Duration in class zio.DurationOps has a different result type in current version, where it is scala.concurrent.duration.FiniteDuration rather than scala.concurrent.duration.Duration
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("zio.DurationOps.asScala$extension")
[error]  * method asScala()scala.concurrent.duration.Duration in class zio.DurationOps has a different result type in current version, where it is scala.concurrent.duration.FiniteDuration rather than scala.concurrent.duration.Duration
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("zio.DurationOps.asScala")
[error]  * extension method asScala$extension(java.time.Duration)scala.concurrent.duration.Duration in object zio.DurationOps has a different result type in current version, where it is scala.concurrent.duration.FiniteDuration rather than scala.concurrent.duration.Duration
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("zio.DurationOps.asScala$extension")

I think we can safely ignore this as it would only affect us if someone made a subtype of the extension methods class... which is...UNLIKELY. Can you add to mima exclusion list and re-open when build is passing?

Otherwise, looks good to merge!

@jdegoes jdegoes closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdegoes jdegoes jdegoes left review comments

@hearnadam hearnadam hearnadam approved these changes

Assignees
No one assigned
Labels
🙋 Bounty claim
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add conversion to FiniteDuration to zio.duration
4 participants
@NavidJalali @kyri-petrou @jdegoes @hearnadam