Debug logger #207
Debug logger #207
Conversation
|
|
||
| string message; | ||
| var values = state as ILogValues; | ||
| if (formatter != null) |
There was a problem hiding this comment.
That attribute throws if the value is null, here it is actually optional.
|
Updated |
| return; | ||
| } | ||
|
|
||
| message = $"{ logLevel }: {message}"; |
There was a problem hiding this comment.
take a look at the following recently merged in external PR and see if we should do something like that for log level:
400e191
There was a problem hiding this comment.
nit: extra whitespace in the placeholders here.
There was a problem hiding this comment.
@kichalla that doesn't make as much sense here as we're passing the _name through to the underlying Debug.WriteLine call as the category, so we're not controlling the left-hand side "column" width like we do in the console logger.
|
Looks good to me. |
victorhurdugaci
force-pushed
the
victorhu/debug-logger
branch
from
0cd3ad2 to
5e1b4e2
Compare
| public DebugLogger(string name, Func<string, LogLevel, bool> filter) | ||
| { | ||
| _name = string.IsNullOrEmpty(name) ? nameof(DebugLogger) : name; | ||
| _filter = filter ?? ((category, logLevel) => true); |
There was a problem hiding this comment.
I think we should just leave _filter as null and check for it in IsEnabled rather than assign a default true returning delegate. Why run anything at all when checking IsEnabled if we can just check for null?
| _filter = filter; | ||
| } | ||
|
|
||
| public ILogger CreateLogger(string name) |
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "version": "1.0.0-*", | |||
| "dependencies": { | |||
Fixes #194
Please review @davidfowl @DamianEdwards @mikaelm12