FSharpDiagnostic: add default severity #19152
Conversation
❗ Release notes required
|
Martin521
left a comment
Martin521
left a comment
There was a problem hiding this comment.
That makes a lot of sense.
The whole diagnostics logging area is so over-complicated. I welcome every step to simplify it.
| | FSharpDiagnosticSeverity.Hidden -> [] | ||
| | adjustedSeverity -> | ||
|
|
||
| let diagnostic = { diagnostic with Severity = adjustedSeverity } |
There was a problem hiding this comment.
Without reading the implementation, I had problems imagining what default meant in the context of F.C.S public API.
Did you consider other names?
OriginalSeverity or UnadjustedSeverity perhaps?
There was a problem hiding this comment.
I think the current naming makes sense. Maybe an XmlDoc would be sufficient? We could then leave the current name. If not, I'd vote for OriginalSeverity.
There was a problem hiding this comment.
PhasedDiagnostic is not part of the public API, so I would keep it here as is.
See my comment below for the public API.
| | FSharpDiagnosticSeverity.Hidden -> [] | ||
| | adjustedSeverity -> | ||
|
|
||
| let diagnostic = { diagnostic with Severity = adjustedSeverity } |
There was a problem hiding this comment.
PhasedDiagnostic is not part of the public API, so I would keep it here as is.
See my comment below for the public API.
| member Severity: FSharpDiagnosticSeverity | ||
|
|
||
| /// Gets the default severity, does not take into account diagnostics options | ||
| member DefaultSeverity: FSharpDiagnosticSeverity |
There was a problem hiding this comment.
I don't think the original severity should be publicly accessible. It is an implementation detail. The adjusted severity is the specified output of the compiler (after applying the different flags and directives) and should be the only visible one.
4 participants
This API allows the IDE to differentiate compiler errors from warnings. Useful, for example, for suggesting actions like adding a #nowarn directive.