-
Notifications
You must be signed in to change notification settings - Fork 51
Rules to only allow loose comparison with numeric operands #41
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
Conversation
@carusogabriel Please take a look |
Added loose comparison checks to README.
Blindly copied from `OperandsInArithmeticAdditionRule`, but doesn't belong here.
Fixed merge error in rules.neon. Fixed tests for v0.11 where TypeNodeResolver requires a container.
@ondrejmirtes Fixed and updated. If anything more is needed for this to be merged, please let me know. |
|
Co-Authored-By: Jáchym Toušek <[email protected]>
Hi, I tried out this rule and when I get this message:
It's not obvious to the user what they should do instead. So what would probably be better is to center the wording about "you need to use Can you bring some ideas about the rule positioning and wording with regard to this? Thanks. |
@ondrejmirtes I'm happy to change the text. However, I'm unsure about the wording. The current messages are copied from the existing rules for arithmetic operators and boolean conditions. Do you want something completely different? What is a good example that I might copy?e Note that what you should do isn't always clear for other operators than |
IMHO, the rule should only apply to <, >, <=, >= and <=>.
|
Loose comparison does loose casting with unexpected result. To achieve the purpose of this library, this has to be dealth with.
Similar to arithmetic operators (
+
/-
/*
//
/**
/%
), loose comparison operators should only be used for numeric (andDateTimeInterface
) values. This is true for==
,!=
,<
,>
,<=
,>=
and<=>
.Using any of these operators for non-numeric values may lead to unexpected results.
See issue #40 for more info.