-
Notifications
You must be signed in to change notification settings - Fork 666
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
Use objects, not strings, for assertions #7410
Conversation
This looks really good! |
Hi @muglug ! Do you think you'll finish this soon? I'd really like having this in the alpha for Psalm 5! |
Yup, should have unit tests passing today |
284c23a
to
a707303
Compare
a707303
to
52aeeb2
Compare
Sorry, my last merge on master broke CI. I'll fix that |
Could you add some details on UPDGRADE.md to explain that the old Assertion was renamed and other BC breaks introduced? |
every call to getAssertionString is now with $exact to true. Can we drop the param? |
This makes so many things looks a lot more clearer! Very cool! Thanks :) |
|
||
public function __toString(): string | ||
{ | ||
return '!non-empty'; |
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.
@muglug Is this a BC break regarding projects (including Assert tools) that used empty
or !empty
as a docblock assertion?
Is there a reason why you didn't kept it as empty
?
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 can be “empty”, but that should have no impact on the parsing of assertions either way — do you have an example of failing 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.
Somehow it still works? https://psalm.dev/r/bb6d82a6a3 https://psalm.dev/r/90da8dba12
I didn't expect that. What is the system that parses assertion strings into Assertion objects?
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.
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.
Thanks! I never had to tweak here before... Weird, I thought there would have been a direct link between internal assertion names and user assertions ones. But this answer my question! Thanks both!
It's a big addition, but this would make it slightly easier to track where assertions were being used in the codebase, and removes a lot of checks for
$assertion[0] === '!'
etc.