Issue #3: Refactoring ILogger #31
Conversation
| @@ -37,19 +37,18 @@ public Logger(global::NLog.Logger logger) | |||
| Func<object, Exception, string> formatter) | |||
| { | |||
| var logLevel = GetLogLevel(eventType); | |||
| if (!_logger.IsEnabled(logLevel)) | |||
| if (formatter != null) | |||
There was a problem hiding this comment.
@lodejard What does it mean if the formatter is null after this refactoring?
There was a problem hiding this comment.
Maybe we ought to have a fallback? Like maybe call ToString on state (if it's not null), and then get some data from the exception, such as its Message and/or StackTrace?
There was a problem hiding this comment.
Or throw argument null exception? @Eilon's suggestion wouldn't be surprising either, but if the form a tter is null you're calling the wrong overload...
sonjakhan
force-pushed
the
refactor-ilogger
branch
from
f609635 to
44c6278
Compare
| } | ||
| if (exception != null) | ||
| { | ||
| message += "\r\n" + exception; |
There was a problem hiding this comment.
- Instead of
\r\nuseEnvironment.NewLineso that it'll work right on all OSes - Should the newline be added only if there is a
stateandexception? Otherwise if there's only an exception then there'll be an empty line above it always, no?
There was a problem hiding this comment.
It depends on the formatting defined in NLog.config. In the sample it's
[level:logger] state
exception
so if there's no state it would be
[level:logger]
exception
I think it makes sense to have a new line before the exception even if there is no state, and I believe this code also has a new line even if there's no state. Does the CultureInfo make sure the \r\n is rendered correctly on all OSes?
There was a problem hiding this comment.
CultureInfo doesn't affect newlines at all. It's more about the underlying OS (Windows, OS X, *nix, etc.).
I think I see your point about the newlines. So you always want the exception on its own line no matter what? I can live with that.
There was a problem hiding this comment.
I think that would be most consistent with the extension methods. I will update the extension class to not hard code \r\n
There was a problem hiding this comment.
@victorhurdugaci Eilon and I discussed the new line here, do you still think the newline should be avoided if state == null?
| using System.Diagnostics; | ||
| using Xunit; | ||
| #if ASPNET50 | ||
| using Moq; |
There was a problem hiding this comment.
Are you using Moq at all? If not, remove the reference and enable the tests for ASPNET50
|
|
when 
|
I'm always happy. So I guess |
sonjakhan
force-pushed
the
refactor-ilogger
branch
from
057dd09 to
70318bf
Compare
Moved extension method
IsEnabledto theILoggerinterface, renamedWriteCoretoWriteand changed the return type to void. This will break other repositories that use the ILogger interface, which I will update once I get feedback on the changes here.