-
Notifications
You must be signed in to change notification settings - Fork 317
[5.1] SqlDecimal Extract Data #3465
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
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
Backports a simplified SqlDecimalExtractData implementation for .NET Framework by replacing the reflection/serialization workaround with direct use of SqlDecimal.Data, and cleans up related code and usings.
- Removes the old
SqlDecimalHelperreflection-based logic and adds a lean method that uses the documentedDataproperty. - Adds region/doc comments to mirror the NetCore
WriteTdsValueAPI and cleans up unusedusingdirectives. - Adjusts formatting and links for other type workarounds (e.g.,
SqlMoney,SqlBinary,SqlGuid).
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.netfx.cs:144
- [nitpick] The comment references
m_data[1-4]as if it’s an indexed field array, butSqlDecimaluses separatem_data1–m_data4fields. It may be clearer to rephrase to avoid implying a non-existent array.
// Note: Although it would be faster to use the m_data[1-4] member variables in
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.netfx.cs:137
- This new
SqlDecimalExtractDatalogic should be covered by unit tests to verify correct field extraction (e.g., for various scales, signs, and null values). Consider adding tests to prevent future regressions.
internal static void SqlDecimalExtractData(
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.netfx.cs
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/5.1 #3465 +/- ##
===============================================
- Coverage 71.82% 71.04% -0.79%
===============================================
Files 293 293
Lines 61659 61598 -61
===============================================
- Hits 44289 43763 -526
- Misses 17370 17835 +465
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:
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Due to legal reasons, we have to take a change to the SqlDecimal type workarounds in netfx.
Issues
N/A
Testing
This is a drop-in replacement for the existing method, CI should successfully verify.