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

Prevent obsolete warning on using property on framework greater than 6.0 #718

Closed
wants to merge 1 commit into from
Closed

Prevent obsolete warning on using property on framework greater than 6.0 #718

wants to merge 1 commit into from

Conversation

Owen-Krueger
Copy link

When using NLog from a solution using .NET 7.0 or .NET 8.0, an obsolete warning is displayed when using the IncludeActivityIdsWithBeginScope property.

image

This is caused by the preprocessor check that explicitly checks if the framework is .NET 6.0. Microsoft offers NET6_0_OR_GREATER that should prevent this warning when using frameworks greater than 6.0: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cfe3e59) 80.92% compared to head (9663860) 80.92%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #718   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files          19       19           
  Lines        1735     1735           
  Branches      304      304           
=======================================
  Hits         1404     1404           
  Misses        194      194           
  Partials      137      137           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@snakefoot
Copy link
Contributor

snakefoot commented Dec 29, 2023

Thank you for high-lighting this. I have created #719 that also ensures the feature will actually work on NET8

Notice that IncludeActivityIdsWithBeginScope was mostly made to make the conversion from NetCore31 to NET5 a little easier (re-adding properties that Microsoft removed). Surprised a temporary patch seems to have grown into a default feature.

Notice https://www.nuget.org/packages/NLog.DiagnosticSource#readme-body-tab allows you to extract additional scope-context from System.Diagnostics.Activity.Current.

@Owen-Krueger
Copy link
Author

That sounds great--thanks, @snakefoot! Thanks for your help on this and for all your hard work on this package! I'll close this up, since you've now implemented a better change.

I looked over your PR and noticed one small thing that could be tweaked. The obsolete warning calls out that it should only be used with .NET 6.0 and above, but the actual check is that the solution is .NET 5.0. It would probably be good to change the warning to match the check: https://github.com/snakefoot/NLog.Extensions.Logging/blob/5aec4087d4b799d120b50e87797692a3dd5484c3/src/NLog.Extensions.Logging/Logging/NLogProviderOptions.cs#L81

@snakefoot
Copy link
Contributor

snakefoot commented Dec 29, 2023

The obsolete warning calls out that it should only be used with .NET 6.0 and above, but the actual check is that the solution is .NET 5

Yes it is a little confusing now:

  • The setting was introduced with NET5, so I thought it would be nice that it said NET5_0_OR_GREATER.
  • But NET5 is now obsolete so NLog.Extensions.Logging-nuget-package does not have direct support for NET5, but will instead fallback to NetStandard2.1-platform when used in NET5-application-project.
  • Only if using NET6 / NET7 / NET8 then the setting will work, so I think the obsolete message is correct.

@snakefoot
Copy link
Contributor

snakefoot commented Dec 29, 2023

https://www.nuget.org/packages/NLog.Extensions.Logging/5.3.8 has been released.

Thanks again for reporting the issue.

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.

3 participants