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

Merge to shared - SqlInternalConnection #1598

Merged
merged 7 commits into from
Oct 5, 2022

Conversation

lcheunglci
Copy link
Contributor

Relates to #1261. Merged netfx into netcore for SqlInternalConnection, and moving it to shared src and updating file to conform with coding style. There's a few IDE1006 naming violations because it's being referenced in SqlDataReader and SqlCommand, so I left those alone.

@Wraith2
Copy link
Contributor

Wraith2 commented Apr 30, 2022

I think once the codebases a merged we'll be able to do another pass to clean up naming and style.

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Base: 71.67% // Head: 71.47% // Decreases project coverage by -0.20% ⚠️

Coverage data is based on head (2e63a80) compared to base (4cb2b67).
Patch coverage: 58.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1598      +/-   ##
==========================================
- Coverage   71.67%   71.47%   -0.21%     
==========================================
  Files         293      292       -1     
  Lines       61376    61218     -158     
==========================================
- Hits        43990    43754     -236     
- Misses      17386    17464      +78     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 75.02% <50.00%> (-0.20%) ⬇️
netfx 69.32% <58.33%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
.../Microsoft/Data/SqlClient/SqlInternalConnection.cs 72.77% <58.33%> (ø)
...ActiveDirectoryAuthenticationTimeoutRetryHelper.cs 56.81% <0.00%> (-18.19%) ⬇️
...nt/src/Microsoft/Data/ProviderBase/TimeoutTimer.cs 78.57% <0.00%> (-17.86%) ⬇️
...ata/SqlClient/SqlConnectionTimeoutErrorInternal.cs 41.96% <0.00%> (-9.83%) ⬇️
...t/Data/ProviderBase/DbConnectionPool.NetCoreApp.cs 75.00% <0.00%> (-6.82%) ⬇️
...Client/Reliability/Common/SqlRetryLogicProvider.cs 89.13% <0.00%> (-2.18%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 81.72% <0.00%> (-1.26%) ⬇️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs 77.31% <0.00%> (-1.21%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 67.71% <0.00%> (-0.96%) ⬇️
...rc/Microsoft/Data/ProviderBase/DbConnectionPool.cs 85.63% <0.00%> (-0.88%) ⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JRahnama JRahnama added the ➕ Code Health Changes related to source code improvements label Aug 16, 2022
@JRahnama JRahnama added this to In Progress in SqlClient v5.1 via automation Aug 16, 2022
@JRahnama JRahnama added this to the 5.1.0-preview1 milestone Aug 16, 2022
Comment on lines 30 to 32
#if NETFRAMEWORK
private bool _isAzureSQLConnection = false; // If connected to Azure SQL
#else
Copy link
Member

Choose a reason for hiding this comment

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

Just for having a more transparent code, I'd suggest keeping netfx-specific source code together as much as possible instead of scattered if-else directives.

Comment on lines 282 to 296
catch (System.OutOfMemoryException e)
{
Connection.Abort(e);
throw;
}
catch (System.StackOverflowException e)
{
Connection.Abort(e);
throw;
}
catch (System.Threading.ThreadAbortException e)
{
Connection.Abort(e);
BestEffortCleanup(bestEffortCleanupTarget);
throw;
Copy link
Member

Choose a reason for hiding this comment

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

This section can be added to the netcore as well.
cc @David-Engel

Comment on lines 377 to 392
catch (OutOfMemoryException)
{
DoomThisConnection();
throw;
}
catch (StackOverflowException)
{
DoomThisConnection();
throw;
}
catch (System.Threading.ThreadAbortException)
{
DoomThisConnection();
BestEffortCleanup(bestEffortCleanupTarget);
throw;
}
Copy link
Member

Choose a reason for hiding this comment

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

This section can be added to the netcore as well.

Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

I see more opportunities to change like "Order modifiers" and "Expression body" that don't affect the program logic. So, I'll leave it to your choice. 🙂

SqlClient v5.1 automation moved this from In Progress to Review Approved Oct 5, 2022
@JRahnama JRahnama merged commit eb1b922 into dotnet:main Oct 5, 2022
SqlClient v5.1 automation moved this from Review Approved to Done Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Changes related to source code improvements
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants