Visitar URL original
Cleanup | Merge SqlDataRecord and remove context connection remnants by edwardneal · Pull Request #3454 · dotnet/SqlClient · GitHub
Skip to content

Cleanup | Merge SqlDataRecord and remove context connection remnants - #3454

Merged
cheenamalhotra merged 7 commits into
dotnet:mainfrom
edwardneal:cleanup/sqldatarecord-contextconnection
Jul 10, 2025
Merged

Cleanup | Merge SqlDataRecord and remove context connection remnants#3454
cheenamalhotra merged 7 commits into
dotnet:mainfrom
edwardneal:cleanup/sqldatarecord-contextconnection

Conversation

@edwardneal

@edwardneal edwardneal commented Jun 27, 2025

Copy link
Copy Markdown
Contributor

Description

This picks up where #3438 left off. With SmiEventSink_Default removed, almost all of the code in SqlDataRecord.netfx.cs and SqlDataRecord.netcore.cs is identical and can be removed. We do so.

The SmiRecordBuffer class is also removed, since this isn't doing anything meaningful. I'm not pleased with the overall type hierarchy here, but I want to think through the consequences of changing them in the wider context of ValueUtilsSmi.

With that work done, we just remove the now-unused resource strings and exception methods from SqlUtil, AdapterUtil and the string resources. There are other unused strings and exception methods here, for anyone interested in picking up a bulky commit.

Closes #2838.

Issues

Follows #3438.
Resolves #2838.
Contributes to #1261.

Testing

Local unit tests for SqlDataRecord pass.

@edwardneal
edwardneal requested a review from a team as a code owner June 27, 2025 17:19
@benrr101

benrr101 commented Jun 30, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jun 30, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

benrr101
benrr101 previously approved these changes Jun 30, 2025
View reviewed changes

@benrr101 benrr101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do so 🫡

(one small request, but overall it's great)

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs Resolved
@apoorvdeshmukh

apoorvdeshmukh commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 2, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@codecov

codecov Bot commented Jul 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 77.96610% with 13 lines in your changes missing coverage. Please review.

Project coverage is 58.82%. Comparing base (fc950e8) to head (e136b03).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...c/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs 83.01% 9 Missing ⚠️
...rosoft/Data/SqlClient/Server/MemoryRecordBuffer.cs 0.00% 2 Missing ⚠️
...c/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs 33.33% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (fc950e8) and HEAD (e136b03). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (fc950e8) HEAD (e136b03)
addons 1 0
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3454      +/-   ##
==========================================
- Coverage   64.55%   58.82%   -5.73%     
==========================================
  Files         281      271      -10     
  Lines       62519    61984     -535     
==========================================
- Hits        40358    36464    -3894     
- Misses      22161    25520    +3359
Flag Coverage Δ
addons ?
netcore 63.32% <78.57%> (-3.62%) ⬇️
netfx 60.48% <77.58%> (-7.47%) ⬇️

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.

paulmedynski
paulmedynski previously approved these changes Jul 2, 2025
View reviewed changes
samsharma2700
samsharma2700 suggested changes Jul 3, 2025
View reviewed changes
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs Resolved
@edwardneal
Respond to code review
e136b03 
Exit early if we have no fields to set.
Clarify comment.
@paulmedynski

paulmedynski commented Jul 4, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 4, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@cheenamalhotra cheenamalhotra added this to the 7.0-preview1 milestone Jul 10, 2025
@cheenamalhotra
cheenamalhotra merged commit ee9640c into dotnet:main Jul 10, 2025
@edwardneal
edwardneal deleted the cleanup/sqldatarecord-contextconnection branch July 10, 2025 19:40
This was referenced Mar 17, 2026
Closed
Closed
Merged
Merged
This was referenced Nov 6, 2026
Closed
Open
Open
Closed
Merged
Open
Closed
Closed
Closed
Open
Merged
Closed
Merged
Open
@MZurmuehlen89 MZurmuehlen89 mentioned this pull request Nov 10, 2026
Closed
This was referenced Nov 10, 2026
Open
Closed
Open
Closed
Merged
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benrr101 benrr101 benrr101 approved these changes
@paulmedynski paulmedynski paulmedynski approved these changes
+1 more reviewer
@samsharma2700 samsharma2700 samsharma2700 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

7.0-preview1

Development

Successfully merging this pull request may close these issues.

.NET Framework SQLCLR support

6 participants

@edwardneal @benrr101 @apoorvdeshmukh @paulmedynski @samsharma2700 @cheenamalhotra