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

Delete S2255 (Deprecated) #7853

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Conversation

mary-georgiou-sonarsource
Copy link
Contributor

Rspec update: SonarSource/rspec#2930

Deleting S2255 as it has been deprecated.
Deprecated since:

  • sonar-dotnet 8.9.0.19135, released on Jun 26, 2020
  • SQ 8.4.0.35506 on Jul 3, 2020.

@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Aug 23, 2023
Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-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! Added a CaYC comment.

@@ -28,7 +28,7 @@ public class RuleTypeTest
{
// Rules that have been deprecated and deleted.
// When changing this please do not forget to notify the product teams (SQ, SC, SL).
private static readonly HashSet<string> DeletedRules = new() { "S1145", "S1697", "S4142", "S2758", "S2070", "S3693", "S4432", "S2278" };
private static readonly HashSet<string> DeletedRules = new() { "S1145", "S1697", "S4142", "S2758", "S2070", "S3693", "S4432", "S2278", "S2255" };
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reorder the collection in ascending order.

@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Aug 23, 2023
@zsolt-kolbay-sonarsource
Copy link
Contributor

One more: delete it from the IT ruleset files (I've found 4 references for it):

<Rule Id="S2255" Action="Warning" />

@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 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 Aug 23, 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
0.0% 0.0% Duplication

@@ -28,7 +28,7 @@ public class RuleTypeTest
{
// Rules that have been deprecated and deleted.
// When changing this please do not forget to notify the product teams (SQ, SC, SL).
private static readonly HashSet<string> DeletedRules = new() { "S1145", "S1697", "S4142", "S2758", "S2070", "S3693", "S4432", "S2278" };
private static readonly HashSet<string> DeletedRules = new() { "S1145", "S1697", "S2070", "S2255", "S2278", "S2758", "S3693", "S4142", "S4432"};
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this field? If i delete it, nothing breaks.
So it doesn't make sense to add anything here => just delete the field, and keep the test. In case we'd delete rule, it would bite us.

Also do not forget to plan to notify SQ, SC and SL teams, as mentioned in the comment below.

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably lost its meaning during some of the RSPEC reworks in the past

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some logic there that uses this hashset.
I prefer to merge this PR as is and add a task to my calendar to check the actual usage of this hashset and remove it eventually in its own PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I did when asking for this. Just remove the field completely. If you clear all the elements, the UT still works => it doesn't have a purpose

@mary-georgiou-sonarsource mary-georgiou-sonarsource merged commit 84a28d7 into master Aug 23, 2023
28 checks passed
Best Kanban automation moved this from Review approved to Validate Peach Aug 23, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource deleted the mary/delete-S2255 branch August 23, 2023 10:26
@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from Validate Peach to Done in Best Kanban Sep 1, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.9 milestone Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Best Kanban
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants