-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add SqlTypes apis for SqlClient #72724
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @cheenamalhotra, @David-Engel Issue Detailscloses #51836 Implements changes to SqlDecimal, SqlBinary and SqlMoney. Changes to SqlGuid are being handled by changing the implementation of SqlGuid in #72549 and the api for SqlDateTime is now obsolete because the implementation was pulled up into SqlClient for performance improvements after the original api request was opened, that work was dotnet/SqlClient#912 and I chose not to try and backport the change to /runtime because I don't want to encourage people to use SqlTypes. /cc @MichalPetryka, @David-Engel
|
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.
LGTM. Thanks, @Wraith2!
Failures aren't related to these changes. So as far as I can tell this is good to merge. So have the permissions been sorted so you can merge @David-Engel or do we need someone who can do that for us? |
The Squash and merge button isn't disabled for me, so I appear to have write access. I'm good with this going into 8. There is no rush to force this into 7, if it's already missed the train. I don't see SqlClient building a target just for this change against 7, which I don't think is going to be an LTS release. But if we can version logic the SqlClient code, I'm good if you want to try and get this ported to 7. |
The runtime being on yearly releases means that if we miss 7 we have to wait a year so I'd rather have this available and ready to use if we get the chance from SqlClient. The changes are strictly additive so they don't pose any problem other than documentation. If the SqlGuid change PR is merged for 7 then we absolutely have to have an SqlClient release ready at the same time the runtime or people will be unable to read guids, in that case since we would be forced to have a net7 target it would make sense to include these changes at the same time. |
We are planning on adding a .NET 6 target for the 5.1 release. I was hoping to not have to add a .NET 7 one so it would be nice if we could logic block this behavior in the code rather than create a new target for it. If we service SDS, we'll need to do the same thing, too. I don't know if we can even add a target to the SDS package. |
The changes in this PR are entirely optional for SqlClient. We can pick them up whenever we choose. |
closes #51836
Implements changes to SqlDecimal, SqlBinary and SqlMoney. Changes to SqlGuid are being handled by changing the implementation of SqlGuid in #72549 and the api for SqlDateTime is now obsolete because the implementation was pulled up into SqlClient for performance improvements after the original api request was opened, that work was dotnet/SqlClient#912 and I chose not to try and backport the change to /runtime because I don't want to encourage people to use SqlTypes.
/cc @MichalPetryka, @David-Engel