-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Bugfix: Broken xml stops logging #671
Conversation
- Auto_reload_validxml_test succeeds - Auto_Reload_invalidxml_test fails
problem: XmlLoggingConfiguration.Initialize eats exception with invalid XML. ALso XmlLoggingConfiguration.Reload never returns null. So the null check is not enough - added InitializeSucceeded in XmlLoggingConfiguration - checks also InitializeSucceeded. If false, DON't apply the config. Result: invalidation XML wont stop logging, the old config will be used. Also, the auto reload won't turned to false (the default)
The changes are small, but a review would be wise because there is a new public property added. |
I works locally
Looks good to me. Just wondering about all the linebreaks between the tests? Any reason for this? |
The line breaks is just bad coding style of me ;) - I like white space when programming. Most of the time I let Resharper fix this, but too bad the NLog code style if really different then my Resharper profile (I like I will fix this code styling thing with a commit. |
wtf. Removed newlines and now are the unit tests failing? |
Unit tests are fixed and PR is ready for merge after review. |
@@ -62,6 +62,8 @@ public class XmlLoggingConfiguration : LoggingConfiguration | |||
|
|||
private string originalFileName; | |||
|
|||
|
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.
Could you delete these extra newlines?
@304NotModified Good work, please see my comments, otherwise it looks great |
Thanks. Unneeded newlines removed! |
Bugfix: Broken xml stops logging
fixes #273
problem:
XmlLoggingConfiguration.Initialize
eats exception with invalidXML. ALso
XmlLoggingConfiguration.Reload
never returnsnull
.So the
null
check is not enoughInitializeSucceeded
(nullablebool
) inXmlLoggingConfiguration
InitializeSucceeded
. Iffalse
, DON't apply the config.Result: invalidating the XML won't stop logging, the old config will be used.
Also, the auto reload won't turned to
false
(the default)The unit tests proves the solution and also the previous problem.