-
Notifications
You must be signed in to change notification settings - Fork 745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Use single Diagnostic
class to represent all core diagnostics
#14875
Conversation
Test this change out locally with the following install scripts (Action run 10532680475) VSCode
Azure CLI
|
ba33ad5
to
2dab2ec
Compare
2dab2ec
to
49bbaa7
Compare
Diagnostic
class to represent all core diagnostics
|
||
namespace Bicep.Core.Diagnostics; | ||
|
||
public class ResultWithDiagnosticBuilder<TSuccess> : Result<TSuccess, DiagnosticBuilder.DiagnosticBuilderDelegate> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the new name, this was confusing.
@@ -228,8 +228,8 @@ private DeclaredTypeAssignment GetParameterType(ParameterDeclarationSyntax synta | |||
if (!binder.FileSymbol.TryGetBicepFileSemanticModelViaUsing().IsSuccess(out var semanticModel, out var failureDiagnostic)) | |||
{ | |||
// failed to resolve using | |||
return failureDiagnostic is ErrorDiagnostic error | |||
? ErrorType.Create(error) | |||
return failureDiagnostic.Level == DiagnosticLevel.Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth it to have an IsErrorDiagnostic() or IsErrorLevel() extension for IDiagnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I have a follow-up PR - will address this there.
The split class structure between
ErrorDiagnostic
&Diagnostic
is something I've been meaning to remove since the very early days of Bicep. I've decided to do it now, because it'll simplify the refactor needed for #14842.The majority of the changes in this PR were made using refactoring tools, not manually. The main changes I made were:
ErrorDiagnostic
,ErrorBuilderDelegate
,FixableErrorDiagnostic
diag is ErrorDiagnostic
type checks to usediag.Level == DiagnosticLevel.Error
insteadResultWithDiagnostic
toResultWithDiagnosticBuilder
instead to more accurately represent its purposeResultWithDiagnotic<T>
, and replaced usage ofResult<T, Diagnostic>
with itMicrosoft Reviewers: Open in CodeFlow