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
Remove language property from SonarLintXmlReader #6905
Remove language property from SonarLintXmlReader #6905
Conversation
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContextBase.cs
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContextBase.cs
Show resolved
Hide resolved
28f2a29
to
6f3a31d
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.
LGTM! I left a nitpick
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarAnalysisContextBaseTest.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.
Kaboom
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/SonarLintXmlReader.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.
Last round
...sts/SonarAnalyzer.UnitTest/AnalysisContext/SonarAnalysisContextBaseTest.ShouldAnalyzeTree.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.
LGTM
[TestMethod] | ||
public void Message_CS_String_Ctor() => | ||
new UnexpectedLanguageException("C#").Message.Should().Be("Unexpected language: C#"); | ||
|
||
[TestMethod] | ||
public void Message_VB_String_Ctor() => | ||
new UnexpectedLanguageException("Visual Basic").Message.Should().Be("Unexpected language: Visual Basic"); |
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.
Ideas:
- We don't need this twice.
- If we would, it would be better to use
LanguagesNames.Xxx
. - _CS and _VB is typically used as a final suffix in the name
Solution: Just use some random value once instead
[TestMethod] | |
public void Message_CS_String_Ctor() => | |
new UnexpectedLanguageException("C#").Message.Should().Be("Unexpected language: C#"); | |
[TestMethod] | |
public void Message_VB_String_Ctor() => | |
new UnexpectedLanguageException("Visual Basic").Message.Should().Be("Unexpected language: Visual Basic"); | |
[TestMethod] | |
public void Message_String_Ctor() => | |
new UnexpectedLanguageException("Xxx").Message.Should().Be("Unexpected language: Xxx"); |
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.
Still LGTM, just clicking the correct button
There's one last nitpick hidden in the unresolved conversations above |
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
f0553a7
into
feature/SL-exclusions-inclusions
Addresses comments from #6870