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

Code Health | Cleanup SNIMARSConnection #1985

Merged
merged 4 commits into from May 3, 2023

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Apr 10, 2023

In #933 many changes were made to SNIMarsConnection which were then lost when that PR was reverted. This PR implements the harmless and side-effect-less changes such as using scoped event tracing like the rest of SNI does, cleaning up a class name variable and adding a dedicated sync object (there are no locks on instances of this class outside this file, i've checked).

This does not change the demuxing code in any way. I want to get the files cleaned up and then try again with the muxing code in a future PR. this is a simple way to get easy changes in place minimizing review complexity.

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Patch coverage: 78.57% and project coverage change: +0.36 🎉

Comparison is base (a5ad838) 70.59% compared to head (caeea0f) 70.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1985      +/-   ##
==========================================
+ Coverage   70.59%   70.95%   +0.36%     
==========================================
  Files         306      306              
  Lines       61667    64036    +2369     
==========================================
+ Hits        43533    45437    +1904     
- Misses      18134    18599     +465     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.67% <78.57%> (+0.27%) ⬆️
netfx 69.55% <ø> (+0.41%) ⬆️

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

Impacted Files Coverage Δ
.../Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs 77.70% <78.57%> (+0.97%) ⬆️

... and 15 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JRahnama
Copy link
Member

/azurepipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Wraith2
Copy link
Contributor Author

Wraith2 commented Apr 10, 2023

test failure is the usual unreliable reliability test and 3 legs failed to configure their firewall.

@JRahnama
Copy link
Member

/azurepiplines run

@JRahnama JRahnama changed the title Cleanup SNIMARSConnection Code Health | Cleanup SNIMARSConnection May 3, 2023
@JRahnama JRahnama merged commit bc0a1a6 into dotnet:main May 3, 2023
131 checks passed
@DavoudEshtehari DavoudEshtehari added ➕ Code Health Changes related to source code improvements netcore labels May 4, 2023
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
@Wraith2 Wraith2 deleted the cleanup-snimarsconnection branch August 13, 2023 13:38
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 netcore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants