refactor: preserve original trivia in syntax transformations and clean up trailing newlines in migration code

[thomhurst]

Fixes #3621

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3625

I'll review this pull request focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files for code quality
• Check for potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide final review summary

✅ Overall Assessment: APPROVED

This PR successfully addresses trivia preservation and cleanup issues in migration code fixers. The implementation is solid and follows TUnit's coding standards.

🎯 Key Improvements

1. Trivia Preservation Enhancement

   • AssertionRewriter.cs:22-25: Properly preserves original whitespace and comments during syntax transformations
   • BaseMigrationCodeFixProvider.cs:190-194: Hook attributes maintain their original trivia structure

2. Comprehensive Cleanup Logic

   • CleanupClassMemberLeadingTrivia(): Removes excessive blank lines after class opening braces
   • CleanupEndOfFileTrivia(): Eliminates trailing newlines at file end
   • Smart trivia handling in XUnit's TheoryData conversion preserves brace placement

🔍 Code Quality: Excellent

• ✅ Modern C# patterns (collection expressions, pattern matching)
• ✅ Follows TUnit's coding standards from CLAUDE.md
• ✅ Proper error handling with try-catch returning original document on failure
• ✅ Good separation of concerns with specialized rewriters

🐛 Potential Issues: Minor

1. Performance Consideration (XUnitMigrationCodeFixProvider.cs:56-72):

   // Multiple compilation updates could be expensive for large files
   UpdateSyntaxTrees(ref compilation, ref syntaxTree, ref updatedRoot);

   This iterative approach is necessary for complex transformations but could be slow for large codebases. However, it's the correct approach for maintaining semantic accuracy.

2. Edge Case (BaseMigrationCodeFixProvider.cs:108-109):

   currentRoot = currentRoot.RemoveNode(parameterSyntax, SyntaxRemoveOptions.KeepNoTrivia)!;

   The null-forgiving operator assumes RemoveNode never returns null, which is safe here but worth noting.

⚡ Performance: Good

• Efficient use of SyntaxRemoveOptions.KeepNoTrivia to prevent unnecessary trivia propagation
• Batch removal of multiple nodes where possible (RemoveNodes vs individual RemoveNode calls)
• LINQ usage is appropriate for non-hot-path analyzer code

🔒 Security: No Concerns

• No dynamic code generation or reflection usage
• Safe handling of syntax tree transformations
• Proper null checking throughout

🧪 Test Coverage: Adequate

• Test files show coverage for MSTest and XUnit migration scenarios
• Tests verify both positive and negative cases for attribute conversion
• New trivia preservation logic would benefit from explicit formatting tests

📝 Minor Suggestions

1. Consider adding XML documentation for the new cleanup methods in BaseMigrationCodeFixProvider.cs
2. The UpdateSyntaxTrees method could benefit from a comment explaining why multiple compilation updates are necessary

🎉 Excellent Work!

This PR demonstrates thoughtful engineering around a complex problem (preserving formatting during syntax transformations). The solution is robust, handles edge cases well, and maintains TUnit's high code quality standards.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3625

I'll review this pull request focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files for code quality
• Check for potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide final review summary

✅ Overall Assessment: APPROVED

This PR successfully addresses trivia preservation and cleanup issues in migration code fixers. The implementation is solid and follows TUnit's coding standards.

🎯 Key Improvements

1. Trivia Preservation Enhancement

   • AssertionRewriter.cs:22-25: Properly preserves original whitespace and comments during syntax transformations
   • BaseMigrationCodeFixProvider.cs:190-194: Hook attributes maintain their original trivia structure

2. Comprehensive Cleanup Logic

   • CleanupClassMemberLeadingTrivia(): Removes excessive blank lines after class opening braces
   • CleanupEndOfFileTrivia(): Eliminates trailing newlines at file end
   • Smart trivia handling in XUnit's TheoryData conversion preserves brace placement

🔍 Code Quality: Excellent

• ✅ Modern C# patterns (collection expressions, pattern matching)
• ✅ Follows TUnit's coding standards from CLAUDE.md
• ✅ Proper error handling with try-catch returning original document on failure
• ✅ Good separation of concerns with specialized rewriters

🐛 Potential Issues: Minor

1. Performance Consideration (XUnitMigrationCodeFixProvider.cs:56-72):

   // Multiple compilation updates could be expensive for large files
   UpdateSyntaxTrees(ref compilation, ref syntaxTree, ref updatedRoot);

   This iterative approach is necessary for complex transformations but could be slow for large codebases. However, it's the correct approach for maintaining semantic accuracy.

2. Edge Case (BaseMigrationCodeFixProvider.cs:108-109):

   currentRoot = currentRoot.RemoveNode(parameterSyntax, SyntaxRemoveOptions.KeepNoTrivia)!;

   The null-forgiving operator assumes RemoveNode never returns null, which is safe here but worth noting.

⚡ Performance: Good

• Efficient use of SyntaxRemoveOptions.KeepNoTrivia to prevent unnecessary trivia propagation
• Batch removal of multiple nodes where possible (RemoveNodes vs individual RemoveNode calls)
• LINQ usage is appropriate for non-hot-path analyzer code

🔒 Security: No Concerns

• No dynamic code generation or reflection usage
• Safe handling of syntax tree transformations
• Proper null checking throughout

🧪 Test Coverage: Adequate

• Test files show coverage for MSTest and XUnit migration scenarios
• Tests verify both positive and negative cases for attribute conversion
• New trivia preservation logic would benefit from explicit formatting tests

📝 Minor Suggestions

1. Consider adding XML documentation for the new cleanup methods in BaseMigrationCodeFixProvider.cs
2. The UpdateSyntaxTrees method could benefit from a comment explaining why multiple compilation updates are necessary

🎉 Excellent Work!

This PR demonstrates thoughtful engineering around a complex problem (preserving formatting during syntax transformations). The solution is robust, handles edge cases well, and maintains TUnit's high code quality standards.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3625

I'll review this pull request focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files for code quality
• Check for potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide final review summary

✅ Overall Assessment: APPROVED

This PR successfully addresses trivia preservation and cleanup issues in migration code fixers. The implementation is solid and follows TUnit's coding standards.

🎯 Key Improvements

1. Trivia Preservation Enhancement

   • AssertionRewriter.cs:22-25: Properly preserves original whitespace and comments during syntax transformations
   • BaseMigrationCodeFixProvider.cs:190-194: Hook attributes maintain their original trivia structure

2. Comprehensive Cleanup Logic

   • CleanupClassMemberLeadingTrivia(): Removes excessive blank lines after class opening braces
   • CleanupEndOfFileTrivia(): Eliminates trailing newlines at file end
   • Smart trivia handling in XUnit's TheoryData conversion preserves brace placement

🔍 Code Quality: Excellent

• ✅ Modern C# patterns (collection expressions, pattern matching)
• ✅ Follows TUnit's coding standards from CLAUDE.md
• ✅ Proper error handling with try-catch returning original document on failure
• ✅ Good separation of concerns with specialized rewriters

🐛 Potential Issues: Minor

1. Performance Consideration (XUnitMigrationCodeFixProvider.cs:56-72):

   // Multiple compilation updates could be expensive for large files
   UpdateSyntaxTrees(ref compilation, ref syntaxTree, ref updatedRoot);

   This iterative approach is necessary for complex transformations but could be slow for large codebases. However, it's the correct approach for maintaining semantic accuracy.

2. Edge Case (BaseMigrationCodeFixProvider.cs:108-109):

   currentRoot = currentRoot.RemoveNode(parameterSyntax, SyntaxRemoveOptions.KeepNoTrivia)!;

   The null-forgiving operator assumes RemoveNode never returns null, which is safe here but worth noting.

⚡ Performance: Good

• Efficient use of SyntaxRemoveOptions.KeepNoTrivia to prevent unnecessary trivia propagation
• Batch removal of multiple nodes where possible (RemoveNodes vs individual RemoveNode calls)
• LINQ usage is appropriate for non-hot-path analyzer code

🔒 Security: No Concerns

• No dynamic code generation or reflection usage
• Safe handling of syntax tree transformations
• Proper null checking throughout

🧪 Test Coverage: Adequate

• Test files show coverage for MSTest and XUnit migration scenarios
• Tests verify both positive and negative cases for attribute conversion
• New trivia preservation logic would benefit from explicit formatting tests

📝 Minor Suggestions

1. Consider adding XML documentation for the new cleanup methods in BaseMigrationCodeFixProvider.cs
2. The UpdateSyntaxTrees method could benefit from a comment explaining why multiple compilation updates are necessary

🎉 Excellent Work!

This PR demonstrates thoughtful engineering around a complex problem (preserving formatting during syntax transformations). The solution is robust, handles edge cases well, and maintains TUnit's high code quality standards.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3625

I'll review this pull request focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files for code quality
• Check for potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide final review summary

✅ Overall Assessment: APPROVED

This PR successfully addresses trivia preservation and cleanup issues in migration code fixers. The implementation is solid and follows TUnit's coding standards.

### Code Quality Analysis

The changes show excellent adherence to TUnit's coding standards and modern C# practices:

✅ Strengths:

• Modern C# syntax with collection expressions, pattern matching, and file-scoped namespaces
• Proper error handling with try-catch returning original document on failure at BaseMigrationCodeFixProvider.cs:90-94
• Good separation of concerns with specialized rewriters for different migration aspects
• Comprehensive trivia preservation logic maintaining original formatting

📍 Key Improvements:

1. Trivia Preservation (AssertionRewriter.cs:22-25):

return convertedAssertion
    .WithLeadingTrivia(node.GetLeadingTrivia())
    .WithTrailingTrivia(node.GetTrailingTrivia());

This properly preserves whitespace and comments during syntax transformations.

2. Cross-Platform Line Ending Normalization (BaseMigrationCodeFixProvider.cs:174-183):
   The new NormalizeLineEndings method detects and maintains consistent line endings, preventing cross-platform test failures.

3. Smart Cleanup Methods:

   • CleanupClassMemberLeadingTrivia() removes excessive blank lines after class opening braces
   • CleanupEndOfFileTrivia() eliminates trailing newlines at file end

### Potential Issues Analysis

🟡 Minor Considerations:

1. Performance Impact (XUnitMigrationCodeFixProvider.cs:51-63):

// Multiple compilation updates in sequence
UpdateSyntaxTrees(ref compilation, ref syntaxTree, ref updatedRoot);

The iterative approach with multiple compilation updates could be expensive for large files. However, this is necessary for maintaining semantic accuracy during complex transformations.

2. Null-Forgiving Operator (BaseMigrationCodeFixProvider.cs:101):

currentRoot = currentRoot.RemoveNode(parameterSyntax, SyntaxRemoveOptions.KeepNoTrivia)!;

The ! operator assumes RemoveNode never returns null, which is safe in this context but worth noting.

### Performance & Security

✅ Performance:

• Efficient use of SyntaxRemoveOptions.KeepNoTrivia to prevent unnecessary trivia propagation
• Batch removal of multiple nodes where possible (RemoveNodes vs individual RemoveNode calls)
• LINQ usage is appropriate for non-hot-path analyzer code
• Performance characteristics are acceptable for code fixer context

🔒 Security:

• No dynamic code generation or reflection usage
• Safe handling of syntax tree transformations
• Proper null checking throughout
• No security concerns identified

### Test Coverage

✅ Adequate Coverage:

• Test files show coverage for MSTest and XUnit migration scenarios
• Tests verify both positive and negative cases for attribute conversion
• New trivia preservation logic would benefit from explicit formatting tests, but existing coverage validates the core functionality

### Recommendations

1. Consider adding XML documentation for the new cleanup methods in BaseMigrationCodeFixProvider.cs
2. The UpdateSyntaxTrees method could benefit from a comment explaining why multiple compilation updates are necessary

### Final Assessment

🎉 Excellent Work! This PR demonstrates thoughtful engineering around a complex problem (preserving formatting during syntax transformations). The solution is robust, handles edge cases well, and maintains TUnit's high code quality standards. The implementation correctly addresses issue #3621 with comprehensive trivia handling and cleanup logic.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3625

I'll review this pull request focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files for code quality
• Check for potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide final review summary

✅ Overall Assessment: APPROVED

This PR successfully addresses trivia preservation and cleanup issues in migration code fixers. The implementation is solid and follows TUnit's coding standards.

🎯 Key Improvements

1. Trivia Preservation Enhancement

   • AssertionRewriter.cs:22-25: Properly preserves original whitespace and comments during syntax transformations
   • BaseMigrationCodeFixProvider.cs:190-194: Hook attributes maintain their original trivia structure

2. Comprehensive Cleanup Logic

   • CleanupClassMemberLeadingTrivia(): Removes excessive blank lines after class opening braces
   • CleanupEndOfFileTrivia(): Eliminates trailing newlines at file end
   • Smart trivia handling in XUnit's TheoryData conversion preserves brace placement

🔍 Code Quality: Excellent

• ✅ Modern C# patterns (collection expressions, pattern matching)
• ✅ Follows TUnit's coding standards from CLAUDE.md
• ✅ Proper error handling with try-catch returning original document on failure
• ✅ Good separation of concerns with specialized rewriters

🐛 Potential Issues: Minor

1. Performance Consideration (XUnitMigrationCodeFixProvider.cs:56-72):

   // Multiple compilation updates could be expensive for large files
   UpdateSyntaxTrees(ref compilation, ref syntaxTree, ref updatedRoot);

   This iterative approach is necessary for complex transformations but could be slow for large codebases. However, it's the correct approach for maintaining semantic accuracy.

2. Edge Case (BaseMigrationCodeFixProvider.cs:108-109):

   currentRoot = currentRoot.RemoveNode(parameterSyntax, SyntaxRemoveOptions.KeepNoTrivia)!;

   The null-forgiving operator assumes RemoveNode never returns null, which is safe here but worth noting.

⚡ Performance: Good

• Efficient use of SyntaxRemoveOptions.KeepNoTrivia to prevent unnecessary trivia propagation
• Batch removal of multiple nodes where possible (RemoveNodes vs individual RemoveNode calls)
• LINQ usage is appropriate for non-hot-path analyzer code

🔒 Security: No Concerns

• No dynamic code generation or reflection usage
• Safe handling of syntax tree transformations
• Proper null checking throughout

🧪 Test Coverage: Adequate

• Test files show coverage for MSTest and XUnit migration scenarios
• Tests verify both positive and negative cases for attribute conversion
• New trivia preservation logic would benefit from explicit formatting tests

📝 Minor Suggestions

1. Consider adding XML documentation for the new cleanup methods in BaseMigrationCodeFixProvider.cs
2. The UpdateSyntaxTrees method could benefit from a comment explaining why multiple compilation updates are necessary

🎉 Excellent Work!

This PR demonstrates thoughtful engineering around a complex problem (preserving formatting during syntax transformations). The solution is robust, handles edge cases well, and maintains TUnit's high code quality standards.

---