feat: add DisableLogoCommandProvider

[thomhurst]

Fixes #2735

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: DisableLogoCommandProvider

Reviewing this PR that adds DisableLogoCommandProvider functionality. Here's my progress:

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze the modified files for code quality
• Check issue Missing --disable-logo command line switch in v0.50.0 #2735 for context
• Review for potential bugs and issues
• Assess performance considerations
• Check for security concerns
• Evaluate test coverage needs
• Provide comprehensive feedback

---

Code Review Summary

This PR successfully re-adds the --disable-logo command line flag functionality that appears to have been missing. The implementation is clean and follows the existing patterns in the codebase.

✅ Strengths

1. Good Architecture: The DisableLogoCommandProvider follows the established pattern of other command providers in the codebase
2. Proper Dependency Injection: The BannerCapability constructor correctly accepts the new ILoggerFactory parameter
3. Clear Documentation: The command line flag is already documented in /docs/docs/reference/command-line-flags.md
4. Simple Logic: The banner suppression logic is straightforward and easy to understand
5. Consistent Naming: Uses the same DisableLogo constant across both files

🔍 Code Quality Observations

BannerCapability.cs (lines 25-29):

• The logic combines two conditions for hiding the logo: DisableLogo flag OR logger information level
• The second condition loggerFactory.CreateLogger(nameof(BannerCapability)).IsEnabled(LogLevel.Information) seems unusual - typically you'd want to show MORE when logging is enabled, not hide the logo

TestApplicationBuilderExtensions.cs:

• Clean registration of the new command provider on line 34
• Proper parameter addition to BannerCapability constructor on line 48

⚠️ Potential Issues

1. Logger Condition Logic: The condition || loggerFactory.CreateLogger(nameof(BannerCapability)).IsEnabled(LogLevel.Information) in BannerCapability.cs:26 seems counterintuitive. When Information-level logging is enabled, this will suppress the logo and only show runtime details. Is this the intended behavior? Usually you'd want more output when verbose logging is enabled, not less.

2. Missing Tests: There are no unit tests for the new DisableLogoCommandProvider or the updated banner logic. Consider adding tests for:

   • Command provider registration and options
   • Banner capability behavior with/without the disable flag
   • Banner capability behavior with different logger levels

🚀 Performance Considerations

• Minimal performance impact - only adds a simple boolean check
• Logger creation in the banner method is lightweight

🔒 Security Considerations

• No security concerns identified
• The command line option has zero arity (no arguments), so no injection risks

📋 Test Coverage Recommendations

Consider adding tests in the following areas:

// Test the command provider
[Test]
public void DisableLogoCommandProvider_Should_Register_Correct_Option()
{
    // Verify command line option registration
}

// Test banner behavior
[Test]
public void BannerCapability_Should_Hide_Logo_When_DisableFlag_Set()
{
    // Test banner output with --disable-logo flag
}

[Test]
public void BannerCapability_Should_Show_Logo_By_Default()
{
    // Test default banner behavior
}

🤔 Questions for Clarification

1. Logger Condition: Can you clarify the intended behavior of the logger condition? Should Information-level logging suppress or enhance the banner display?

2. Integration Testing: Have you manually tested that --disable-logo works as expected when running TUnit?

📝 Minor Suggestions

1. Consider adding XML documentation comments to the DisableLogoCommandProvider class
2. The comment on line 36 of TestApplicationBuilderExtensions.cs mentions "(replaces HideTestOutput, DisableLogo, DetailedStacktrace)" but DisableLogo is now a separate provider - this comment might need updating

🎯 Overall Assessment

This is a solid, straightforward implementation that restores important functionality. The code follows established patterns and is well-structured. The main concern is the logger condition logic which may need clarification. Once that's addressed and some tests are added, this would be ready for merge.

Recommendation: ✅ Approve with minor suggestions

---