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

NET-913 Modify rule S1264: Improve description to match the implementation #4587

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

sebastien-marichal
Copy link
Contributor

@sebastien-marichal sebastien-marichal commented Dec 23, 2024

NET-913

Part of NET-255

Fixes SonarSource/sonar-dotnet#8249

Review

A dedicated reviewer checked the rule description successfully for:

  • logical errors and incorrect information
  • information gaps and missing content
  • text style and tone
  • PR summary and labels follow the guidelines

Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

----
int i;

for (i = 0; i < 10;) // Noncompliant; the initializer section should contain a variable declaration
Copy link
Contributor

@CristianAmbrosini CristianAmbrosini Dec 23, 2024

Choose a reason for hiding this comment

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

Is the message correct? the initializer section should contain a variable declaration
Shouldn't it be the initializer section should contain an increment statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initializer section here contains only an initialization without a variable declaration.
The increment section is missing in this example.
The wording could be better

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the rule reporting also for missing variable declarations?
ie:

var i;
for (i = 0; i <100; i++) { }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not it won't, for now it is checking if both the initializer and the increment section are valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then // Noncompliant; the initializer section should contain a variable declaration is not a good comment. It shouldn't raise for that and it's not why the rule is raising in that case, it's just the missing incrementer. Do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does raise because of the missing declaration.
If there were a variable declaration, the rule won't raise even though there is no incrementer.

The condition for the rule to raise is:

  • missing initialization section
  • and, missing increment section

In the rule we check if there is a variable declaration.
So if the content of the initialization section is not a declaration and the increment section is empty, we raise the rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will drop this example, it brings more confusion than it helps to understand the rule.
I have added in the description the following:

The initializer section should contain a variable declaration to be considered as a valid initialization.

And I think this should be enough for this case then.

Copy link
Contributor

@CristianAmbrosini CristianAmbrosini left a comment

Choose a reason for hiding this comment

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

LGTM!

@CristianAmbrosini CristianAmbrosini merged commit 3301562 into master Dec 24, 2024
19 checks passed
@CristianAmbrosini CristianAmbrosini deleted the sma/modify-S1264 branch December 24, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve S1264: Update RSPEC to include using initialization for simple assignation
2 participants