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
Added LogMessageGenerator overloads for exceptions #2051
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2051 +/- ##
=======================================
+ Coverage 81% 81% +<1%
=======================================
Files 288 289 +1
Lines 19840 19900 +60
Branches 2339 2352 +13
=======================================
+ Hits 16135 16187 +52
- Misses 3109 3119 +10
+ Partials 596 594 -2 Continue to review full report at Codecov.
|
Thanks! Unfortunately this isn't backwards-compatible. I think we should write a C# extensions. What do you think? PS: changes ignoring whitespace changes: https://github.com/NLog/NLog/pull/2051/files?w=1 |
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.
Breaking change
Could you clarify what you mean by it being a breaking change so I know for next time? I think extensions sound good, though. Do you mean something like this? |
Well extending a public interface is a breaking change as other uses who implement that interface have to implement those new methods. (Custom) Code written to NLog 4.x, should always work for every NLog 4.y (where |
A full list of what is breaking and what is not: https://github.com/dotnet/corefx/blob/c02d33b18398199f6acc17d375dab154e9a1df66/Documentation/coding-guidelines/breaking-change-rules.md |
Thanks for the link! That list makes a lot of sense. How should I back out these breaking changes in order to add the extension methods? |
Well you could add methods to the interface by using c# extensions. That won't break anything :) |
Thanks for the suggestion. I added a new class for the extension methods. Let me know what you think. |
Looks good, but the changes to What's the problem with the CLScompliant? We prefer CLScompliant code |
Whoops, I forgot to revert the changes to I couldn't have the extensions be CLS-compliant because the |
Thanks! Merged! Will be in NLog 4.4.6 |
FYI, NLog 4.4.6 is live! |
Added support for deferred message generation on method overloads that also support Exceptions. See #2014