Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Cleanup | SMI Cleanup (part 3)#3431

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
benrr101 merged 18 commits intomainfromdev/russellben/cleanup/smi-dead-code3
Jun 23, 2025

Conversation

benrr101
Copy link
Contributor

@benrr101benrr101 commentedJun 18, 2025•
edited
Loading

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!

  • SqlDataReaderSmi.cs
  • SqlInternalConnectionSmi
  • SqlSequentialStreamSmi
  • SqlSequentialTextReaderSmi

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.

cheenamalhotra reacted with rocket emoji
@benrr101benrr101 added this to the7.0-preview1 milestoneJun 18, 2025
@CopilotCopilotAI review requested due to automatic review settingsJune 18, 2025 22:52
@benrr101benrr101 added Code Health 💊Issues/PRs that are targeted to source code quality improvements. Common Project 🚮Things that relate to the common project project labelsJun 18, 2025
@benrr101benrr101 requested a review froma team as acode ownerJune 18, 2025 22:52
Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes unused SMI/server-side code and obsolete interfaces, refactors typed getters/setters to drop the unusedEventSink parameter, and cleans up related project file entries.

  • Deleted dead classes, methods, and netfx-specific SMI files no longer in use
  • UpdatedITypedGettersV3/ITypedSettersV3 and callers to remove theEventSink argument
  • 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
FileDescription
SqlMetaDataTest.csRemoved reflection-based tests forGetPartialLengthMetaData
SmiEventSink_Default.csSimplified message handling and removed parent-sink logic
SqlBulkCopy.csDropped_rowSourceIsSqlDataReaderSmi flag in streaming checks
ITypedGettersV3.cs, ITypedSettersV3.cs, SmiTypedGetterSetter.cs, TdsRecordBufferSetter.cs, TdsParameterSetter.cs, ValueUtilsSmi.cs, SqlUtil.cs, MetadataUtilsSmi.cs, etc.Refactored to removeSmiEventSink from signatures and cleanup obsolete code
*.csprojRemoved 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-removedGetPartialLengthMetaData methods; 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 refactoredHasMessages no longer considers the parent sink, so nested sinks may miss messages propagated from their parent; restore or extend this logic to includeparent.HasMessages when_parent is 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_rowSourceIsSqlDataReaderSmi check, streaming may now be enabled forSqlDataReaderSmi where 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))

Copy link
ContributorAuthor

@benrr101benrr101 left a comment

Choose a reason for hiding this comment

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

Calling out some notable non-deletion changes

mdaigle
mdaigle previously approved these changesJun 19, 2025
@codecovCodecov
Copy link

codecovbot commentedJun 19, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is76.43678% with41 lines in your changes missing coverage. Please review.

Project coverage is 64.82%. Comparing base(12f742d) to head(e04b4e9).
Report is 3 commits behind head on main.

Files with missing linesPatch %Lines
...c/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs74.10%29 Missing⚠️
...soft/Data/SqlClient/Server/SmiEventSink_Default.cs16.66%5 Missing⚠️
...src/Microsoft/Data/SqlClient/TdsParameterSetter.cs0.00%3 Missing⚠️
...Microsoft/Data/SqlClient/Server/SmiRecordBuffer.cs0.00%2 Missing⚠️
.../Microsoft/Data/SqlClient/TdsRecordBufferSetter.cs0.00%2 Missing⚠️
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
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore69.01% <76.07%> (+2.08%)⬆️
netfx66.30% <76.74%> (+0.08%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

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

One improvement suggested.

@benrr101benrr101 merged commitb9b9153 intomainJun 23, 2025
251 checks passed
@benrr101benrr101 deleted the dev/russellben/cleanup/smi-dead-code3 branchJune 23, 2025 22:12
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

@mdaiglemdaiglemdaigle approved these changes

@paulmedynskipaulmedynskipaulmedynski approved these changes

Assignees
No one assigned
Labels
Code Health 💊Issues/PRs that are targeted to source code quality improvements.Common Project 🚮Things that relate to the common project project
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

3 participants
@benrr101@mdaigle@paulmedynski
[8]ページ先頭
©2009-2025 Movatter.jp