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

Minimumlevel partial fix with IConfigurationRoot.Providers #233

Merged
merged 2 commits into from Sep 13, 2022

Conversation

HexHunter97
Copy link

A partial fix for minimumlevel not being correctly overriden by higher priority providers.

The fix only works for the .NET 4.6.1+ and .NET Standard builds as the IConfigurationRoot interface did not have Providers in .NET 4.5.1.

I understand this is more of an annoyance than a real issue, but it's easy to miss.

A partial fix for minimumlevel not being correctly overriden by higher priority providers.
#if NETSTANDARD || NET461
_configurationRoot = configuration as IConfigurationRoot;
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

#endif //NET451 or fallback

return minimumLevelDirective.Value != null ? minimumLevelDirective : minimumLevelDirective.GetSection("Default");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@nblumhardt
Copy link
Member

Thanks! 👍 As it's a slightly tricky scenario, it would be great to have a test for this behavior, so we can lessen the risk of regression in the future.

@HexHunter97
Copy link
Author

I realise the long lines are less than desirable, but I don't know how to break them up properly wihout without taking up too much vertical space. :(
Any advice on the matter welcome.

@nblumhardt
Copy link
Member

Sorry about the long delay on this; merging to the 3.5.0-dev-* version now.

@nblumhardt nblumhardt merged commit 375c5bb into serilog:dev Sep 13, 2022
@HexHunter97
Copy link
Author

It's ok, there are other priorities sometimes.
Thanks for letting it in!

@nblumhardt nblumhardt mentioned this pull request Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants