-
Notifications
You must be signed in to change notification settings - Fork 83
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
Linter checks for the switch(rand(L, H)) pattern #302
Linter checks for the switch(rand(L, H)) pattern #302
Conversation
if the range is not fully covered by cases a warning is thrown if a case is outside of the rand range a warning is thrown
Warnings emitted on tgstation master for reference:
|
I'm dumb
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.
Extremely good, thank you
Can you make it possible to disable warning
If I replace |
I recommend leaving it blank with an explanatory comment: switch(rand(1, 20))
if(1 to 9)
bad_effect()
if(10)
// Neutral roll, do nothing.
if(11 to 20)
good_effect() This also works fine with switch(rand(1, 20))
if(1 to 17)
get_hurt()
if(18 to 20)
; // Defense succeeded, no damage. |
A pattern such as
is not uncommon in SS13 code. Sadly, it is also not uncommon for people to add new cases to a longer switch of this form and forget to change the
rand
arguments at the top. This PR adds a linter warning for when therand
range is not fully covered by the cases and for when a case lies completely outside of therand
range.This currently generates 7 warnings on tgstation master. Sadly, it is (at least to me) not quite clear if the omissions of a part of the
rand
range are intentional there or not. However, I believe that even if they are intentional it is worth adding the missing part of the range as a case with empty body explicitly to make the intent clear to future developers.As a less related (but perhaps more important) addition this PR now also adds a warning for switch branches of the form
if(X || Y)
which is pretty much always a mistake and should instead beif(X, Y)
. This lint generates 5 warnings on current tg code, seemingly all being actual mistakes.