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

Allow adding effect's contextual environment to the log context #800

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iRevive
Copy link

@iRevive iRevive commented Oct 26, 2023

Closes #461. Some ideas are based on #676.

build.sbt Outdated
@@ -47,7 +48,8 @@ lazy val core = crossProject(JSPlatform, JVMPlatform, NativePlatform)
name := "log4cats-core",
libraryDependencies ++= Seq(
"org.typelevel" %%% "cats-core" % catsV,
"org.typelevel" %%% "cats-effect-std" % catsEffectV
"org.typelevel" %%% "cats-effect-std" % catsEffectV,
"org.typelevel" %%% "cats-mtl" % catsMtlV
Copy link
Author

Choose a reason for hiding this comment

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

The drawback of having this functionality in the core module.

I can move this code into log4cats-extras module. It could be handy if we want to provide some 'non-trivial' functionality: invocation positions (#525), etc.

Copy link
Member

Choose a reason for hiding this comment

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

Just an idea, maybe it's worth creating a log4cats-mtl module then?

Copy link
Author

Choose a reason for hiding this comment

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

Valid point

@iRevive iRevive force-pushed the contextual-logger branch 6 times, most recently from d0bf3e7 to 8f5d723 Compare October 30, 2023 10:58
@iRevive
Copy link
Author

iRevive commented Oct 30, 2023

I made log4cats-mtl module.

Should I set an upcoming release via tlVersionIntroduced := List("2.12", "2.13", "3").map(_ -> "2.7.0").toMap to suppress MiMa errors in the module? Or should I bump the base version instead: tlBaseVersion := 2.7?

@armanbilge
Copy link
Member

If we are making a log4cats-mtl module, we should consider that cats-mtl may become a direct dependency of Cats Effect via typelevel/cats-effect#3385. Although that might be Cats Effect core only, and not the std module we're using here.

Just want to make sure that we think about the migration path in the case MTL ends up on the log4cats-core classpath (an acceptable migration path may be to accept the less-than-ideal situation as a historical artifact :)

@iRevive
Copy link
Author

iRevive commented Oct 30, 2023

Is there any ETA? It would be nice to have Local as a part of cats-effect.

@armanbilge
Copy link
Member

It could be as early as Cats Effect 3.6. I think last time that we discussed it Daniel was pretty convinced.

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.

FiberLocal / IOLocal logger wrapper
3 participants