Skip to content

Merge | Move ReliabilitySection / BestEffortCleanup to framework specific files - #2984

Merged
benrr101 merged 7 commits into
dotnet:mainfrom
MichelZ:tdsparser-move-netcorenetfxcode
Nov 25, 2024
Merged

Merge | Move ReliabilitySection / BestEffortCleanup to framework specific files#2984
benrr101 merged 7 commits into
dotnet:mainfrom
MichelZ:tdsparser-move-netcorenetfxcode

Conversation

@MichelZ

Copy link
Copy Markdown
Contributor

I don't think we want to wrap those in #IF NETFRAMEWORK / NETCORE conditional compiles, it makes sense to have them in their own files, so I've moved them out of the "future-common-project" file(s)

Part of #2953

@MichelZMichelZ mentioned this pull request Nov 7, 2024
@codecov

codecovBot commented Nov 7, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 37.03704% with 34 lines in your changes missing coverage. Please review.

Project coverage is 72.67%. Comparing base (1b9df10) to head (f97eaf4).
Report is 8 commits behind head on main.

Files with missing linesPatch %Lines
...fx/src/Microsoft/Data/SqlClient/TdsParser.netfx.cs39.21%31 Missing ⚠️
.../src/Microsoft/Data/SqlClient/TdsParser.netcore.cs0.00%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #2984 +/- ##
==========================================
+ Coverage 72.64% 72.67% +0.02% 
==========================================
Files 285 286 +1 Lines 59160 59160 ==========================================
+ Hits 42979 42995 +16 + Misses 16181 16165 -16 
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore75.44% <0.00%> (+0.05%)⬆️
netfx71.07% <39.21%> (-0.02%)⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichelZ
MichelZforce-pushed the tdsparser-move-netcorenetfxcode branch from c634c01 to ac8aa33CompareNovember 7, 2024 13:53

@benrr101benrr101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall I think this makes sense and is probably a good idea. Still not a huge fan of the conditional attribute, but since it's already there, I won't raise a fuss :)

@benrr101benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 14, 2024
@benrr101

Copy link
Copy Markdown
Contributor

@MichelZ if you can resolve the conflict, I can get this merged 🚢

@MichelZ

Copy link
Copy Markdown
ContributorAuthor

@benrr101 Done! Thx

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Common Project 🚮Things that relate to the common project project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@MichelZ@benrr101@cheenamalhotra