Cleanup | Merge SqlTypes resource string methodology #3733
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors error message handling by removing the legacy SQLResource class and migrating its functionality to the existing SQLMessage class in SqlUtil.cs and the ADP class in AdapterUtil.cs. This consolidates error message handling into a consistent pattern using the StringsHelper class.
Key Changes:
- Removed the
SQLResource.csfile entirely - Migrated
SQLResource.NullStringtoSQLMessage.NullString()method - Migrated
SQLResource.ConversionOverflowMessagetoSQL.ConversionOverflow()method - Migrated
SQLResource.InvalidArraySizeMessageusage toADP.InvalidArraySize()method - Updated all references in production and test code
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/Microsoft/Data/SqlTypes/SQLResource.cs |
Deleted legacy resource class containing static error message strings |
src/Microsoft/Data/SqlTypes/SqlVector.cs |
Updated to use SQLMessage.NullString() and ADP.InvalidArraySize() instead of SQLResource |
src/Microsoft/Data/SqlClient/SqlUtil.cs |
Added SQL.ConversionOverflow() and SQLMessage.NullString() methods to provide error messages |
src/Microsoft/Data/SqlClient/SqlBuffer.cs |
Updated exception throwing to use SQL.ConversionOverflow() instead of SQLResource |
src/Microsoft/Data/Common/AdapterUtil.cs |
Added ADP.InvalidArraySize() method to handle invalid array size exceptions |
netfx/src/Microsoft.Data.SqlClient.csproj |
Removed reference to deleted SQLResource.cs file |
netcore/src/Microsoft.Data.SqlClient.csproj |
Removed reference to deleted SQLResource.cs file |
tests/UnitTests/.../SqlVectorTest.cs |
Updated test assertions to use SQLMessage.NullString() instead of SQLResource |
paulmedynski
approved these changes
Oct 30, 2025
apoorvdeshmukh
approved these changes
Oct 30, 2025
priyankatiwari08
approved these changes
Nov 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The SqlTypes namespace has had its own
SQLResourceclass since its inception, sat alongside the existingStrings,StringsHelper,SQLandSQLMessageclasses. This only had fields referring to twenty resource strings, and only four of these fields were used. I've removed the unused fields and moved the used references to the existing locations.Besides a modest code cleanup, this also clears the way for a broader point: we've got several hundred resource strings in the library which aren't used. Based on their contents, I think they were pulled in during the split from System.Data for the initial upload. Once everyone's happy with this PR, I plan to remove these strings.
Issues
None.
Testing
The
SqlVectorTestsclass was modified, and this passes. I'm not expecting any other impact - any issues should be flagged by a simple compilation.