Merge some netcore and netfx files #871
Conversation
| @@ -730,7 +730,7 @@ override public string CommandText | |||
| { | |||
| SqlClientEventSource.Log.TryTraceEvent("<sc.SqlCommand.set_CommandText|API> {0}, String Value = '{1}'", ObjectID, value); | |||
|
|
|||
| if (0 != ADP.SrcCompare(_commandText, value)) | |||
| if (_commandText != value) | |||
There was a problem hiding this comment.
Sort of. The intent was to clean up ADP as well but the PR was getting too large and complicated so I stopped part way. This change on it's own isn't harmful though so I left it in.
Functions like SrcCompare and DestCompare which just do string comparisons don't really have any benefit to being functions instead of inline, they're trivial and not worth the extra effort required to investigate what they're doing.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameterCollection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/NameValuePair.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/FieldNameLookup.cs
Show resolved
Hide resolved
|
@cheenamalhotra any chance of getting this PR reviewed next? I've got a few things I'd like to be able to change in SqlParameter that it's blocking. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Apart from the minor nit-pick issues, LGTM.
I know we have a lot of these types of refactors ahead of us. As a suggestion for future ones, it might be helpful to reviewers to break the refactor into multiple steps/commits. For example, keep any method/property re-ordering to a single commit without other changes. That way someone can review each commit to see a "no change" re-order commit, followed by a commit that has cleanup of code within the methods. This will make it much easier to identify potential differences that may introduce bugs and give us more confidence in approving them.
I do appreciate the work!
| <Compile Include="Microsoft\Data\SqlClient\SqlCommand.cs"> | ||
| <SubType>Component</SubType> | ||
| </Compile> |
There was a problem hiding this comment.
Revert these subtype component changes.
I wish there was a way to tell VS to stop trying to turn these classes into Components...
There was a problem hiding this comment.
Me too. The only say i've found to deal with them is to keep the project unloaded but that isn't helpful when trying to merge thing. They're cleared.
There was a problem hiding this comment.
untested drive-by... but: adding [System.ComponentModel.DesignerCategory("")] to the class seems to be the magic incantation :)
There was a problem hiding this comment.
I'll give that a try, thanks.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs
Show resolved
Hide resolved
To answer your question, no. We only have to support versions that are within extended support (10 years). So SQL 2012 and up, at this point. |
In a future merge the 2005 code can be dropped then.
This one did end up being too large. I've tried to keep subsequent ones smaller. I'll bear in mind the phased approach in future. |
SqlCollection and SqlParameter were defined over two files in both solutions and some of the files were in different places. They are now defined in a single file each. SqlParameterCollection is now shared between both implementations. SqlParameter is not shared because there are differences; is sql 2005 supported in this library? Some other files have been changed to support the major files being merged.
The majority of changes are SqlParameter and looking at those diffs is very difficult. I suggest pulling the changed files locally and reviewing the files themselves.