-
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: Can't update EventLog's Source property #1548
Conversation
EventLog Source not updated - Fix
@@ -349,7 +349,9 @@ internal string GetFixedSource() | |||
/// <returns></returns> | |||
private EventLog GetEventLog(LogEventInfo logEvent) | |||
{ | |||
return eventLogInstance ?? (eventLogInstance = new EventLog(this.Log, this.MachineName, this.Source.Render(logEvent))); | |||
if (eventLogInstance?.Source == this.Source.Render(logEvent)) |
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.
In worst case we're rendering the source
two times?
Also there is an (implicit) two times check on null
and we need to check also log and machine?
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.
Do you mean something like this?
if (eventLogInstance?.Source == this.Source.Render(logEvent) && eventLogInstance?.Log == this.Log && eventLogInstance?.MachineName == this.MachineName)
return eventLogInstance ?? (eventLogInstance = new EventLog(this.Log, this.MachineName, this.Source.Render(logEvent)));
return eventLogInstance = new EventLog(this.Log, this.MachineName, this.Source.Render(logEvent));
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.
Think it fixes the issue (hard to read on mobile) but Its still double checking null (?.
and ==
), which is a bit sloppy
Thanks! Added a note. Not sure why appveyor complains, looks unrelated |
Current coverage is 76% (diff: 75%)@@ master #1548 diff @@
==========================================
Files 269 269
Lines 16266 16288 +22
Methods 2605 2607 +2
Messages 0 0
Branches 1769 1776 +7
==========================================
+ Hits 12367 12386 +19
- Misses 3487 3491 +4
+ Partials 412 411 -1
|
fixes #1542