- Notifications
You must be signed in to change notification settings - Fork311
Merge | PacketHandle.*.cs#3241
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR moves the PacketHandle.netcore.Windows.cs and PacketHandle.netcore.Unix.cs files from the netcore project to the common project while updating file names to clarify that they only apply to netcore.
- Added conditional compilation directives (#if NET / #endif) to both files
- Reformatted comments and updated multi-line lambda formatting for readability
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.netcore.Unix.cs | Added #if NET wrapper and updated comment formatting and lambda method style |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.netcore.Windows.cs | Added #if NET wrapper, updated comment formatting, lambda method style, and reordered field declarations |
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj: Language not supported
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.netcore.Windows.cs:28
- The reordered field declarations (NativePacket before NativePointer) may affect memory layout if the struct relies on a specific field order. Please confirm that this change is intentional and does not impact any layout-dependent logic.
public readonly SNIPacket NativePacket;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops - are you compiling the Unix file?
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
codecovbot commentedMar 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #3241 +/- ##==========================================+ Coverage 72.69% 72.80% +0.10%========================================== Files 303 300 -3 Lines 59718 59611 -107 ==========================================- Hits 43414 43398 -16+ Misses 16304 16213 -91
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The reason there are two files is that at the time the common practice in this library was to avoid using compiler directives and use msbuild conditions. Since then I think everyone involved has got tired of the complexity involved and the amount of difficulty debugging msbuild so it might be worth considering just combining the two files and using compiler directives. /cc@David-Engel |
@Wraith2 yes, the eventual plan is to use compiler directives for the different OS builds. Not there yet, but it's in the plan. |
414f016 intomainUh oh!
There was an error while loading.Please reload this page.
Description: Very simple move of PacketHandle.Windows.cs and PacketHandle.Unix from the netcore project to the common project. File names updated to reflect that they only apply to netcore.
Testing: Literally no functional changes, just moving files.