Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement S3898 -ValueTypes should implement IEquatable - for VB.NET #5740

Merged
merged 21 commits into from
Jan 26, 2023
Merged

Implement S3898 -ValueTypes should implement IEquatable - for VB.NET #5740

merged 21 commits into from
Jan 26, 2023

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Jun 14, 2022 •
edited by pavel-mikula-sonarsource
Loading

@pavel-mikula-sonarsource
Copy link
Contributor

@Corniel Can you please rebase all your PRs on the latest master? We've merged quite a big refactoring of context classes and the way rules register themselves that affects all the rules, see #6532

Thank you

@Corniel
Copy link
Contributor Author

Corniel commented Jan 23, 2023

@Corniel Can you please rebase all your PRs on the latest master? We've merged quite a big refactoring of context classes and the way rules register themselves that affects all the rules, see #6532

Thank you

I noticed. :) I'll do that.

@Corniel Corniel force-pushed the S3898-ValueTypeShouldImplementIEquatable branch 2 times, most recently from d1b09ac to 0504c0c Compare January 23, 2023 10:29
pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes Jan 23, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nitpicks

analyzers/src/SonarAnalyzer.Common/Rules/PropertyGetterWithThrowBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/ValueTypeShouldImplementIEquatableBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/ValueTypeShouldImplementIEquatableBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ValueTypeShouldImplementIEquatable.vb Outdated Show resolved Hide resolved
Corniel Nobel and others added 5 commits January 24, 2023 09:33
Implement for VB.NET.
f6bf92c 
Rebase aftermath.
Drop not relevant namespaces.
06d7ecc
171 VB rules. :)
c34d314
@pavel-mikula-sonarsource
Suggestions
bdb49e2 
Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Update analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Update analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Update analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Update analyzers/src/SonarAnalyzer.Common/Rules/ValueTypeShouldImplementIEquatableBase.cs

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Update analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ValueTypeShouldImplementIEquatable.vb

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Fix
99a9a90
@Corniel Corniel force-pushed the S3898-ValueTypeShouldImplementIEquatable branch from a683b54 to 99a9a90 Compare January 24, 2023 08:33
pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, let's add one more test case

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ValueTypeShouldImplementIEquatable.cs Show resolved Hide resolved
@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Corniel Nobel and others added 5 commits January 25, 2023 16:32
Add test cases.
0128aea
@pavel-mikula-sonarsource
Update analyzers/src/SonarAnalyzer.VisualBasic/Rules/ValueTypeShouldI…
521f8db 
…mplementIEquatable.cs

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
@pavel-mikula-sonarsource
Update analyzers/src/SonarAnalyzer.CSharp/Rules/ValueTypeShouldImplem…
5643ca9 
…entIEquatable.cs

Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
Extend coverage.
e5b2ccd
Merge branch 'S3898-ValueTypeShouldImplementIEquatable' of https://gi…
e8ef2e2 
…thub.com/Corniel/sonar-csharp into S3898-ValueTypeShouldImplementIEquatable
pavel-mikula-sonarsource
pavel-mikula-sonarsource approved these changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

analyzers/tests/SonarAnalyzer.UnitTest/Facade/SyntaxFacadeTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Facade/SyntaxFacadeTest.cs Outdated Show resolved Hide resolved
@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavel-mikula-sonarsource pavel-mikula-sonarsource removed their assignment Jan 26, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jan 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Jan 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 7cce08b into SonarSource:master Jan 26, 2023
@pavel-mikula-sonarsource
Copy link
Contributor

Thank you @Corniel for handling this

@Corniel Corniel deleted the S3898-ValueTypeShouldImplementIEquatable branch January 26, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavel-mikula-sonarsource pavel-mikula-sonarsource pavel-mikula-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement S3898 -ValueTypes should implement IEquatable - for VB.NET
2 participants
@Corniel @pavel-mikula-sonarsource