Fix decimal parsing and allow string representation

[thomhurst]

No description provided.

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Fix decimal parsing and allow string representation

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files to understand decimal parsing improvements
• Review new test file for coverage adequacy
• Check code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Review security concerns
• Verify behavioral parity between source-generated and reflection modes
• Provide comprehensive feedback

Summary

This PR implements significant improvements to decimal parsing in TUnit's source generator, specifically addressing precision handling and string representation for decimal values that cannot be expressed as C# numeric literals (like decimal.MaxValue).

Code Quality & Best Practices ✅

Positive aspects:

• Modern C# patterns: Code follows TUnit's standards with proper use of pattern matching, var declarations, and collection expressions where appropriate
• Consistent formatting: Proper indentation, bracing style, and naming conventions throughout
• Good separation of concerns: Changes are logically distributed across appropriate classes (TypedConstantFormatter, CodeGenerationHelpers, etc.)
• Comprehensive test coverage: New test file includes edge cases like decimal.MaxValue, small decimals, and string literals

Areas for improvement:

• Complex logic in CodeGenerationHelpers.cs: The decimal handling logic (lines 102-122, 135-154, 166-184) is duplicated across multiple locations. Consider extracting this into a dedicated method like HandleDecimalArgument() to improve maintainability.

Potential Bugs & Issues ⚠️

1. String trimming logic: In TypedConstantParser.cs:34, the logic newExpression.ToString().TrimEnd('d') only removes 'd' suffixes but doesn't handle other numeric suffixes consistently. This could cause issues with other literal types.

2. Syntax index management: In CodeGenerationHelpers.cs, the syntaxIndex variable is manipulated with increment/decrement operations (lines 107, 113, 125, etc.). This complex state management could lead to index misalignment bugs, especially with mixed literal types.

3. Missing edge case: The decimal handling doesn't appear to handle negative decimal literals properly in all paths. Consider testing with [Arguments(-123.456)] to ensure the minus sign is preserved correctly.

Performance Considerations ✅

Good performance practices:

• String operations: Using InvariantCulture for consistent formatting across locales
• Lazy evaluation: Only processes decimal-specific logic when targetType is decimal
• Efficient type checking: Uses SpecialType.System_Decimal for fast type comparisons

Performance is maintained: No significant allocations or computational overhead introduced. The changes primarily affect code generation time, not runtime performance.

Security Concerns ✅

No security issues identified:

• Input validation: Decimal parsing uses decimal.TryParse() appropriately
• Code injection prevention: All generated code is properly escaped and formatted
• Type safety: Maintains strong typing throughout the conversion process

Behavioral Parity Analysis ✅

Critical requirement met: The implementation correctly maintains behavioral parity between source-generated and reflection modes:

• Source generator path: Enhanced decimal handling in TypedConstantFormatter and CodeGenerationHelpers
• Reflection path: The analyzer (TestDataAnalyzer.cs) includes corresponding logic for decimal validation (lines 928-945)
• Consistent validation: Both modes now support string literals for decimal parameters that can't be expressed as numeric literals

Test Coverage Assessment ✅

Excellent test coverage in DecimalArgumentTests.cs:

• ✅ High precision decimals (2_000.00000000000000001)
• ✅ String representation for extreme values (decimal.MaxValue, decimal.MinValue)
• ✅ Mixed argument scenarios
• ✅ Precision preservation validation
• ✅ Multiple decimal parameters

Suggested additional test cases:

[Test]
[Arguments(-123.456789)] // Negative decimals
public async Task NegativeDecimal(decimal value) { ... }

[Test]
[Arguments("0")] // Zero as string
public async Task ZeroAsString(decimal value) { ... }

Recommendations

1. Refactor duplicate logic: Extract decimal handling into a shared method to reduce code duplication in CodeGenerationHelpers.cs

2. Improve syntax index management: Consider using a more robust approach for tracking syntax argument positions, possibly with a dedicated state object

3. Add negative decimal tests: Include test cases for negative decimal values to ensure proper handling

4. Verify snapshot tests: Ensure dotnet test TUnit.Core.SourceGenerator.Tests passes and accept any intentional snapshot changes per CLAUDE.md guidelines

Conclusion

This is a solid implementation that successfully addresses decimal precision issues while maintaining TUnit's architectural principles. The code quality is high, performance impact is minimal, and behavioral parity is preserved. The main areas for improvement are around code organization and test coverage completeness.

Recommendation: Approve with minor suggestions - The core functionality is sound and ready for merge after addressing the optional refactoring suggestions.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Fix decimal parsing and allow string representation

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files to understand decimal parsing improvements
• Review new test file for coverage adequacy
• Check code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Review security concerns
• Verify behavioral parity between source-generated and reflection modes
• Provide comprehensive feedback

Summary

This PR implements significant improvements to decimal parsing in TUnit's source generator, specifically addressing precision handling and string representation for decimal values that cannot be expressed as C# numeric literals (like decimal.MaxValue).

Code Quality & Best Practices ✅

Positive aspects:

• Modern C# patterns: Code follows TUnit's standards with proper use of pattern matching, var declarations, and collection expressions where appropriate
• Consistent formatting: Proper indentation, bracing style, and naming conventions throughout
• Good separation of concerns: Changes are logically distributed across appropriate classes (TypedConstantFormatter, CodeGenerationHelpers, etc.)
• Comprehensive test coverage: New test file includes edge cases like decimal.MaxValue, small decimals, and string literals

Areas for improvement:

• Complex logic in CodeGenerationHelpers.cs: The decimal handling logic (lines 102-122, 135-154, 166-184) is duplicated across multiple locations. Consider extracting this into a dedicated method like HandleDecimalArgument() to improve maintainability.

Potential Bugs & Issues ⚠️

1. String trimming logic: In TypedConstantParser.cs:34, the logic newExpression.ToString().TrimEnd('d') only removes 'd' suffixes but doesn't handle other numeric suffixes consistently. This could cause issues with other literal types.

2. Syntax index management: In CodeGenerationHelpers.cs, the syntaxIndex variable is manipulated with increment/decrement operations (lines 107, 113, 125, etc.). This complex state management could lead to index misalignment bugs, especially with mixed literal types.

3. Missing edge case: The decimal handling doesn't appear to handle negative decimal literals properly in all paths. Consider testing with [Arguments(-123.456)] to ensure the minus sign is preserved correctly.

Performance Considerations ✅

Good performance practices:

• String operations: Using InvariantCulture for consistent formatting across locales
• Lazy evaluation: Only processes decimal-specific logic when targetType is decimal
• Efficient type checking: Uses SpecialType.System_Decimal for fast type comparisons

Performance is maintained: No significant allocations or computational overhead introduced. The changes primarily affect code generation time, not runtime performance.

Security Concerns ✅

No security issues identified:

• Input validation: Decimal parsing uses decimal.TryParse() appropriately
• Code injection prevention: All generated code is properly escaped and formatted
• Type safety: Maintains strong typing throughout the conversion process

Behavioral Parity Analysis ✅

Critical requirement met: The implementation correctly maintains behavioral parity between source-generated and reflection modes:

• Source generator path: Enhanced decimal handling in TypedConstantFormatter and CodeGenerationHelpers
• Reflection path: The analyzer (TestDataAnalyzer.cs) includes corresponding logic for decimal validation (lines 928-945)
• Consistent validation: Both modes now support string literals for decimal parameters that can't be expressed as numeric literals

Test Coverage Assessment ✅

Excellent test coverage in DecimalArgumentTests.cs:

• ✅ High precision decimals (2_000.00000000000000001)
• ✅ String representation for extreme values (decimal.MaxValue, decimal.MinValue)
• ✅ Mixed argument scenarios
• ✅ Precision preservation validation
• ✅ Multiple decimal parameters

Suggested additional test cases:

[Test]
[Arguments(-123.456789)] // Negative decimals
public async Task NegativeDecimal(decimal value) { ... }

[Test]
[Arguments("0")] // Zero as string
public async Task ZeroAsString(decimal value) { ... }

Recommendations

1. Refactor duplicate logic: Extract decimal handling into a shared method to reduce code duplication in CodeGenerationHelpers.cs

2. Improve syntax index management: Consider using a more robust approach for tracking syntax argument positions, possibly with a dedicated state object

3. Add negative decimal tests: Include test cases for negative decimal values to ensure proper handling

4. Verify snapshot tests: Ensure dotnet test TUnit.Core.SourceGenerator.Tests passes and accept any intentional snapshot changes per CLAUDE.md guidelines

Conclusion

This is a solid implementation that successfully addresses decimal precision issues while maintaining TUnit's architectural principles. The code quality is high, performance impact is minimal, and behavioral parity is preserved. The main areas for improvement are around code organization and test coverage completeness.

Recommendation: Approve with minor suggestions - The core functionality is sound and ready for merge after addressing the optional refactoring suggestions.

---