-
Notifications
You must be signed in to change notification settings - Fork 223
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
Prevent issues from being reported when in Razor generated file or content after mapped back #7940
Prevent issues from being reported when in Razor generated file or content after mapped back #7940
Conversation
435ed90
to
f704783
Compare
analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarSyntaxNodeReportingContext.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarReportingContextBase.cs
Outdated
Show resolved
Hide resolved
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.
Few small suggestions.
Also, in theory once these changes are done we should be able to remove
|
06343be
to
3208b4c
Compare
f704783
to
b3947bd
Compare
3208b4c
to
be172c4
Compare
b3947bd
to
e52f1c9
Compare
e52f1c9
to
d8fd500
Compare
b24d40b
to
5ee622e
Compare
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.
Looks good, what I am afraid of is that we don't have a single intentional test that changes this change. Even the ITs that would fail are not the Razor or Blazor support samples that we added for razor blazor testing. The NetCore app It can be freely removed and from that point on we would have no tests.
Could you please add a UT on .net side and introduce some issues for the razor blazor app that would raise without your changes?
5ee622e
to
77a761b
Compare
{ | ||
using var scope = new EnvironmentVariableScope(false) { EnableRazorAnalysis = true }; | ||
new VerifierBuilder() | ||
.AddAnalyzer(() => new DummyAnalyzerRazorGeneratedCode()) |
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.
There is no need to create a new dummy analyzer. you can also you DummyCS to assert the functionality.
.AddAnalyzer(() => new DummyAnalyzerRazorGeneratedCode()) | |
.AddAnalyzer(() => new DummyCS()) |
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 am approving, but please address the comment I left before merging.
- Prevent report if location still in generated content after location mapping - Do not analyse if SyntaxNode is in location that cannot be mapped to the original file
77a761b
to
ca0c80e
Compare
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Fixes #7930
Should be merged after #7932