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

"migrate" to cats effect 3 #305

Merged
merged 4 commits into from
Apr 23, 2021
Merged

"migrate" to cats effect 3 #305

merged 4 commits into from
Apr 23, 2021

Conversation

b3nk3i
Copy link
Contributor

@b3nk3i b3nk3i commented Apr 14, 2021 •
edited
Loading

removed dependency to io.monix which is not yet available for CE3..

@kubukoz
Copy link

kubukoz commented Apr 21, 2021

@LukaJCB

@kubukoz
Copy link

kubukoz commented Apr 21, 2021

What was the monix dependency for? The build seems fine without it.

@kubukoz kubukoz mentioned this pull request Apr 21, 2021
Closed
@TimWSpence
Copy link

If we could get this released I would be immensely grateful :) Let me know if I can help in any way

@b3nk3i
Copy link
Contributor Author

b3nk3i commented Apr 22, 2021

What was the monix dependency for? The build seems fine without it.

it was used as example in the doc : policies.md

@kubukoz
Copy link

kubukoz commented Apr 22, 2021 •
edited
Loading

ah, I see it now.

If we want to show another effect being used, we can use a monad transformer or ZIO (zio-interop-cats has been released for CE3)

LukaJCB
LukaJCB approved these changes Apr 22, 2021
View reviewed changes
Copy link
Collaborator

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

LGTM

@kubukoz
Copy link

kubukoz commented Apr 22, 2021 •
edited
Loading

Wanna merge? :D

Edit: noticed the todos.

kubukoz
kubukoz suggested changes Apr 22, 2021
View reviewed changes
build.sbt Outdated
@@ -134,7 +134,7 @@ val docs = project
"org.typelevel" %% "kind-projector" % "0.11.0" cross CrossVersion.full
),
libraryDependencies ++= Seq(
"io.monix" %%% "monix" % "3.1.0"
//"io.monix" %%% "monix" % "3.3.0" // TODO update to version compatible cats effect 3
Copy link

Choose a reason for hiding this comment

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

Let's just drop it I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropped

modules/core/shared/src/main/scala/retry/Sleep.scala
@@ -10,6 +11,6 @@ trait Sleep[M[_]] {
object Sleep {
def apply[M[_]](implicit sleep: Sleep[M]): Sleep[M] = sleep

implicit def sleepUsingTimer[F[_]](implicit timer: Timer[F]): Sleep[F] =
(delay: FiniteDuration) => timer.sleep(delay)
implicit def sleepUsingTemporal[F[_]](implicit t: Temporal[F]): Sleep[F] =
Copy link

Choose a reason for hiding this comment

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

I'm starting to think Sleep should just be replaced with Temporal altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see, replacing Sleep by Temporal has a bigger impact:
given that Temporal[F[_]] = GenTemporal[F, Throwable]

You end up with MonadError[F, Throwable] following the hierarchy GenTemporal[F, E] ... GenConcurrent[F, E] ... GenSpawn[F, E] ... MonadCancel[F, E] ... MonadError[F, E]

Then Temporal[F] will clash with internals based on MonadError[F, E], though it seems that GenTemporal[F, E] solves it but the change propagates up to public function.. not sure if that's desired.. I think it's probably better to try it in a separate PR

Copy link

Choose a reason for hiding this comment

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

Fair enough, these are good points :)

build.sbt Outdated
val catsEffectVersion = "2.3.1"
val catsMtlVersion = "1.1.1"
val catsVersion = "2.5.0"
val catsEffectVersion = "3.0.2"
Copy link

Choose a reason for hiding this comment

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

You can use 3.1.0 now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

version updated

build.sbt Outdated
val catsMtlVersion = "1.1.1"
val catsVersion = "2.5.0"
val catsEffectVersion = "3.0.2"
val catsMtlVersion = "1.1.3"
Copy link

Choose a reason for hiding this comment

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

1.2.0 if you want to be on latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

version updated

kubukoz
kubukoz approved these changes Apr 22, 2021
View reviewed changes
Copy link

@kubukoz kubukoz left a comment

Choose a reason for hiding this comment

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

LGTM

@LukaJCB
Copy link
Collaborator

LukaJCB commented Apr 23, 2021

Thanks a bunch!

@LukaJCB LukaJCB merged commit 65562e6 into cb372:master Apr 23, 2021
@b3nk3i b3nk3i deleted the cats-effect-3 branch April 23, 2021 12:06
@TimWSpence TimWSpence mentioned this pull request Apr 23, 2021
Closed
@kubukoz
Copy link

kubukoz commented Apr 23, 2021

Can we have a milestone for this?

@b3nk3i
Copy link
Contributor Author

b3nk3i commented Apr 26, 2021

Can we have a milestone for this?

The build is failing on a test based on Future.. could be a sporadic failure as it passed on the PR (also locally), can we retry the build ? If it fails again maybe increasing that test timeout will help

@cb372
Copy link
Owner

cb372 commented May 7, 2021

I'm late to the party but LGTM 😄 Thanks for the PR.

Thanks to @LukaJCB this has just been released as v3.0.0.

@cb372 cb372 mentioned this pull request May 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukaJCB LukaJCB LukaJCB approved these changes

@kubukoz kubukoz kubukoz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@b3nk3i @kubukoz @TimWSpence @LukaJCB @cb372