-
Notifications
You must be signed in to change notification settings - Fork 30
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
Modify rule S2198: LaYC format #2156
Conversation
1479d61
to
15b9add
Compare
15b9add
to
cb44ece
Compare
cb44ece
to
8880808
Compare
* comparing a `char` with a numeric constant that is outside of the range of `char` | ||
* comparing a `float` with a numeric constant that is outside of the range of `float` | ||
* comparing a `long` with a numeric constant that is outside of the range of `long` | ||
* comparing a `ulong` with a numeric constant that is outside of the range of `ulong` |
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.
Do you think it would be worth it linking the msdn documentation to the types? Like
https://learn.microsoft.com/en-us/dotnet/visual-basic/language-reference/data-types/ulong-data-type for ulong?
This would enable users to find the ranges more quickly.
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.
I have added two links:
- to the paragraph on the page about integral numeric types, that shows ranges for all numeric types
- to the page for the
char
data type, that shows the range for the type
I have also added etc.
since the list provided here is not comprehensive.
@@ -1,3 +1,3 @@ | |||
{ | |||
|
|||
"quickfix": "targeted" |
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.
What would be the quickfix in your opinion?
Is it just removing the comparison?
I am afraid that might change the behaviour of the code. Imagine for example:
If (f > double.Maxvalue)
{
Console.WriteLine("I am never printed");
}
If you remove the check, the code will actually execute. Would you in this case remove also the body of the if as it was unreachable code?
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.
The fix would either remove the entire if
block, or just the if
condition.
For example, if the code is if (f > double.MaxValue) Body
the entire if
would be removed.
If the code is if (f <= double.MaxValue) Body
the if
would be replaced with Body
.
The same would apply to conditions like f < double.MinValue
and f >= double.MinValue
.
Indeed, there is actually the risk of changing the behavior of the code, for example in the following example:
if (SomeMethod() > int.MaxValue) Console.WriteLine(""Never being executed");
int SomeMethod() => throw new Exception();
If we would apply the code fix here, the entire if
block would be removed, and the code would not raise an exception anymore. However, we can limit the code fix to fields only, if we want to avoid this corner case.
Anyway, even if there are subtleties to be taken care of, I would keep the targeted
, since there is a realm where in principle the code fix is possible. So I would keep the door open for now, and defer the deeper analysis to a possible future task to specify the code fix behavior.
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.
Two nitpicky comments. Plus in addition, I would like to ask you to remove the `++ stuff in the base rule.adoc.
I am approving as all of these are optional.
8880808
to
0ee7e24
Compare
SonarQube Quality Gate for 'rspec-frontend' |
SonarQube Quality Gate for 'rspec-tools' |
I have removed the |
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.
LGTM!
Review
A dedicated reviewer checked the rule description successfully for: