-
Notifications
You must be signed in to change notification settings - Fork 145
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
Allow to pass a thunk as a message factory to assertions #221
base: master
Are you sure you want to change the base?
Conversation
f1379a5
to
48fb067
Compare
* @param mixed $value | ||
* @param string $message | ||
* @param mixed $value | ||
* @param string|callable():string $message |
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.
I did not implement it to all assertions. If RFC is accepted, I'll add it everywhere
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.
Actually, looking at this again, I wonder if using Stringable|string
would be more appropriate, thus dropping the need for a closure.
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.
How would you expect it to work?
my proposal is
Assert::string(1, fn() => heavy compute here)
With Stringable it has to be more verbose
Assert::string(1, new class() { public function __toString() { ... } })
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.
@simPod yes, more verbose. And also more type safe, given that callable(): string
is not enforceable (without static analysis) but Stringable|string
is. 🤔
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.
In any case, I am not merging new features right now, but this is high priority for the next feature release.
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.
Same as array<object|string>
is not enforceable in isInstanceOfAny
, that's not valid argument IMO.
I'm not asking for merge now
What would be the real life use case scenario for this? |
Sometimes my error messages are
so I'd rather skip that for happy path. |
397b0a4
to
c546acd
Compare
@BackEndTea Hi, is there a will to add lazy messages? |
Interesting idea, and certainly something I would consider for the next feature release. |
It is useful in order to delay generating a message. At the moment the assertion does not pass the message is generated. Otherwise the thunk is not executed.
It is useful in order to delay generating a message.
At the moment the assertion does not pass the message is generated. Otherwise the thunk is not executed.