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.

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

Accept Duration for all time-based combinators #3098

Merged
merged 10 commits into from
Sep 21, 2022

Conversation

armanbilge
Copy link
Member

Follow-up to #2954.

Comment on lines +290 to 298
private[effect] def handleDuration[A](duration: Duration, ifInf: => A)(
ifFinite: FiniteDuration => A): A =
duration match {
case _: Duration.Infinite => fa
case finite: FiniteDuration => f(finite)
case fd: FiniteDuration => ifFinite(fd)
case Duration.Inf => ifInf
case d =>
throw new IllegalArgumentException(
s"Duration must be either a `FiniteDuration` or `Duration.Inf`, but got: $d")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

While we're here, should we also do something about negative FiniteDurations?

@armanbilge
Copy link
Member Author

Something I'm leery about is that these changes introduce a lot of branches that aren't currently tested by our laws which I think work in terms of FiniteDuration. We should change the laws as well, and if not then at least add specific test coverage for this.

djspiewak
djspiewak previously approved these changes Sep 20, 2022
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

This seems good if we can get it to build. :-)

@armanbilge
Copy link
Member Author

This seems good if we can get it to build. :-)

Heh, I was waiting for a positive sign before continuing :) I'm on it!

@armanbilge armanbilge added this to the v3.4.0 milestone Sep 20, 2022
@djspiewak djspiewak merged commit 8ba8916 into typelevel:series/3.x Sep 21, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants