-
Notifications
You must be signed in to change notification settings - Fork 509
More precise constant variants of BooleanType #3781
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
@@ -53,4 +53,4 @@ function returnsBool(): bool { | |||
assertType("' 1'", $s); | |||
|
|||
$s = sprintf('%20s', returnsBool()); | |||
assertType("lowercase-string&non-falsy-string", $s); | |||
assertType("' '|' 1'", $s); |
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.
looks great, see https://3v4l.org/ipPIP
@herndlm opinions? |
This pull request has been marked as ready for review. |
Interesting idea! Makes sense IMO for "finite" types like that |
hmm interessting wording you use here. makes me wonder what the semantic difference of
|
I might be also just mixing up things. Don't even know all those related methods that currently exist on the interface. But good point 🤔 |
Quickly checked, looks like you could implement that finite method as well here |
|
src/Type/BooleanType.php
Outdated
public function getConstantStrings(): array | ||
{ | ||
return []; | ||
return [new ConstantStringType('1'), new ConstantStringType('0')]; |
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.
Nope. This function isn't used for loose type casts like that.
Thank yuo. |
instead of resolving bool into true/false in the extension, lets do it in the type-system itself