Remove dependency on logback-classic and scala-logging where unnecessary#3471
Conversation
hygt
force-pushed
the
fix/remove-scala-logging
branch
from
a1a4895 to
a5cc585
Compare
hygt
force-pushed
the
fix/remove-scala-logging
branch
from
a5cc585 to
4b4f1c9
Compare
| val circeGenericExtras = "0.14.3" | ||
| val circeYaml = "0.15.1" | ||
| val helidon = "4.0.0" | ||
| val helidon = "4.0.3" |
There was a problem hiding this comment.
nitpick: Could we keep this change for another PR?
|
|
||
| logger.debug(s"Generated protobuf structure: [$renderedProto]") | ||
| logger.info(s"Writing proto file to the path [$path]") | ||
| logger.debug("Generated protobuf structure: [{}]", renderedProto) |
There was a problem hiding this comment.
suggestion: To take advantage of slf4sj's deferred evaluation when formatting, this should be
logger.debug("Generated protobuf structure: [{}]", renderer.render(proto))
There was a problem hiding this comment.
we're using slf4j, not slf4s, so if at all, this would have to be a lambda to be lazily evaluated?
There was a problem hiding this comment.
Oh right, this has to be a lambda. slf4j has API for that, I think only in the 2.0's fluent API https://www.slf4j.org/manual.html#fluent
There was a problem hiding this comment.
In any case renderedProto is called again on line 28, so I don't think it matters?
There was a problem hiding this comment.
Thanks, I didn't notice that. Maybe we can keep the standard string interpolation then?
| private def debugLog(msg: String, exOpt: Option[Throwable]): Unit = exOpt match { | ||
| case Some(e) => log.log(Level.FINE, msg, e) | ||
| case None => log.log(Level.FINE, msg) | ||
| case Some(e) => log.debug(msg, e) |
There was a problem hiding this comment.
suggestion: Shouldn't this method check logger.isDebugEnabled() as well?
| result match { | ||
| case Right(response) => | ||
| logger.info(s"Request event ${event.requestId} completed successfully") | ||
| logger.info("Request event {} completed successfully", event.requestId) |
There was a problem hiding this comment.
nitpick: While the slf4j's formatter's deferred evaluation can help to avoid evaluating expensive expressions, I don't think it should be used in all cases. Here we have a simple expression (event.requestId) and INFO level, so it will always be evaluated anyway. Using interpolated string where possible makes the message much more readable in the code IMO. (This comment applies to other usages where I don't see a clear gain, like logger.error calls above and below).
|
Thanks a lot @hygt! |
|
Thanks! :) |
3 participants
Fixes #3464