-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Turn some code style rule severity from suggestion to warning #20680
Conversation
75114d1
to
6cba77e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
#pragma warning disable IDE0047 | ||
(*op++) = *ip++; | ||
#pragma warning restore IDE0047 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to disable this here? Are the parentheses not unnecessary but the warning still appears? Line 251 has the same? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try removing parenthesis. Something very weird is going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove the parenthesis rule SA1023 kicks in:
Dereference symbol '*' should not be preceded by a space. csharp(SA1023)
#pragma warning disable IDE0047 | ||
(*(c + s * 256 + h)) = Color.FromAhsv(h / 255f, 1 - s / 255f, V).ToArgb(); | ||
#pragma warning restore IDE0047 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Also the outer for loop would need braces.
@@ -183,6 +183,9 @@ dotnet_diagnostic.CA1827.severity = warning | |||
# Use Length/Count property instead of Enumerable.Count method. | |||
dotnet_diagnostic.CA1829.severity = warning | |||
|
|||
# Use span-based 'string.Concat' (incompatible with mono builds). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe at least add a TODO here, reminding this should be enabled if/when we drop Mono?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mono is mentioned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Use null propagation
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0031
Remove unnecessary parentheses
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0047-ide0048
Silencing
Use span-based 'string.Concat'
suggestion as span it is incompatible with MONOhttps://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1845