Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to Shared for the Microsoft.Data.SqlClient Reference project #1307

Closed

Conversation

lcheunglci
Copy link
Contributor

This is related to #1261 but it's not in the list. @cheenamalhotra mentioned that we would like to merge the two reference projects into one as the long term goal is to have one codebase. I merged the netfx csproj into netcore and combined the Microsoft.Data.SqlClient.cs. I ran MSBUILD on release and generated the nuget package and compared it before it was merged. The file size remained the same and I used NuGet Package Explorer to view the contents of the package and it looked the same.

@lcheunglci lcheunglci marked this pull request as ready for review October 4, 2021 17:01
@Wraith2
Copy link
Contributor

Wraith2 commented Oct 4, 2021

The implication of this merge would be that all builds of SqlClient have feature parity, and I didn't think that was the case.

Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

Please organize merges better in classes, you can search for method/type by their name before categorizing them and put them in 1 location only.

Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

Architecture looks good, few more changes needed.

@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview3 milestone Oct 7, 2021
@Wraith2
Copy link
Contributor

Wraith2 commented Oct 7, 2021

Do we need to have ifdefs and a single file? We could still have netcore and netfx files in the shared location and conditionally include them and I think that would be clearer.

@lcheunglci
Copy link
Contributor Author

I did notice there's already a Microsoft.Data.SqlClient.NetStandard.cs and Microsoft.Data.SqlClient.Manual.cs. I think having one for net framework in Microsoft.Data.SqlClient.NetFx.cs would make sense for clarity. I'll split it up in the next commit.

…he same for netfx and netcore. Move the ifdef for netfx to Microsoft.Data.SqlClient.NetFx.cs so it's cleaner and easier to read. Remove some ifdef where it says NETCOREAPP, NETSTANDARD or NETFRAMEWORK because it exists in all versions.
@cheenamalhotra cheenamalhotra removed this from the 4.0.0-preview3 milestone Oct 19, 2021
@cheenamalhotra cheenamalhotra added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Oct 25, 2021
@JRahnama JRahnama added this to the 5.0.0-preview1 milestone Dec 7, 2021
@DavoudEshtehari DavoudEshtehari removed this from the 5.0.0-preview2 milestone Apr 6, 2022
@DavoudEshtehari DavoudEshtehari added this to the 5.0.0-preview3 milestone Apr 6, 2022
@DavoudEshtehari DavoudEshtehari removed this from the 5.0.0-preview3 milestone May 28, 2022
@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #1307 (1bea873) into main (4e3aa5e) will decrease coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1307      +/-   ##
==========================================
- Coverage   71.54%   71.36%   -0.19%     
==========================================
  Files         291      291              
  Lines       61241    61241              
==========================================
- Hits        43817    43702     -115     
- Misses      17424    17539     +115     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.91% <ø> (-0.18%) ⬇️
netfx 69.19% <ø> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs 68.85% <0.00%> (-9.84%) ⬇️
...ata/SqlClient/SqlConnectionTimeoutErrorInternal.cs 41.96% <0.00%> (-9.83%) ⬇️
...ActiveDirectoryAuthenticationTimeoutRetryHelper.cs 56.81% <0.00%> (-6.82%) ⬇️
...rc/Microsoft/Data/SqlClient/SQLFallbackDNSCache.cs 90.62% <0.00%> (-6.25%) ⬇️
.../src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs 87.89% <0.00%> (-4.49%) ⬇️
...Microsoft/Data/ProviderBase/DbConnectionFactory.cs 17.04% <0.00%> (-3.41%) ⬇️
...e/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs 60.62% <0.00%> (-2.34%) ⬇️
...Client/Reliability/Common/SqlRetryLogicProvider.cs 89.13% <0.00%> (-2.18%) ⬇️
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs 68.85% <0.00%> (-0.82%) ⬇️
...rc/Microsoft/Data/ProviderBase/DbConnectionPool.cs 85.00% <0.00%> (-0.67%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e3aa5e...1bea873. Read the comment docs.

@JRahnama
Copy link
Contributor

Closing this PR as it not in a mergeable state.

@JRahnama JRahnama closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants