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.

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

New rule S2589/S2583 for VB.NET #7727

Merged
merged 20 commits into from
Aug 11, 2023
Merged

Conversation

mary-georgiou-sonarsource
Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource commented Aug 4, 2023

@mary-georgiou-sonarsource
Copy link
Contributor Author

mary-georgiou-sonarsource commented Aug 10, 2023

@Tim-Pohlmann Take a first look - implementation + UTs is done.
What is not done is the rspecs fro these two rules.
I'll update you later with links the PRs.

Ignore Reworked secondary locations commit.

EDIT: This commit was reverted.

Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

Some polishing required

Comment on lines +205 to +209
If sObj?.str?.Length > 2 Then
' ^^^^ Noncompliant
' ^^^^^^^ Secondary@-1
' ^^^^^^^^^^^^^^^^^^^^^ Noncompliant@-2
Console.WriteLine("a") ' Secondary
Copy link
Contributor

Choose a reason for hiding this comment

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

The length of the Secondary location is off. But this will be fixed by #7725

Comment on lines +230 to +233
Dim sObj = Nothing
Dim x = sObj?.str?.Length > 2
' ^^^^ Noncompliant
' ^^^^^^^ Secondary@-1
Copy link
Contributor

Choose a reason for hiding this comment

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

The length of the Secondary location is off. But this will be fixed by #7725

Comment on lines 582 to 583
a = a & true;
if (a) // FN
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only an FN because of the compound assignments above. Needs to be separated.

Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

LGTM, only minor nitpicks left.

@sonarqubecloud
Copy link

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

@sonarqubecloud
Copy link

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

95.8% 95.8% Coverage
0.0% 0.0% Duplication

@mary-georgiou-sonarsource mary-georgiou-sonarsource merged commit 3a8622a into master Aug 11, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource deleted the mary/vb-2589-2583 branch August 11, 2023 07:45
sebastien-marichal pushed a commit that referenced this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants