-
Notifications
You must be signed in to change notification settings - Fork 659
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
fix error with 0 being considered positive #7487
Conversation
'noErrorPushingBigShapeIntoConstant' => [ | ||
'code' => '<?php | ||
class DocComment | ||
{ | ||
private const PSALM_ANNOTATIONS = [ | ||
"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", | ||
"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "" | ||
]; | ||
}', | ||
'assertions' => [ | ||
], | ||
], |
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.
Does this test actually fail before the fix? I tried the same thing with the actual values from DocComment but for some reason it passed fine.
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.
it does: https://psalm.dev/r/1529295950.
EDIT: see result from Psalm's bot
EDIT2: also see the failing CI on master. I retrieved the error from there
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.
Although I noticed that it started failing after 52 values in there. DocComment must have around 55, so maybe you didn't include them all?
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.
@orklah Oh I see what happened, I used --filter
for my test, but I forgot paratest
doesn't support that, you have to use phpunit
directly. I saw the success message and didn't notice it said 0 tests...
Psalm bot, please fetch this for posterity: https://psalm.dev/r/1529295950 |
I found these snippets: https://psalm.dev/r/1529295950<?php
class DocComment
{
private const PSALM_ANNOTATIONS = [
"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", ""
];
}
|
fix a bug introduced in #7473. When combining ints, we were flagging 0, using TPositiveInt and then check if TPositiveInt to add 0 to the combination. When I removed TPositiveInt, I wrongly assumed this had become unused code