Skip to content

Conversation

edwardneal
Copy link
Contributor

Description

This merges the SqlConnection class. I recommend reviewing this commit-by-commit with whitespace changes disabled; the underlying changes are pretty minor.

Earlier PRs have already addressed all of the breaking changes, so each commit handles a fairly small change needed to render the files identical. By b966f7a, these files are ready to merge.

I've tried to avoid functional changes. As a result, I noticed a handful of sections which enabled WindowsIdentity impersonation and have conditionally compiled them for netfx. These would probably work for netcore, but I don't have ready access to a domain from Windows and Linux and so can't test them.

On one final point, this doesn't deal with the SqlConnectionHelper.cs file. That file remains target-specific, it's too difficult to merge a sensible without dealing with the main SqlConnection.cs file first.

Issues

Relates to #1261.
Ports #378 and #379 from netcore to netfx.

Testing

Automated tests run.

Could someone run CI please?

@edwardneal edwardneal requested a review from a team as a code owner September 11, 2025 05:51
@apoorvdeshmukh
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

The target adds an obsolete attribute to generated code, and this blocks compilation
@edwardneal
Copy link
Contributor Author

Thanks - I've just spotted the problem, could someone re-run CI please?

@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 85.24590% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.31%. Comparing base (cd3dbd1) to head (9e11e79).
⚠️ Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
...ient/src/Microsoft/Data/SqlClient/SqlConnection.cs 85.24% 9 Missing ⚠️

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

HEAD has 1 upload less than BASE
Flag BASE (cd3dbd1) HEAD (9e11e79)
addons 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3608      +/-   ##
==========================================
- Coverage   65.48%   60.31%   -5.18%     
==========================================
  Files         275      269       -6     
  Lines       61518    60465    -1053     
==========================================
- Hits        40288    36469    -3819     
- Misses      21230    23996    +2766     
Flag Coverage Δ
addons ?
netcore 62.45% <93.10%> (-5.20%) ⬇️
netfx 64.20% <84.48%> (-5.01%) ⬇️

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.

benrr101
benrr101 previously approved these changes Sep 17, 2025
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Overall looks good, got a couple questions, but glad to move forward with it as-is

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Sep 17, 2025
@benrr101 benrr101 added this to the 7.0-preview2 milestone Sep 17, 2025
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Thanks again for all your work on this! 🚀 🚀

@benrr101
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski paulmedynski merged commit 94a51da into dotnet:main Sep 23, 2025
236 checks passed
@edwardneal edwardneal deleted the merge/sqlconnection branch September 23, 2025 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants