Respect code style option severity in ConvertSwitchStatementToExpress…#36087
Respect code style option severity in ConvertSwitchStatementToExpress…#36087mavasani merged 1 commit intodotnet:masterfrom
Conversation
…ionDiagnosticAnalyzer Fixes dotnet#36086
|
Tag @alrz @CyrusNajmabadi |
| } | ||
|
|
||
| context.ReportDiagnostic(Diagnostic.Create(Descriptor, | ||
| context.ReportDiagnostic(DiagnosticHelper.Create(Descriptor, |
There was a problem hiding this comment.
if this is an optional arg, we should consider making mandatory, so that this can't happen.
There was a problem hiding this comment.
This is already a mandatory argument for DiagnosticHelper.Create API. The issue was the analyzer directly invoked the Diagnostic.Create public API, which is meant for all non-code style analyzers.
There was a problem hiding this comment.
ah, got it. yeah... hrmm, i wonder if we should have an analyzer to catch usages of Diagnostic.Create!
| } | ||
| }"; | ||
| var warningOption = new CodeStyleOption<bool>(true, NotificationOption.Warning); | ||
| var options = Option(CSharpCodeStyleOptions.PreferSwitchExpression, warningOption); |
There was a problem hiding this comment.
can just say CodeStyleOptions.TrueWithSuggestionEnforcement then just validate you got a suggestion back.
| using var workspace = CreateWorkspaceFromOptions(source, testParameters); | ||
| var diags = await GetDiagnosticsAsync(workspace, testParameters); | ||
| Assert.Single(diags); | ||
| Assert.Equal(DiagnosticSeverity.Warning, diags.First().Severity); |
There was a problem hiding this comment.
could just remove the line above and have this be diags.Single().Severity. But i don't care that much :)
There was a problem hiding this comment.
also, can we check if the diag ID is actually the one we expect here?
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Tiny nits/suggestions. Up to you on taking them. I would recommend future work to make this parameter non-optional.
Approve
Submit feedback approving these changes.
Thanks. I am going to do these as a follow-up change as we have a targeted snap for today and I want to ensure this change makes it in. |
…ionDiagnosticAnalyzer
Fixes #36086