Helpers for creating simple/stub versions of SelfAwareStructuredLogger and LoggerFactory
#682
Conversation
| final def error(t: Throwable)(message: => String): F[Unit] = error(message) | ||
| final def warn(t: Throwable)(message: => String): F[Unit] = warn(message) | ||
| final def info(t: Throwable)(message: => String): F[Unit] = info(message) | ||
| final def debug(t: Throwable)(message: => String): F[Unit] = debug(message) | ||
| final def trace(t: Throwable)(message: => String): F[Unit] = trace(message) |
There was a problem hiding this comment.
I couldn't decide whether these should be final, and in fact wasn't consistent within this PR. Any thoughts?
There was a problem hiding this comment.
I don't think there's much reason to make these final. These are just convenient implementation mix-ins, there shouldn't be libraries out in the wild that specifically ask for a Fallback and expect it to work a certain way...
| @@ -28,6 +28,14 @@ trait ErrorLogger[F[_]] { | |||
| } | |||
|
|
|||
| object ErrorLogger { | |||
| trait Fallback[F[_]] extends ErrorLogger[F] { _: MessageLogger[F] => | |||
There was a problem hiding this comment.
I'm having a hard time understanding what the purpose of Fallback[F[_]] is.
Is it a convenience mixin that ignores any Throwables in the signatures of ErrorLogger[F[_]]?
Is it intended for end-users?
Seeing how it's used here I'd rather have it just be an implementation detail of the SelfAwareStructuredLogger.liftF because otherwise it increases the API surface area of the library quite a lot 😅
There was a problem hiding this comment.
Good question. I feel like it's a useful thing to have available but I don't have a very strong, specific answer to your question so yes, probably best to leave it as an implementation detail for now at least.
There was a problem hiding this comment.
I'd prefer making all the helper traits private[log4cats] (or whatever is best for bincompat, I am really bad at this part 😬). We can always make them public later with little effort and in a minor version.
There was a problem hiding this comment.
private[log4cats] doesn't really protect bincompat, but it protects source compat, and we can grant a MiMa waiver if we're sure nothing else used it.
In multi-repo projects like http4s, it's unsafe to waive away package privates, because usages could be in another repo. Here, it's probably safe: nothing other than this project should be in the log4cats namespace.
Either way, I'd agree locking it down is safer for libraries this low in the hierarchy.
|
Did a courtesy merge because we let this rot when it was already close. I didn't follow all the spidering conversations, but I think this was ready if we made the helper traits private to reduce the bincompat surface? |
If this is still a pain point, #854 may help and would benefit from your input |
This came up in discussion on http4s/http4s#6629 about whether the http4s logging middleware should continue to allow the user to provide
String => F[Unit]instead ofLoggerFactory- allowing it adds implementation complexity, but currently it takes a tremendous amount of boilerplate to lift aString => F[Unit]into aLoggerFactory. cc @armanbilgeNames and access modifiers subject to bikeshedding of course.