Merge | SqlConnection cosmetic changes #3275
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
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.
| = new(concurrencyLevel: 4 * Environment.ProcessorCount /* default value in ConcurrentDictionary*/, | ||
| capacity: 1, | ||
| comparer: StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| internal bool ForceNewConnection |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 is dotnet/wpf#10270.
| /// <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 |
There was a problem hiding this comment.
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...
| @@ -178,43 +173,35 @@ public SqlConnection(string connectionString, SqlCredential credential) : this() | |||
| { | |||
| throw ADP.InvalidMixedArgumentOfSecureCredentialAndIntegratedSecurity(); | |||
| } | |||
|
|
|||
| if (UsesContextConnection(connectionOptions)) | |||
| else if (UsesContextConnection(connectionOptions)) | |||
There was a problem hiding this comment.
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, so I 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 :/
benrr101
force-pushed
the
merge/sqlconnection-preparation
branch
from
6ca56d0 to
bdde88e
Compare
|
@edwardneal ok that was more dangerous, but yeahhhh I just rebased your branch off of main and fixed the merge conflicts (please don't be mad) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
benrr101
added
the
Code Health 💊
|
Thanks rebasing this @benrr101. I noticed that CI was failing on the netfx project, so I've adjusted it slightly. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributes to #1261.
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?