fix(dotnet): emit --exclude-diagnostics per diagnostic ID

[thomhurst]

Summary

• Typed DotNetFormatOptions.ExcludeDiagnostics as string[]? so each ID renders as a separate --exclude-diagnostics <id> flag. The old string? value was quoted as one CLI arg, so dotnet format treated the whole value as a single (invalid) diagnostic ID.
• Added a stringlist override in TypeOverrides/dotnet.json so regeneration preserves the fix.
• Added a regression test verifying the array value produces repeated flags.

Fixes #2472

Breaking: callers now pass string[] (e.g. ["CS0246", "CS1503"]) instead of a space-separated string.

Test plan

• dotnet build clean
• New DotNetFormatOptionsTests.ExcludeDiagnostics_Passes_Each_Id_Separately passes
• Existing CliAttributeTests (26 tests) still pass

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	✅ 0 (≤ 20 complexity)

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

Summary: Fixes a bug where ExcludeDiagnostics was typed as string? causing the entire space-separated string to be passed as one CLI argument. The fix correctly changes it to string[]? so the argument builder emits a separate --exclude-diagnostics <id> pair per diagnostic ID. The generator override ensures regeneration preserves the fix.

---

Correctness

The fix is correct. CommandArgumentBuilder already handles IEnumerable (non-char) types by iterating each element and emitting the option name separately per item — changing the type to string[]? is exactly the right lever to pull.

---

Issue: --diagnostics May Have the Same Problem

In Generated/Options/DotNetFormatOptions.cs (line 27), the --diagnostics property is still typed as string?:

public string? Diagnostics { get; set; }

Per the actual dotnet format --help, --diagnostics is also a repeatable list of IDs (same pattern as --exclude-diagnostics). By the same reasoning that motivated this PR, it may need the same string[]? fix and a corresponding generator override. Worth checking and addressing in a follow-up (or this PR if you agree).

---

Issue: Only One of Two DotNetFormatOptions Classes Is Covered by the Test

There are two separate DotNetFormatOptions records in the repo:

1. src/ModularPipelines.DotNet/Generated/Options/DotNetFormatOptions.cs — namespace ModularPipelines.DotNet.Generated.Options, uses [CliCommand("dotnet", "format")]
2. src/ModularPipelines.DotNet/Options/DotNetFormatOptions.Generated.cs — namespace ModularPipelines.DotNet.Options, uses [CliSubCommand("format")]

The test imports ModularPipelines.DotNet.Options (class #2) but does not cover the Generated.Options variant (class #1). Both were modified in this PR. Adding a parallel test case for class #1 would close that gap.

---

Minor: IsEquivalentTo Is Order-Insensitive for a CLI Arg List

await Assert.That(args).IsEquivalentTo(new[]
{
    "--exclude-diagnostics", "CS0246",
    "--exclude-diagnostics", "CS1503",
});

IsEquivalentTo checks set equivalence (order-insensitive). For CLI arguments the position order matters — dotnet format likely accepts either order, but using an ordered sequence assertion (IsEqualTo / ContainsInOrder) would give higher confidence and be more precise. I see this pattern is used consistently in the existing CliAttributeTests.cs, so this is a repo-wide convention rather than specific to this PR — but worth noting.

---

Minor: Missing Edge Case Tests

There is no test for ExcludeDiagnostics = [] (empty array) or ExcludeDiagnostics = null. Both should produce zero CLI arguments. The implementation handles these correctly via the foreach loop in AddOption, but explicit tests would round out coverage.

---

What's Done Well

• The generator override in dotnet.json is clean, follows existing patterns, and includes a helpful "reason" field — this is the right architectural approach to prevent the fix from being overwritten on next generation.
• The doc comment is correctly updated from "A space separated list" to "A list", which accurately reflects the new API contract.
• The test directly exercises the previously-broken behavior, which is good regression coverage.
• The breaking change is correctly noted in the PR summary.

---

Overall: Good, minimal, correct fix. The main follow-up items are checking --diagnostics for the same problem and adding test coverage for the Generated.Options variant of the class.

[claude]

Follow-up Review

This review compares the current state of the PR against my previous review posted earlier today. No new commits have been added since then, so none of the previously raised issues have been addressed yet. Summarizing them here for clarity.

---

Still Open: --diagnostics Has the Same Bug

In src/ModularPipelines.DotNet/Generated/Options/DotNetFormatOptions.cs (line 31):

[CliOption("--diagnostics")]
public string? Diagnostics { get; set; }

dotnet format --diagnostics accepts a repeatable list of diagnostic IDs — exactly the same pattern as --exclude-diagnostics. The same root cause applies: passing a space-separated string will hand a single multi-word token to the CLI instead of repeated flags. This should be string[]? with a corresponding stringlist entry in dotnet.json, mirroring the fix applied to ExcludeDiagnostics.

---

Still Open: Test Only Covers One of the Two DotNetFormatOptions Records

The test imports ModularPipelines.DotNet.Options (the hand-edited DotNetFormatOptions.Generated.cs), but not ModularPipelines.DotNet.Generated.Options (the fully-generated DotNetFormatOptions.cs). Both were modified in this PR. A parallel test case instantiating ModularPipelines.DotNet.Generated.Options.DotNetFormatOptions would confirm the generated class also produces separate flags.

---

Minor (repo-wide pattern, not a blocker): IsEquivalentTo Is Order-Insensitive

await Assert.That(args).IsEquivalentTo(new[]
{
    "--exclude-diagnostics", "CS0246",
    "--exclude-diagnostics", "CS1503",
});

IsEquivalentTo is set-equivalence — it won't catch a reversal of argument pairs. Since this is consistent with the existing CliAttributeTests.cs convention in the repo, I'm not asking for a change, just flagging that the assertion gives slightly weaker ordering guarantees than IsEqualTo/ContainsInOrder would.

---

What's Done Well (unchanged)

The core fix — string[]?, updated doc comment, and stringlist override in dotnet.json — remains clean and correct. The generator override is the right architectural lever to prevent regeneration from reverting the fix.

---

Recommendation: The two open items above are worth addressing before merge: (1) apply the same string[]? fix to --diagnostics, and (2) add a test for the Generated.Options variant. The core fix is solid.