Conversation
…s no longer being used)
…ey are no longer used
… methods throw now ...
There was a problem hiding this comment.
Pull Request Overview
This PR removes unused SMI/server-side code and obsolete interfaces, refactors typed getters/setters to drop the unused EventSink parameter, and cleans up related project file entries.
- Deleted dead classes, methods, and netfx-specific SMI files no longer in use
- Updated
ITypedGettersV3/ITypedSettersV3and callers to remove theEventSinkargument - Removed legacy tests for private methods and streamlined compile includes in csproj
Reviewed Changes
Copilot reviewed 33 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| SqlMetaDataTest.cs | Removed reflection-based tests for GetPartialLengthMetaData |
| SmiEventSink_Default.cs | Simplified message handling and removed parent-sink logic |
| SqlBulkCopy.cs | Dropped _rowSourceIsSqlDataReaderSmi flag in streaming checks |
| ITypedGettersV3.cs, ITypedSettersV3.cs, SmiTypedGetterSetter.cs, TdsRecordBufferSetter.cs, TdsParameterSetter.cs, ValueUtilsSmi.cs, SqlUtil.cs, MetadataUtilsSmi.cs, etc. | Refactored to remove SmiEventSink from signatures and cleanup obsolete code |
| *.csproj | Removed compile references to deleted netfx SMI files |
Comments suppressed due to low confidence (3)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlMetaDataTest.cs:613
- These tests covered the behavior of the now-removed
GetPartialLengthMetaDatamethods; consider adding new tests for the adjusted typed getters/setters or removing references to ensure coverage isn't inadvertently reduced.
[Fact]
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiEventSink_Default.cs:20
- The refactored
HasMessagesno longer considers the parent sink, so nested sinks may miss messages propagated from their parent; restore or extend this logic to includeparent.HasMessageswhen_parentis set.
internal bool HasMessages => _errors is not null || _warnings is not null;
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:1255
- By removing the
_rowSourceIsSqlDataReaderSmicheck, streaming may now be enabled forSqlDataReaderSmiwhere it was previously disabled; reintroduce or replace this guard if streaming should remain unsupported for that reader type.
else if (_enableStreaming && (metadata.length == MAX_LENGTH || metadata.metaType.SqlDbType == SqlDbTypeExtensions.Json))
benrr101
left a comment
There was a problem hiding this comment.
Calling out some notable non-deletion changes
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3431 +/- ##
==========================================
+ Coverage 63.51% 64.82% +1.31%
==========================================
Files 293 282 -11
Lines 63810 62307 -1503
==========================================
- Hits 40526 40391 -135
+ Misses 23284 21916 -1368
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paulmedynski
left a comment
There was a problem hiding this comment.
One improvement suggested.
Description
This PR is a continuation of work done in parts 1 and 2 to clean up dead code in SMI. This time I took a slightly more heavy handed approach - if the class isn't being constructed anywhere, I deleted it. I then went through and removed any methods, properties, and variables that weren't being used in the Server folder, using IDE analysis of the files.
One of the biggest not-strictly-delete changes is to the methods in ITypedGetters/ITypedSetters. The methods in ITypedGetters/ITypedSetters did not take an EventSink in their arguments, but the interfaces themselves were not being used. The methods in ITypedGettersV3 and ITypedSettersV3 were being used around the codebase and take an EventSink in their arguments, but were never using the sink. As such, I deleted the ITypedGetters/ITypedSetters, and modified the ITypedGettersV3/ITypedSettersV3 to not take an EventSink in their arguments.
Issues
Believe it or not, this actually contributes to #1261 -
twofour of the classes wholesale deleted from the project were slated for merging!Testing
I attempted to run tests locally and identified a few that were failing due to reflection accessing methods that were removed. I suspect there will be more of these, but I was too impatient with the tests to wait for more to tests to complete.