-
Notifications
You must be signed in to change notification settings - Fork 315
[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
base: release/5.1
Are you sure you want to change the base?
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
SqlDecimalHelper
reflection-based logic and adds a lean method that uses the documentedData
property. - Adds region/doc comments to mirror the NetCore
WriteTdsValue
API and cleans up unusedusing
directives. - 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, butSqlDecimal
uses separatem_data1
–m_data4
fields. 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
SqlDecimalExtractData
logic 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.83% 68.62% -3.21%
===============================================
Files 293 293
Lines 61647 61586 -61
===============================================
- Hits 44283 42264 -2019
- Misses 17364 19322 +1958
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.