-
Notifications
You must be signed in to change notification settings - Fork 77
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
SLVS-1510 Add IEmbeddedRoslynAnalyzerProvider & IConnectedModeRoslynAnalyzerProvider #5749
SLVS-1510 Add IEmbeddedRoslynAnalyzerProvider & IConnectedModeRoslynAnalyzerProvider #5749
Conversation
|
||
namespace SonarLint.VisualStudio.Infrastructure.VS.Roslyn; | ||
|
||
/// <summary> |
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.
This comment belongs better to the Get method
/// </summary> | ||
public interface IConnectedModeRoslynAnalyzerProvider | ||
{ | ||
ImmutableArray<AnalyzerFileReference>? GetOrNull(ServerConnection connection); |
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 think there is no need to call the method GetOrNull or it is obvious from the return type that it can be null
Get should be enough or GetAnalyzers
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 just named it like that to be explicit about what the contract of this method
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.
Isn't the return type explicit enough?
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.
Is it a big deal to have it in the name? I feel like it doesn't hurt at all
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.
Feels superfluous and I feel like naming is important. But I guess you can leave it.
namespace SonarLint.VisualStudio.Infrastructure.VS.Roslyn; | ||
|
||
/// <summary> | ||
/// Returns SonarAnalyzer.CSharp & SonarAnalyzer.VisualBasic analyzer DLLs that are embedded in the VSIX |
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.
Move the documentation to the get method
/// <summary> | ||
/// Returns SonarAnalyzer.CSharp & SonarAnalyzer.VisualBasic analyzer DLLs that are downloaded from the server for the current binding | ||
/// </summary> | ||
public interface IConnectedModeRoslynAnalyzerProvider |
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.
Consider if Repository might be a better name
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 would like to keep the provider and the writer interfaces split. The implementation could have Repository in the name though
Quality Gate passedIssues Measures |
e4627c9
into
feature/dotnet-analyzer-repackaging
SLVS-1510