Skip to content
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

FilteringTargetWrapper - Fix XSD for Filter-property #3474

Closed
wants to merge 1 commit into from

Conversation

snakefoot
Copy link
Contributor

No description provided.

@codecov-io
Copy link

Codecov Report

Merging #3474 into dev will increase coverage by <1%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev   #3474    +/-   ##
======================================
+ Coverage     81%     81%   +<1%     
======================================
  Files        343     343            
  Lines      27834   27834            
  Branches    3766    3766            
======================================
+ Hits       22447   22455     +8     
+ Misses      4295    4287     -8     
  Partials    1092    1092

@304NotModified
Copy link
Member

That's an easy solution :)

But do you think we should have the possibility to set the filter attribute (and also condition?) on the filter wrapper?

@304NotModified
Copy link
Member

Another option is to hide it

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 11, 2019 via email

@304NotModified
Copy link
Member

Ok, but I mean from the XML (NLog.config, not xsd) for

But do you think we should have the possibility to set the filter attribute (and also condition?) on the filter wrapper?

Another option is to hide it

So functional, not technical. :)

@304NotModified
Copy link
Member

304NotModified commented Jun 11, 2019

Eg

We have

target xsi:type="FilteringWrapper" name="String" condition="Condition">

Does this makes sense?

target xsi:type="FilteringWrapper" name="String" filter="string">

And then what about

target xsi:type="FilteringWrapper" name="String" condition="Condition" filter="string">

@snakefoot
Copy link
Contributor Author

Sounds like you are on top of things. If you can hide Filter as attribute, then please do so.

@304NotModified
Copy link
Member

thanks, I like the idea as failsafe, so added it here: #3475

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 12, 2019 via email

@304NotModified
Copy link
Member

Why not? I tested it, look ok to me

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 12, 2019

Why not? I tested it, look ok to me

I'm very confused. You said already that you know that NLogConfigurationIgnoreProperty will affect how configuration is applied. Then you add this attribute and insists it will work just fine.

Well updated #3476 with a unit test that will fail, when people adds unwanted attributes to random properties.

@304NotModified
Copy link
Member

304NotModified commented Jun 12, 2019

Yes, and afterwards I tested it, including the code paths that are changed...

@snakefoot
Copy link
Contributor Author

But the updated unit-test in #3476 fails when you add that attribute to the Filter-property. Please try.

@304NotModified
Copy link
Member

You're right. I didn't understand that my change was breaking things.

Then my PR is a no-go :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants