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
Disjoint Sets primitives #9145
Disjoint Sets primitives #9145
Conversation
b74449f
to
fbfd1da
Compare
@mary-georgiou-sonarsource @gregory-paidis-sonarsource Regarding this issue on SonarCloud: the result of Wrapping the inner list into a non-generic structure to fix the issue, in my opinion, will make the code that manipulates the structure more complex and less clear. So I would declare the issue as Wont Fix. |
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.
Very nice testcases!
Left a few comments, mostly optional and questions.
analyzers/src/SonarAnalyzer.Common/Helpers/DisjointSetsPrimitives.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Helpers/DisjointSetsPrimitivesTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Helpers/DisjointSetsPrimitivesTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Helpers/DisjointSetsPrimitivesTest.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.
It's super clear what this code is doing and it's very elegant.
However, I feel like when I see it it's not part of this codebase somehow.
If I read it without any context, I would have a hard time getting a quick idea of where it can be used and what its purpose is in the context of sonar-dotnet.
Maybe it's better to create a base class analyzer called "FindClassCodepedencies" or something similar, and then we can extend this class for every rule that needs this functionality.
WDYT? this is a question also for @gregory-paidis-sonarsource
analyzers/src/SonarAnalyzer.Common/Helpers/DisjointSetsPrimitives.cs
Outdated
Show resolved
Hide resolved
namespace SonarAnalyzer.Helpers; | ||
|
||
public static class DisjointSetsPrimitives | ||
{ |
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 add a comment here explaining a bit about the purpose of the class and these methods in the context of sonar-dotnet.
analyzers/tests/SonarAnalyzer.Test/Helpers/DisjointSetsPrimitivesTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Helpers/DisjointSetsPrimitivesTest.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.
Left only one comment, about how to utilize this class.
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
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
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
Part of #9089