-
Notifications
You must be signed in to change notification settings - Fork 148
Fix comparisons with NAN #306
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
base: master
Are you sure you want to change the base?
Conversation
@gordinskiy thank you for the additional explanation, it is very helpful to understand the context and other approaches. I do not want to introduce an self::assertNumeric($value);
self::assertNumeric($limit);
if ($value <= $limit) {
static::reportInvalidArgument(\sprintf(
$message ?: 'Expected a value greater than %2$s. Got: %s',
static::valueToString($value),
static::valueToString($limit)
));
} |
@shadowhand My first solution was similar but I abandoned it due to the difficulties with customizing messages from nested assert (maybe it's just not necessary). The problem with We can add one more assertion for these cases - I had thoughts about adding an assertion |
@gordinskiy I think if (!is_numeric($value) || !(is_float($value) && is_nan($value)) { /* throw, etc */ } |
37a7014
to
3488270
Compare
|
self::numeric($value); | ||
} | ||
|
||
if (is_numeric($value)) { |
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 (is_numeric($value)) { | |
if (is_numeric($limit)) { |
Attempt to fix #302
What a wonderful bug we found.
I'll start from afar and take the function
Assert::greaterThan($value, $limit)
as an example.The asserted condition is literally described in PHP as follows:
And when we need to describe opposite condition in which our assertion must throw an exception there are at least two ways:
!($value > $limit)
(literal)$value <= $limit
(simple)In the first case, the condition is described literally as the logical negation of the assertion.
In the second case, the same conditions is described, but in a more readable form using only one operand.
Its a common practice to avoid negative conditions for better readability.
So it is not surprising that many methods in this library use the second one.
Unfortunately this doesn't work with
NAN
.In normal cases both form give the same result:
But it doesn't work that way for
NAN
.Any comparison with
NAN
will returnfalse
(except!=
and!==
).And our forms of negative condition now return different result:
In addition to functions mentioned in issue I found 9 more assertions that lead to unexpected results when passing
NAN
.I added additional test cases for those method in this PR.
Solutions
Straightforward one.
Just change the form of the negative condition from literal to simple.
This is the solution I described in this PR.
Another solution is to left simple condition but add additional checks for NAN for each of arguments.
Method to check for
NAN
:And the condition for Assert::greaterThan function will be:
This is a more readable but verbose method.
I like the second solution more. To reduce verbosity we can change check-method signature to
isNaN(...$values)
oratLeastOneIsNaN(...$values)