Skip to content

Add GenTemporal#cachedRealTime - #2941

Merged
djspiewak merged 7 commits into
typelevel:series/3.xfrom
brendanmaguire:cached-real-time
Apr 10, 2022
Merged

Add GenTemporal#cachedRealTime#2941
djspiewak merged 7 commits into
typelevel:series/3.xfrom
brendanmaguire:cached-real-time

Conversation

@brendanmaguire

Copy link
Copy Markdown
Contributor

This method provides a nested effect which calculates time using a cached offset and monotonic. This is much faster than calling realTime repeatedly and is useful for some applications where precise time is not required

This method provides a nested effect which calculates time using a cached offset and `monotonic`. This is much faster than calling `realTime` repeatedly and is useful for some applications where precise time is not required
@brendanmaguire

Copy link
Copy Markdown
Contributor Author

As described in the discussion in #2416 , and in particular this comment.

@brendanmaguire

Copy link
Copy Markdown
Contributor Author

@djspiewak & @armanbilge , would you mind taking a look. It seems messy to me but I'm not sure how to clean it up. Suggestions welcome.

@djspiewak djspiewak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't seem too messy to me. :-) It's a surprisingly involved effect. The big thing that would clean it up is just leveraging the syntax.

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
brendanmaguire and others added 3 commits April 5, 2022 09:28
@brendanmaguire

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @djspiewak 🙂 Changes pushed.

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated

@djspiewak djspiewak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really minor required change and some stylistic nits, but this is looking very good!

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala
brendanmaguire and others added 2 commits April 6, 2022 07:40
@brendanmaguire

Copy link
Copy Markdown
Contributor Author

Thanks again for the feedback @djspiewak . I will start using mapN more often in my own code after this 🙂 Changes made.

@djspiewak

Copy link
Copy Markdown
Member

Reopening to get a new merge commit for CI

@djspiewak djspiewak closed this Apr 7, 2022
@djspiewak djspiewak reopened this Apr 7, 2022
}
else
pure(timeNow + offset)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can this be rewrite to for expression?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It could be but we tend to avoid them in most places since their desugaring is both unintuitive (creating an extra map in most cases) and not the same across Scala 2 and 3 (due to our use of a compiler plugin on the former). Perhaps more relevantly though, I don't think a for-comprehension would help much here with readability, since you would still have the testing due to the F[F[FiniteDuration]] result type.

@djspiewak
djspiewak merged commit f51d87d into typelevel:series/3.x Apr 10, 2022
@brendanmaguire
brendanmaguire deleted the cached-real-time branch May 23, 2022 15:36
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