Skip to content

Conversation

benrr101
Copy link
Contributor

@benrr101 benrr101 commented Sep 2, 2025

Description

This PR merges the methods used for preparing and unpreparing SqlCommand objects. This PR introduces the new merge style that I'm adopting for this class.

Merge Partials

Rather than try to do the merge as one big bang as I've done in the past, with simpler classes, this class needs some special attention. One particular issue that causes great headache is the disorder of the members of the class and the sheer size of the class. I would like to make improvements here while also making it clear what how things move from the separate projects to the common project. So, I've made the SqlCommand files in the separate projects partials, and added a partial to the common project. This allows me to move each member to the partial one at a time, give it access to what's in the separate project files, give the separate project files access to whats been merged, as well as sort the merged members and apply light cleanup in each merge.

Please note, there are a lot of "TODO" items added to this code, mostly as notes for myself on how to clean up the class once merging is complete.

Each commit has been tested to build - it is encouraged to step through this PR commit-by-commit

Issues

Continuation of work for #1261

Testing

Code still builds, CI should confirm

@benrr101 benrr101 added this to the 7.0-preview1 milestone Sep 2, 2025
@Copilot Copilot AI review requested due to automatic review settings September 2, 2025 22:40
@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Sep 2, 2025
@benrr101 benrr101 requested a review from a team as a code owner September 2, 2025 22:40
Copy link
Contributor

@Copilot Copilot AI 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 introduces a partial class pattern for SqlCommand to facilitate merging platform-specific code into a common implementation. The goal is to incrementally move members from separate .NET Framework and .NET Core implementations to a shared codebase while maintaining build integrity and improving code organization.

  • Converts SqlCommand to partial classes across platforms
  • Moves Prepare/Unprepare functionality to shared implementation
  • Adds structured organization with regions and improved documentation

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.stub.cs Removes temporary stub class used during merge process
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs Adds new shared partial class with Prepare/Unprepare methods and common fields
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientEventSource.cs Adds new TryCorrelationTraceEvent overload for parameterless messages
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.netfx.cs Converts to partial class and removes moved Prepare/Unprepare code
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj Updates project to include shared SqlCommand.cs and renames platform file
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs Converts to partial class and removes moved Prepare/Unprepare code
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj Updates project to include shared SqlCommand.cs and renames platform file

mdaigle
mdaigle previously approved these changes Sep 3, 2025
Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

A few questions, and feel free to do more tidying in the new SqlCommand.cs file if you want.

paulmedynski
paulmedynski previously approved these changes Sep 9, 2025
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Looks good - thanks for the explanations!

@cheenamalhotra
Copy link
Member

It seems there's a test failure corresponding to changes in the PR, @benrr101
Could you address them asap?

Copy link

codecov bot commented Sep 9, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.56%. Comparing base (cf2bdc7) to head (581b273).
⚠️ Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
...lClient/src/Microsoft/Data/SqlClient/SqlCommand.cs 88.46% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3593      +/-   ##
==========================================
+ Coverage   63.19%   68.56%   +5.36%     
==========================================
  Files         274      275       +1     
  Lines       62478    61588     -890     
==========================================
+ Hits        39486    42230    +2744     
+ Misses      22992    19358    -3634     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore 73.46% <87.01%> (+6.69%) ⬆️
netfx 66.61% <88.60%> (+0.44%) ⬆️

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

@samsharma2700 samsharma2700 left a comment

Choose a reason for hiding this comment

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

LGTM

@benrr101 benrr101 merged commit 0809419 into main Sep 10, 2025
250 checks passed
@benrr101 benrr101 deleted the dev/russellben/merge/sqlcommand-nocer-prepare branch September 10, 2025 17:14
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.

5 participants