LogFactory - Fixed EventArgs for ConfigurationChanged #1897
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1897 +/- ##
========================================
Coverage ? 82%
========================================
Files ? 307
Lines ? 21810
Branches ? 2656
========================================
Hits ? 17850
Misses ? 3286
Partials ? 674 |
snakefoot
force-pushed
the
LogFactoryConfigurationChangedFlip2
branch
from
c743e4e
to
429a748
Compare
snakefoot
changed the title
snakefoot
changed the title
snakefoot
force-pushed
the
LogFactoryConfigurationChangedFlip2
branch
2 times, most recently
from
7df36f6
to
ff3bb11
Compare
src/NLog/LogFactory.cs
Outdated
| @@ -338,7 +338,7 @@ public LoggingConfiguration Configuration | |||
| } | |||
| } | |||
|
|
|||
| this.OnConfigurationChanged(new LoggingConfigurationChangedEventArgs(value, oldConfig)); | |||
| this.OnConfigurationChanged(new LoggingConfigurationChangedEventArgs(oldConfig, value)); | |||
There was a problem hiding this comment.
so this is still a breaking change.
I like to resolve this issue, but if it's breaking, it should be NLog 5.
There was a problem hiding this comment.
No it is not a breaking change as I have "fixed" the now obsolete properties for LoggingConfigurationChangedEventArgs. Guess it should wait for NLog 4.5 because of the obsolete handling
There was a problem hiding this comment.
If I call new LoggingConfigurationChangedEventArgs(value, oldConfig) after this PR has been merged, then it has a different behavior? Or am I missing something :)
304NotModified
added
the
breaking change
|
Not sure if there is a real benefit to introduce a breaking change which isn't really needed (even if it's NLog 5) |
|
Well it fooled me, as I tried to add a configuration-changed event handler for the unit-tests, where I enabled throw-exceptions for all loaded configurations. Sadly enough it never worked, because I actually enabled it for the old-configuration. Others also think this is weird as you can see with #1122 |
yes could't agree more.
and after this change. it could "magically" won't work for others am I afraid of. |
snakefoot
force-pushed
the
LogFactoryConfigurationChangedFlip2
branch
from
ff3bb11
to
4c06a77
Compare
|
You are right. People who allocates the NLog LoggingConfigurationChangedEventArgs for use elsewhere will get a breaking change. I have updated the PR. |
|
@304NotModified This is actually not a breaking change. It just introduces new properties and marks the old ones as obsolete. There is no change between the constructor-input-parameters and the behavior of the obsolete properties. |
…ration (Made faulty OldConfiguration and NewConfiguration obsolete)
snakefoot
force-pushed
the
LogFactoryConfigurationChangedFlip2
branch
from
4c06a77
to
4db35b5
Compare
|
@304NotModified Can we have this PR included for NLog 4.5 as it is not a breaking change? Now just faulty properties marked as obsolete, and introduction of correctly working properties. |
|
isn't the breaking change here the reordered parameters in the ctor? |
How are the parameters re-ordered? Since NLog 4.0 broke LoggingConfigurationChangedEventArgs, then the first parameter has become the new active configuration. The obsolete (and faulty) OldConfiguration makes sure to return the first parameter (new active configuration). |
|
|
No matter what then NLog ver 3 users will feel the punishment of the NLog 4 breaking change. In NLog 4 the first parameter has always been the new active configuration, it was just named wrong in the documentation. I have just fixed the documentation. |
Ah, indeed, missed that. |
304NotModified
added
bug
|
merged! |
Fixes the issue #1122 with the breaking change introduced with NLog ver. 4 for LoggingConfigurationChangedEventArgs. See also #491
This marks the broken OldConfiguration and NewConfiguration as obsolete, and instead introduces two new properties:
No source breaking change, and no binary breaking change. Just obsolete warnings and new properties.