-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
Codecov Report
@@ Coverage Diff @@
## main #478 +/- ##
=======================================
- Coverage 77.0% 76.9% -0.2%
=======================================
Files 85 85
Lines 4153 4153
=======================================
- Hits 3200 3195 -5
- Misses 669 673 +4
- Partials 284 285 +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.
Can you add a test or two to demonstrate the functionality?
RenderedLevel string `xml:"RenderingInfo>Level"` | ||
Level string `xml:"System>Level"` |
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.
Based on the issue description, it sounds like we might expect only one or the other of these. Is that right? If so, should we parse both here and only set whichever one is populated?
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.
That would be correct, only one of these would be populated, that being said I'm sure that could be done to only populate one of the fields. But I do have a question about that, would this mean getting rid of the RenderedLevel
field of the EventXML struct, or simply removing that field from the output?
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.
I would expect that it only requires a change to the body of the log that is emitted.
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 to clarify, both may be populated in the XML. The RenderingInfo
tag contains the "friendly" versions of these fields, for instance, System>Level
may be 4
, but RenderingInfo>Level
would be Informational
. In this case, both would be present on the XML returned from windows.
Only populating one field might make sense for the sake of removing redundancy, but I just want to clarify.
@djaglowski there should be some tests in there now |
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.
This looks good to me,
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.
Thanks @armstrmi
Changes
xml.go
file.xml_test.go
and a sample WEL xml fileLink to Tracking Issue
#476