Serialise scale for vartime SqlParameter types that have been explicitly set by the user to zero #2411
Conversation
…y the user to zero
|
It wasn't stuck in limbo it was decided against, see #1380 (comment) The only way you're going to get the change implemented is with an appcontext opt-in, if you include that then it might be includable. You'll need to add tests. You'll need to change the documentation. |
Apologies, I'd assumed that this was blocked based on this #1381 (comment) I'm really out of options for how to mitigate the performance impact of this short of moving off MSSQL or forking this library. I really don't want to do either :( I'll chip away and attempt to make this change based on an app context switch, try to get some testing around it and update the docs and then and see if I can get some traction. Also, it looks like the failed checks are unrelated to this pull request as best that I can tell, I'm really not sure how to get the checks to pass. |
|
It looks like the explicit calling out of this behaviour in the docs for the previous library System.Data.SqlClient as referred to here: #1380 (comment) are no longer in the docs for Microsoft.Data.SqlClient. Therefore I don't think there are any doc changes to make as part of this pull request. Still working on tests :) |
|
@Wraith2 pull request updated with context switch and tests. I don't believe there are documentation updates required for the Scale parameter itself as noted above, but the existing context switches do appear in documentation here: https://learn.microsoft.com/en-us/sql/connect/ado-net/appcontext-switches?view=sql-server-ver16 but I'm not sure where that lives. Any guidance would be appreciated. |
|
@Wraith2 Incidentally I noticed that the UseMinimumLoginTimeout context switch changed default behaviour in the last refactor of LocalAppContextSwitches in November 2023 and now returns a default value of "false" though the triple slash summary still says it should have a default of "true". The documentation @ https://learn.microsoft.com/en-us/sql/connect/ado-net/appcontext-switches?view=sql-server-ver16 does not give any guidance on default value but it looks to me like this default value change was unintended, but I think you would be better placed to determine that. |
|
Can you open a new issue with the details on that default so it can be reviewed please. |
EamonHetherton
changed the title
EamonHetherton
changed the title
|
Is there anything more I can do to move this along? failing that is there anywhere for me to get access to the pull request nuget package that was built "Microsoft.Data.SqlClient.6.0.0-pull.107808"? |
|
No. You just wait for the team to get to it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2411 +/- ##
==========================================
+ Coverage 72.66% 72.70% +0.04%
==========================================
Files 313 313
Lines 61737 61747 +10
==========================================
+ Hits 44861 44895 +34
+ Misses 16876 16852 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
revive old pull request #1381 to fix #1380 that got stuck in limbo due to a code freeze