-
-
Notifications
You must be signed in to change notification settings - Fork 51
Fix: An array can have only integers or strings as keys #132
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
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 for this improvement, it would be nice if you can update the PR with my requested change.
After that, this should be ready to merge.
src/Types/ClassString.php
Outdated
|
||
/** | ||
* Value Object representing the type 'string'. | ||
* | ||
* @psalm-immutable | ||
*/ | ||
final class ClassString implements Type | ||
final class ClassString extends String_ |
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 think we forgot this one when introducing the PseudoType interface, Could you please add that as well here.
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.
done
src/Types/ArrayKey.php
Outdated
{ | ||
public function __construct() | ||
{ | ||
parent::__construct([new String_(), new Integer()], '|'); | ||
} | ||
|
||
public function underlyingType(): Type | ||
{ | ||
return new String_(); |
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 seems incorrect to me, this type is not a string, it should return a Compound
with the types String_
and Integer
. ArrayKey is an alternative notation for String|Int
, not just String
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.
fixed
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 for yet another nice contribution!
We should allow all kind of string types here. We currently do not have an array-key interface or something like that, so I added the new type into the check.