-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Respect dynamic logging level changes for LevelSwitch section #160
Conversation
- fine-tune ReloadDelay for the DynamicLevelChangeTests
Need some investigation why test on Travis / Linux fails. Probably should change file based notifications in the test to something more stable. |
- fixed failing test
Ok, ready for review when someone have a time :) |
Assert.Single(DummyConsoleSink.Emitted); | ||
} | ||
|
||
void UpdateConfig(LogEventLevel? minimumLevel = null, LogEventLevel? switchLevel = null, LogEventLevel? overrideLevel = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that's personal taste, but I think turning the UpdateConfig
Local Function into a proper method would make it clearer (and shorten the code in the test method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor style comment, but, it looks good !
LGTM
LevelSwitch
sectionDummyConsoleSink
Fixes #99