-
Notifications
You must be signed in to change notification settings - Fork 300
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
Remove code that is unused in .NET 8 #2991
Conversation
Should also fix #303 I think |
@@ -217,9 +217,6 @@ | |||
<Compile Include="$(CommonSourceRoot)Microsoft\Data\ProviderBase\TimeoutTimer.cs"> | |||
<Link>Microsoft\Data\ProviderBase\TimeoutTimer.cs</Link> | |||
</Compile> | |||
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SSPI\ManagedSSPIContextProvider.cs"> |
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.
Were these files not used in .NET Framework?
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 is the #IF NET6_0 file... so while it was referenced, it was not compiled for netfx
@@ -9,9 +9,8 @@ internal static partial class Libraries | |||
internal const string Kernel32 = "kernel32.dll"; | |||
internal const string NtDll = "ntdll.dll"; | |||
#if !NET8_0_OR_GREATER |
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.
Why was this left in?
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.
The tests use that in multiple locations in SecurityWrapper.cs
SqlClient/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/SSPI/SecurityWrapper.cs
Line 27 in 4a9b070
[DllImport(Interop.Libraries.SspiCli, CharSet = CharSet.Unicode, EntryPoint = "AcquireCredentialsHandleW")] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2991 +/- ##
==========================================
+ Coverage 72.47% 72.50% +0.02%
==========================================
Files 288 288
Lines 59498 59498
==========================================
+ Hits 43124 43141 +17
+ Misses 16374 16357 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Hot dang! I love these kinds of changes! My only complaint is you stole all the fun from me :P
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 @MichelZ
LGTM!
There's code marked as !NET8_0_OR_GREATER in the netcore project, or NET && !NET8_0_OR_GREATER in the shared project that can be removed, now that the only supported .NET (Core) version is 8+