DiagnosticResult builder simplification

[sharwell]

This pull request contains an alternative to the approach in #518. I'd specifically like to compare the solutions in their ability to meet the varying needs of the following diagnostics before moving forward with one or the other:

1. SA1600
2. SA1603 (which has arguments and also has multiple locations)
3. NumberSignSpacingTestBase (used by SA1021 and SA1022 tests), which has arguments
4. SA1119 (which has multiple diagnostic IDs being reported by one analyzer)

This pull request includes implementations of each of the above for review (they are provided as separate commits to make things a bit easier).

[vweijsters]

This will trigger SA1503. There are several more instances below that do the same.

[vweijsters]

I quite like this syntax, but I'm not too happy with having to create an array each time.
Would it be an option to go for a more Xunit like syntax?
Something like:

Expect.Diagnostic().WithLocation(1,1);
Expect.Diagnostic(MyDiagnosticId).WithLocation(3,14);

The VerifyCSharpDiagnosticAsync method must then be modified to no longer include the expected diagnostics as parameter.

[pdelvo]

I agree with @vweijsters.

[sharwell]

@vweijsters @pdelvo What about an overload of VerifyCSharpDiagnosticAsync which uses DiagnosticResult (instead of DiagnosticResult[]) as the second parameter? The current overload seems to do fine with cases where multiple diagnostics are reported and with cases where no diagnostics are reported (due to the shared EmptyDiagnosticResults), but it overly verbose for cases where exactly one diagnostic is reported.

[vweijsters]

That would ok with me.
It will probably be quite difficult to implement the Expect mechanism nicely.

To make things even easier, I think that VerifyCSharpDiagnosticAsync should have a default value of CancellationToken.None for the cancellationToken parameter as well.

[sharwell]

@vweijsters @pdelvo How would you like to proceed regarding this and #518?

To make things even easier, I think that VerifyCSharpDiagnosticAsync should have a default value of CancellationToken.None for the cancellationToken parameter as well.

I'm pretty heavily opposed to this, but as with anything else I'm will to discuss it separately.

[pdelvo]

If xunit supports CancellationTokens we could use them. But we probably want a code fix/analyzer that would add that because it would take quite some time changing over a thausand unit tests.

[sharwell]

@pdelvo Handling of CancellationToken will be a separate issue. I'm specifically referring to the construction and use of DiagnosticResult.

[pdelvo]

Im fine with using your solution with an overload added to VerifyCSharpDiagnosticAsync which accepts DiagnosticResult.

[vweijsters]

I prefer the syntax in this pull request, so I would be in favor for using this instead of #518.
Assuming of course that you will add the overload 😉

[sharwell]

@pdelvo @vweijsters Thank you for your prompt feedback. I have updated the pull request to include the new overload, followed by a commit to use that overload instead of the array where applicable (only changed the instances which were already converted to the new builder form)

[sharwell]

@pdelvo @vweijsters I'm planning to merge this as soon as the automated build passes. I'll update the remaining tests (spacing + SA1400) as a separate pull request.