Skip to content

Conversation

MichelZ
Copy link
Contributor

@MichelZ MichelZ commented Nov 7, 2024

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

Copy link

codecov bot commented Nov 7, 2024

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 lines Patch % Lines
...fx/src/Microsoft/Data/SqlClient/TdsParser.netfx.cs 39.21% 31 Missing ⚠️
.../src/Microsoft/Data/SqlClient/TdsParser.netcore.cs 0.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     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.44% <0.00%> (+0.05%) ⬆️
netfx 71.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 MichelZ force-pushed the tdsparser-move-netcorenetfxcode branch from c634c01 to ac8aa33 Compare November 7, 2024 13:53
Copy link
Contributor

@benrr101 benrr101 left a comment

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 :)

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

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

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 21, 2024

@benrr101 Done! Thx

Sign up for free to 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