-
Notifications
You must be signed in to change notification settings - Fork 231
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
S1939: Add support for record struct #5606
Conversation
Code coverage is to low because of the ILookup implementation. As this probably gets deleted anyway, I will only add tests if we keep it. |
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'd remove the ILookup
, I don't see enough value added by it.
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Common/MultiValueDictionary.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Common/MultiValueDictionary.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
d3075bc
to
ac7e015
Compare
analyzers/src/SonarAnalyzer.Common/Common/MultiValueDictionary.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Rules/RedundantInheritanceListTest.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantInheritanceList.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.
Nothing new, just those two last unresolved convesations.
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
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
This PR includes a custom
ILookup
implementation . It replaces the MultiValueDictionary used here. There is nothing wrong with MultiValueDictionary and it is used in multiple other places too.ILookup
is just better suited for the intent here. I implemented my ownILookup
because theToLookup
extension method would have introduced a big overhead.I don't think it is worth to keep my custom
Lookup
implementation, as it basically does the same as the existing MultiValueDictionary. I left it for the moment in case you see value in keeping it.