Skip to content

Conversation

@benrr101
Copy link
Contributor

Description

This is a continuation of the merge of SqlCommand class. This specifically focuses on merging public, internal, and protected properties. As with the previous PRs, each commit is bite sized and stands on its own. The following members are merged:

  • _batchRPCMode
  • _columnEncryptionSetting
  • _commandTimeout
  • _designTimeInvisible
  • _notification
  • _notificationAutoEnlist
  • _parameters
  • _pendingCancel
  • _preparedConnectionCloseCount
  • _preparedConnectionReconnectionCount
  • _retryLogicProvider
  • _rowsAffected
  • _sqlDep
  • _statementCompletedEventHandler
  • _transaction
  • _updatedRowSource
  • ColumnEncryptionSetting
  • CommandText - "" was replaced with string.Empty
  • CommandTimeout
  • CommandType - removed temp variable
  • Connection
  • DbConnection
  • DbParameterCollection
  • DbTransaction
  • DefaultCommandTimeout
  • DesignTimeVisible
  • EnableOptimizedParameterBinding
  • InternalRecordsAffected
  • InternalTdsConnection
  • IsColumnEncryptionEnabled
  • Notification
  • NotificationAutoEnlist
  • Parameters
  • PropertyChanging()
  • RetryLogicProvider
  • StatementCompleted
  • Statistics
  • Transaction
  • UpdatedRowSource

And a few changes to things other than SqlCommand:

  • ADP.InvalidCommandTimeout was defined twice in AdapterUtility, once for netfx without [CallerMemberName] and once for netcore with [CallerMemberName]. I standardized on the netcore version and moved it where the rest of the methods live.

Issues

Continuation of work for #1261

Testing

Code is moving with minimal stylistic changes here and there. Compilation and CI should be sufficient testing.

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

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 continues the merging of the SqlCommand class across different target frameworks by consolidating public, internal, and protected properties into a shared base implementation. The primary goal is to eliminate code duplication between .NET Framework and .NET Core implementations by moving shared properties to the common SqlCommand.cs file.

Key changes include:

  • Moved field declarations and property implementations from platform-specific files to the shared base
  • Consolidated property getter/setter logic with minor stylistic improvements
  • Standardized the InvalidCommandTimeout exception method across platforms

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs Added shared field declarations, public properties, and internal/protected properties that were previously duplicated across platforms
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Consolidated InvalidCommandTimeout method definitions by moving the version with [CallerMemberName] to the shared location and removing duplicates
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.netfx.cs Removed field declarations and property implementations that are now in the shared base class
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs Removed field declarations and property implementations that are now in the shared base class

Copy link
Contributor

@edwardneal edwardneal left a comment

Choose a reason for hiding this comment

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

One or two comments, but it's great to see this pressing forwards!

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.82%. Comparing base (cd3dbd1) to head (49be6a2).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3611       +/-   ##
===========================================
+ Coverage   65.48%   90.82%   +25.33%     
===========================================
  Files         275        6      -269     
  Lines       61518      316    -61202     
===========================================
- Hits        40288      287    -40001     
+ Misses      21230       29    -21201     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore ?
netfx ?

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.

Looks good.

@benrr101 benrr101 merged commit e472245 into main Sep 17, 2025
252 checks passed
@benrr101 benrr101 deleted the dev/russellben/merge/sqlcommand-nocer-publicprops branch September 17, 2025 23:07
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