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-936 Modify S3247: Add benchmarks #4596

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

martin-strecker-sonarsource
Copy link
Contributor

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

@sebastien-marichal sebastien-marichal changed the title Modify S3247: Add benchmarks NET-936 Modify S3247: Add benchmarks Jan 7, 2025
Copy link
Contributor

@sebastien-marichal sebastien-marichal left a comment

Choose a reason for hiding this comment

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

LGTM!
I have some minor concerns that could be ignored.


[options="header"]
|===
| Method | Runtime | Mean | Standard Deviation | Ratio
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't always include the Ratio column.
I guess it make sense with the Benchmark category.
However, if we decide to keep it, I am wondering what would be the result in the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not include the Ratio column.
I talked with Greg that spend some time on what to show in Benchmarks and said the Ratio was misleading.

Comment on lines 63 to 64
| IsPattern_ValueType | .NET 9.0 | 22.58 ns | 1.161 ns | 1.00
| IsWithCast_ValueType | .NET 9.0 | 19.41 ns | 2.675 ns | 0.86
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope that it will not bring more work for us and that this point:

* Performance: Performing the type check and cast separately can lead to minor performance issues. While this might not be noticeable in small applications, it can add up in larger, more complex systems.

will be enough for users to not be too picky about this rule being tagged as performance.

Copy link

sonarqube-next bot commented Jan 9, 2025

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

Copy link

sonarqube-next bot commented Jan 9, 2025

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

@martin-strecker-sonarsource martin-strecker-sonarsource merged commit 8536b27 into master Jan 9, 2025
8 of 9 checks passed
@martin-strecker-sonarsource martin-strecker-sonarsource deleted the Martin/Benchmark_S3247 branch January 9, 2025 10:00
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.

2 participants