Skip to content

Merge | Netfx SqlTypeWorkarounds#3448

Merged
benrr101 merged 10 commits intomainfrom
dev/russellben/merge/sqltypeworkarounds-netfx
Jul 9, 2025
Merged

Merge | Netfx SqlTypeWorkarounds#3448
benrr101 merged 10 commits intomainfrom
dev/russellben/merge/sqltypeworkarounds-netfx

Conversation

@benrr101
Copy link
Contributor

Description

This PR is less of a merge and more of a total rewrite 😅 . There are a handful of methods that are used in the code to convert between internal representations of a handful of Sql* types (SqlBinary, SqlDecimal, SqlGuid, and SqlMoney). In netcore, we can use built-in methods to get at these internal representations, but netfx will not have access to them. There are equivalent conversion methods available, but they almost always make copies of the data. Thus, we have these methods that directly access the data or internal conversion method for the best performance when reading these types.

The original implementations of these were clunky and confusing, and although it'd be easy to copy-paste them into the common project file, I wanted to make sure we knew what was going on (and whether they were being used). Thus, with a bit of AI magic, I rewrote these conversion methods. Now, I'm sure no one will trust that I did it right - I mean, I don't even trust myself! So, this is my first PR where I'm introducing true unit tests! These were checked against the behavior of the original implementations, so it can be safely assumed that the new implementations do things correctly. I have also added comments to the methods to give a better understanding of how they work, for future people.

Also, say goodbye to dynamic IL method creation!

Issues

#1261

Testing

Added unit tests for the rewritten methods!

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

Labels

Code Health 💊 Issues/PRs that are targeted to source code quality improvements. Common Project 🚮 Things that relate to the common project project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants