-
Notifications
You must be signed in to change notification settings - Fork 44
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
[core] Log improvements #783
Conversation
@@ -2,10 +2,28 @@ package kyo | |||
|
|||
import kyo.internal.LogPlatformSpecific | |||
|
|||
final case class Log(unsafe: Log.Unsafe) extends AnyVal: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow the pattern of other effects with a Log
and Log.Unsafe
pair
/** Logging utility object for Kyo applications. */ | ||
object Log extends LogPlatformSpecific: | ||
|
||
private val local = Local.init[Unsafe](unsafe) | ||
enum Level: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More high-level handling of log levels
* The result of executing the effect with the console logger | ||
*/ | ||
def withConsoleLogger[A, S](name: String = "kyo.logs", level: Level = Level.Debug)(v: A < S)(using Frame): A < S = | ||
let(Log(Unsafe.ConsoleLogger(name, level)))(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convenience methods to redirect logs to the console. Note how ConsoleLogger
also supports specifying a log level.
@@ -37,89 +91,82 @@ object Log extends LogPlatformSpecific: | |||
def warn(msg: => String, t: => Throwable)(using frame: Frame, allow: AllowUnsafe): Unit | |||
def error(msg: => String)(using frame: Frame, allow: AllowUnsafe): Unit | |||
def error(msg: => String, t: => Throwable)(using frame: Frame, allow: AllowUnsafe): Unit | |||
|
|||
def safe: Log = Log(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I see the value in this to be honest. It's unlike other designs where the APIs are hidden behind an opaque type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for adding it was so it's possible to have Log.get
and Local.use
methods. It'd be strange to add them with a Log.Unsafe
type. That's something useful because computations can get the Log
instance once and avoid further Local
lookups. Another use case is passing the instance to some unsafe execution context like library integrations. While not essential, adding it keeps the consistency with similar APIs and provides these benefits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the local store the Safe variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've changed it to use safe variant
private val local = Local.init[Unsafe](unsafe) | ||
enum Level: | ||
case Trace, Debug, Info, Warn, Error | ||
def enabled(minLevel: Level): Boolean = this.ordinal >= minLevel.ordinal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this have any issues if we need to add/remove levels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it be based on an Int
with some padding so we can make changes in the future
Following up on #780 (comment)