This is easier to review commit-by-commit - it performs formatting changes, reorders members, synchronises comments and other cosmetic changes. The end result is that SqlConnection is reduced to a -315/+305 diff.

The remaining diff is 1/3 context connection handling, 1/3 logging changes, 1/6 unported performance improvements and 1/6 being the "real" set of behavioural changes.)

Could someone run CI against this please?

Copy link

Contributor

paulmedynski commentedApr 14, 2025

/azp run

paulmedynski self-assigned this

Apr 14, 2025

Copy link

azure-pipelinesbot commentedApr 14, 2025

Azure Pipelines successfully started running 1 pipeline(s).

paulmedynski requested changes

Apr 14, 2025

View reviewed changes

Copy link

Contributor

paulmedynski left a comment

Choose a reason for hiding this comment

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

Cleanup is always welcome! A couple of minor tweaks.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.csShow resolvedHide resolved

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs OutdatedShow resolvedHide resolved

Copy link

codecovbot commentedApr 14, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is82.75862% with25 lines in your changes missing coverage. Please review.

Project coverage is 66.16%. Comparing base(c5e3c40) to head(62aab14).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	83.70%	22 Missing⚠️
...etcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	0.00%	1 Missing⚠️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs	87.50%	1 Missing⚠️
.../netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	0.00%	1 Missing⚠️

❗ There is a different number of reports uploaded between BASE (c5e3c40) and HEAD (62aab14). Click for more details.
HEAD has 1 upload less than BASE
Flag BASE (c5e3c40) HEAD (62aab14)
addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@##             main    #3275      +/-   ##==========================================- Coverage   72.98%   66.16%   -6.83%==========================================  Files         299      292       -7       Lines       57215    56691     -524     ==========================================- Hits        41760    37509    -4251- Misses      15455    19182    +3727

Flag	Coverage Δ
addons	`?`
netcore	`68.99% <77.77%> (-6.20%)`	⬇️
netfx	`65.21% <83.08%> (-6.28%)`	⬇️

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 approved these changes

Apr 14, 2025

View reviewed changes

Copy link

Contributor

paulmedynski commentedApr 14, 2025

/azp run

Copy link

azure-pipelinesbot commentedApr 14, 2025

Azure Pipelines successfully started running 1 pipeline(s).

samsharma2700 approved these changes

Apr 15, 2025

View reviewed changes

cheenamalhotra added this to the6.1-preview1 milestone

Apr 16, 2025

cheenamalhotra approved these changes

Apr 16, 2025

View reviewed changes

cheenamalhotra requested a review frombenrr101

April 16, 2025 03:03

benrr101 approved these changes

Apr 16, 2025

View reviewed changes

Copy link

Contributor

benrr101 left a comment

Choose a reason for hiding this comment

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

I think you goal here is to get one side in line with the other. In that case I can be a lot more forgiving to stuff like order of properties. No need to address the comments, just raising them as things to think on.
Otherwise, glad to get closer to merging the big beasts.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs Outdated

		= new(concurrencyLevel: 4 * Environment.ProcessorCount /* default value in ConcurrentDictionary*/,
		capacity: 1,
		comparer: StringComparer.OrdinalIgnoreCase);

		internal bool ForceNewConnection

Copy link

Contributor

benrr101Apr 16, 2025

Choose a reason for hiding this comment

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

I'm assuming the goal here is just to get it into the same place as it is in netcore? Otherwise I'd be a stickler and say properties belong after constructors

Copy link

ContributorAuthor

edwardnealApr 17, 2025

Choose a reason for hiding this comment

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

Exactly - the goal's to make a Visual Studio diff useful. I've tested reordering properties, but it'd rapidly increase the size of the diff: even moving a dozen properties resulted in a 300-line change.

I agree it'd be a good idea to deal with code style fixups though. We could do this after the codebases are merged, or file-by-file through the merged codebase. The WPF repo is going through this at the moment, and the approach they've taken isdotnet/wpf#10270.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs

		/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/CommandTimeout/*' />
		[DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
		[ResDescription(StringsHelper.ResourceNames.SqlConnection_ConnectionTimeout)]
		public int CommandTimeout

Copy link

Contributor

benrr101Apr 16, 2025

Choose a reason for hiding this comment

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

I don't get this ordering scheme at all.... If we're just trying to get one side in line with the other side, then I won't complain. But if we're not, we've got privates before publics, properties before constructors, properties after constructors...

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs

		@@ -178,43 +173,35 @@ public SqlConnection(string connectionString, SqlCredential credential) : this()
		{
		throw ADP.InvalidMixedArgumentOfSecureCredentialAndIntegratedSecurity();
		}

		if (UsesContextConnection(connectionOptions))
		else if (UsesContextConnection(connectionOptions))

Copy link

Contributor

benrr101Apr 16, 2025

Choose a reason for hiding this comment

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

nit: this one is one I've butted heads with people in the past. Technically, the else is redundant here since if the condition is triggered, we exit the method. In my IDE, I get notices about them, soI usually try to avoid them. But I admit, it is tidier to have the else-if chain. Though it would be even better if this could be written as a switch block :/