-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feat: Support more constraint operators #47
Conversation
…erted' option was set to true
'contextName' => 'email', | ||
'operator' => ConstraintOperator::STRING_STARTS_WITH, | ||
'inverted' => true, | ||
// missing 'value' and 'values' and 'inverted' set to true |
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.
nice =)
protected function convert(string $number): int|float | ||
{ | ||
if (str_contains($number, '.')) { | ||
return (float) $number; |
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.
nice, I will add a float test to the specifications also.
{ | ||
assert(is_string($searchInValue)); | ||
|
||
return str_ends_with($searchInValue, $currentValue); |
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.
Is str_ends_with
supported in PHP 7.2+, or is it only PHP 8.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.
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.
8.0, but this repo contains a downgrader which downgrades the code to multiple version, it gets automatically changed to something suitable for each version during deploy, see for example here for version downgraded to php 7.4.
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.
And here you can see one of the str_* functions downgraded:
if (substr_compare($url, '/', -strlen('/')) !== 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.
This is nice. Thanks for explaining 👍🏼
{ | ||
assert(is_string($searchInValue)); | ||
|
||
return str_starts_with($currentValue, $searchInValue); |
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.
only from PHP 8?
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.
Explained in previous comments, gets downgraded to suitable expression for each version.
{ | ||
assert(is_string($searchInValue)); | ||
|
||
return str_contains($currentValue, $searchInValue); |
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.
only from PHP 8?
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 looks good!
It looks to me some of the operators used will only work on PHP 8. It would be good to use simple work-around to make sure they also work on PHP 7, given that it is possible and they are straight forward.
As mentioned before, it gets downgraded for each version, that's why it's kinda easy to support older versions without sacrificing the features of new versions. |
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.
lgtm :D
closes #42
Before merging the reference to client-specifications repository should be put back to the
main
branch.