-
-
Notifications
You must be signed in to change notification settings - Fork 105
feat: add DecimalEqualsAssertion for decimal comparisons with tolerance #3496
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
Conversation
|
Claude finished @thomhurst's task —— View job Code Review Complete ✅I've thoroughly reviewed the DecimalEqualsAssertion implementation. Here's my comprehensive analysis: 🟢 Positive AspectsImplementation Quality
Test Coverage
API Consistency
🟡 Areas for ImprovementCritical: Missing Dual-Mode ImplementationAccording to TUnit's MANDATORY RULE #1 from CLAUDE.md: "ALL changes must work identically in both source-generated and reflection modes" Issue: The implementation only adds the assertion class but doesn't include corresponding reflection-mode support in Evidence:
Recommendation: Verify that the reflection engine can properly discover and execute decimal equality assertions, or add explicit support if needed. Tolerance ValidationThe current implementation doesn't validate tolerance values: // Consider adding validation in DecimalEqualsAssertion.Within()
if (tolerance < 0)
throw new ArgumentException("Tolerance must be non-negative", nameof(tolerance));Documentation
🟢 Security & PerformanceSecurity: ✅ No Concerns
Performance: ✅ Optimized
📋 Compliance ChecklistBased on TUnit's verification requirements:
🎯 Recommendations
🏆 Overall AssessmentThis is a high-quality implementation that follows TUnit's established patterns and provides a valuable feature for decimal comparisons. The code is clean, well-tested, and consistent with the framework's design principles. The main concern is ensuring compliance with TUnit's dual-mode requirement. Recommendation: ✅ Approve with verification - Confirm reflection mode support, then this PR is ready to merge. |
Fixes #3494