Fix DatabaseTypeRequest.Equals to use type-specific semantics (v2.0.3) #20
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
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on c197d5e..64a4767
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (3)
• CHANGELOG.md
• Tests/DatabaseTypeRequestEnhancedTests.cs
• TypeGuesser/DatabaseTypeRequest.cs
Copilot finished reviewing on behalf of
jas88
November 19, 2025 02:08
Pull Request Test Coverage Report for Build 19487755250Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes equality semantics for DatabaseTypeRequest to use type-specific comparisons, resolving issue #19 where semantically equivalent objects were incorrectly considered unequal. The fix ensures that only properties relevant to each SQL type are compared during equality checks.
- Updated
Equals()andGetHashCode()methods with type-specific logic - Added 17 comprehensive tests covering all type-specific equality scenarios
- Documented the breaking change in CHANGELOG.md
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| TypeGuesser/DatabaseTypeRequest.cs | Implemented type-specific equality logic for decimal, string, byte[], and other types |
| Tests/DatabaseTypeRequestEnhancedTests.cs | Added comprehensive test coverage for type-specific equality behavior |
| CHANGELOG.md | Documented the breaking change and fix details |
| data/openmemory.sqlite* | Binary SQLite database files (test artifacts) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jas88
added a commit
that referenced
this pull request
Nov 19, 2025
Addresses Copilot AI review feedback on PR #20. The Width property computes Math.Max(_maxWidthForStrings, Size.ToStringLength()), which means comparing the Width property directly for string and byte[] types was incorrect. Two instances with the same _maxWidthForStrings but different Size values would have different Width property values, causing false negatives in equality checks. Fixed by comparing the _maxWidthForStrings backing field directly instead of the computed Width property for string and byte[] types. Added 2 comprehensive tests to verify the edge case: - Equals_String_WithSameWidthButDifferentSize_AreEqual - Equals_ByteArray_WithSameWidthButDifferentSize_AreEqual All 396 tests pass.
Copilot finished reviewing on behalf of
jas88
November 19, 2025 02:21
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Resolves #19 DatabaseTypeRequest.Equals() now compares only the properties relevant to each type's SQL representation: - decimal: Compare CSharpType and Size (precision/scale) only Ignores Width and Unicode which are irrelevant for DECIMAL(p,s) - string: Compare CSharpType, Width, and Unicode Ignores Size which is irrelevant for VARCHAR/NVARCHAR - byte[]: Compare CSharpType and Width Ignores Size and Unicode which are irrelevant for VARBINARY(n) - All other types (bool, int, long, float, double, DateTime, TimeSpan, Guid, etc.): Compare only CSharpType These have fixed SQL storage sizes This fixes round-trip equality when comparing guesser-created DatabaseTypeRequest objects (which may have Width set) with SQL reverse-engineered objects (which typically have Width=null for numeric types). GetHashCode() updated to match new Equals() semantics for consistency. Added 17 comprehensive tests covering all type-specific equality cases. All 394 tests pass with 0 warnings.
Complete release notes with version number and date for v2.0.2. Version is already set to 2.0.2 in SharedAssemblyInfo.cs.
The data/ directory contains OpenMemory SQLite database files that should not be tracked in version control.
Addresses Copilot AI review feedback on PR #20. The Width property computes Math.Max(_maxWidthForStrings, Size.ToStringLength()), which means comparing the Width property directly for string and byte[] types was incorrect. Two instances with the same _maxWidthForStrings but different Size values would have different Width property values, causing false negatives in equality checks. Fixed by comparing the _maxWidthForStrings backing field directly instead of the computed Width property for string and byte[] types. Added 2 comprehensive tests to verify the edge case: - Equals_String_WithSameWidthButDifferentSize_AreEqual - Equals_ByteArray_WithSameWidthButDifferentSize_AreEqual All 396 tests pass.
1. Fix potential NullReferenceException in GetHashCode() - Changed from CSharpType.GetHashCode() to HashCode.Combine(CSharpType) - HashCode.Combine handles null values properly 2. Update CHANGELOG for accuracy - Clarify that string/byte[] compare _maxWidthForStrings (backing field) - Not the Width property which includes Size.ToStringLength() - Add note about avoiding Size interference - Mention null-safety improvement in GetHashCode() - Update test count to 19 (added 2 edge case tests) All 396 tests pass.
1. Remove misleading 'null-safety' mention from CHANGELOG - The improvement was about matching equality semantics, not null-safety - Both old and new implementations use HashCode.Combine which handles nulls 2. Fix misleading comment in byte[] edge case test - Comment said Width values are different, but they're actually the same - Clarified that Width is same (1000) despite different Size values - This is exactly what the test is verifying
jas88
force-pushed
the
fix/decimal-equals-ignore-width-unicode
branch
from
afd8dcc to
49f0b4f
Compare
jas88
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes #19 -
DatabaseTypeRequest.Equals()now uses type-specific equality semantics, comparing only the properties that are semantically meaningful for each SQL type.Problem
Previously,
Equals()compared all properties (CSharpType,Width,Size,Unicode) regardless of type, causing false negatives when comparing semantically equivalent objects:DatabaseTypeRequest(decimal, Width=4, Size=DecimalSize(4,1))DatabaseTypeRequest(decimal, Width=null, Size=DecimalSize(4,1))These represent the same
DECIMAL(5,1)type but were considered not equal due to differentWidthvalues.Solution
Updated
Equals()to compare only relevant properties per type:CSharpType,Size(precision/scale)Width,UnicodeCSharpType,_maxWidthForStrings,UnicodeSizeCSharpType,_maxWidthForStringsSize,UnicodeCSharpTypeonlyWidth,Size,UnicodeOther types: bool, byte, short, int, long, float, double, DateTime, TimeSpan, Guid (fixed SQL storage sizes)
Changes
TypeGuesser/DatabaseTypeRequest.cs:
Equals()with type-specific comparison logicGetHashCode()to match new equality semantics_maxWidthForStringsbacking field instead of computedWidthproperty (Copilot feedback)Tests/DatabaseTypeRequestEnhancedTests.cs:
CHANGELOG.md:
Test Results
✅ All 396 tests pass
✅ 0 warnings, 0 errors
✅ AOT/trim compatible (no reflection added)
Copilot AI Review Addressed
Copilot identified a critical bug where comparing the computed
Widthproperty (which includesSize.ToStringLength()) caused incorrect equality for string/byte[] types. Fixed by comparing the_maxWidthForStringsbacking field directly.Breaking Change
This is technically a breaking change in equality semantics, but it fixes incorrect behavior. Code relying on the old (incorrect) behavior where
Widthaffected decimal equality may need updating.