-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
Improve return type inference of Collection::groupBy. #1860
base: 2.x
Are you sure you want to change the base?
Conversation
assertType('Illuminate\Database\Eloquent\Collection<(int|string), Illuminate\Database\Eloquent\Collection<(int|string), App\User>>', $collection->groupBy('id')); | ||
assertType('Illuminate\Support\Collection<(int|string), Illuminate\Support\Collection<(int|string), int>>', $items->groupBy('id')); | ||
assertType('Illuminate\Database\Eloquent\Collection<int, Illuminate\Database\Eloquent\Collection<int, App\User>>', $collection->groupBy('id')); | ||
assertType("Illuminate\Support\Collection<'', Illuminate\Support\Collection<int, int>>", $items->groupBy('id')); |
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.
If it can't read a property off the types you have in your collection it helpfully groups them all into an empty string group...
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 get why you do this, although not sure I really care for it...
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 don't care for it either but it accurately models what laravel is doing with it's types.. i could make it be new StringType()
but that would give a false sense of security aswell
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.
If it can't read the type then why not just default to (int|string)
like the previous diff had?
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.
because phpstan will report an error when trying to access any other key, alerting the developer to the mistake they have made:
One of the |
yeah already on it :) |
f808594
to
b0fc157
Compare
b0fc157
to
088cf43
Compare
if (count($types) === 0) { | ||
$keyType = new ConstantStringType(''); | ||
} else { | ||
$keyType = TypeCombinator::union(...$types); |
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.
Shouldn't we check if the types are either int
or string
? What I mean is that for example objects can't be array keys. Or if any of the types were nullable we would have for example int|null
as collection key. Which does not make sense.
Maybe we can call toArrayKey
on this type?
closes #1813
Changes
Adds a return type extension for the
Enumerable::groupBy()
method that provides more precise type information