Testing for NONE log level
#42
Conversation
Tests and code to make them pass for either: * `NONE` as the min level of a logger * `NONE` as the caller's logging level
No need to start Gradle daemon processes for an ephemeral container. No reuse of the daemons after container finishes.
No daemon mode is provided via an env var.
|
Thanks @binkley for helping me to sort out log level checking. I had got myself confused with these things. I wrote out the following today. Does it make sense? Do your changes implement it? I will put it into the documentation soon. Log levels and checkingBackground
Rules
CodeApplicable to both
|
|
I am also unsure what |
| @@ -45,7 +46,7 @@ public interface BaseLogger { | |||
| * Check whether this logger will emit log events at the specified logging | |||
| * level. | |||
| */ | |||
| public fun isLevelEnabled(level: Level): Boolean = minLevel() <= level | |||
| public fun isLevelEnabled(level: Level): Boolean = NONE != level && minLevel() <= level | |||
There was a problem hiding this comment.
IMO this is prettier
public fun isLevelEnabled(level: Level): Boolean = when(level)
{
NONE -> false
else -> minLevel() <= level
}
There was a problem hiding this comment.
NONE is used in SLF4J for example with dynamic changes in log level. I've actually used this in a prod system that was overflowing a downstream system with logging. We changed the log level in prod via JMX to NONE.
There was a problem hiding this comment.
If you keep NONE, I'd update the above docs in the "Rules" section to call out NONE as special.
|
|
||
| public suspend fun log(level: Level, exception: Exception, event: suspend Klogger.() -> Any?) { | ||
| if (isLevelEnabled(level)) emitEvent(level, exception, event()) | ||
| if (!isLevelEnabled(level)) return |
There was a problem hiding this comment.
I think the original is clearer. An early return is only needed if what follows is more complex.
But I won’t go to the barricades over this. I can see the consistency with the earlier, more complex functions.
There was a problem hiding this comment.
Yeah, two reasons:
- Consistency -- as noted
- Personal preference. I like to see early return. It's like a "pre condition", so I know what follows is the real work
| } | ||
|
|
||
| public suspend fun log(level: Level, event: suspend Klogger.() -> Any?) { | ||
| if (isLevelEnabled(level)) emitEvent(level, null, event()) | ||
| if (!isLevelEnabled(level)) return |
|
|
||
| describe("KtLogger") { | ||
| describe("does not log") { |
There was a problem hiding this comment.
Description is incomplete. It should be describe("does not log at level NONE") or similar.
| randomString().let { message -> | ||
| logger.log(eventLevel, message) | ||
|
|
||
| if (!logger.isLevelEnabled(eventLevel)) |
There was a problem hiding this comment.
Why not put if (logger.isLevelEnabled(eventLevel)) and switch the assertions? Using ! is a slight extra impediment to understandability.
There was a problem hiding this comment.
Artifact of refactoring. In Java, IntelliJ warns me about this. Not sure how to enable that for Kotlin support. I'm always of the same view as you just stated. (Another example of my preference for early return, to avoid this situation.)
Several commits together. Key points:
NONEis handled, especially for:public fun isLevelEnabled(level: Level): Boolean = NONE != level && minLevel() <= levellogto skip processing if the log level is disabledbatect.yml(which I added myself previously: I had not noticed theGRADLE_OPTSenv var)