Skip to content
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 accuracy of Collection::isEmpty and isNotEmpty assertions #52184

Merged

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented Jul 18, 2024

Those methods do not necessarily prove that the result of first() is not null. However, they can prove that the result of first() is of the type contained within the Collection.

Those methods do not necessarily prove that the result of `first()` is not null.
However, they can prove that the result of `first()` is of the type contained within the `Collection`.
@spawnia spawnia changed the title Improve accuracy of Collection::isEmpty and isNotEmpty Improve accuracy of Collection::isEmpty and isNotEmpty assertions Jul 18, 2024
@johanrosenson
Copy link
Contributor

johanrosenson commented Jul 18, 2024

This solution i stand behind 100%.

Have you verified that this method is actually working? I think I tried this solution originally but I didn't get the results I expected, but maybe I was too far down the rabbit hole at that point and maybe I missed something.

Another thought I had was that @phpstan-assert-if-true should not really be null either, because it's actually returning the value of $default, can you see if you get it working with @phpstan-assert-if-true TFirstDefault $this->first() ?

I think I tried that also, but again, didn't get the desired result, but I might have missed something in my testing.

@spawnia
Copy link
Contributor Author

spawnia commented Jul 18, 2024

Have you verified that this method is actually working?

I think the additions i made to types/Support/Collection.php prove it.

Another thought I had was that @phpstan-assert-if-true should not really be null either, because it's actually returning the value of $default, can you see if you get it working with @phpstan-assert-if-true TFirstDefault $this->first() ?

The assertions only affect calls that are exactly first(). When parameters are passed, the assertion has no effect. See 858be4b for additional tests that prove this.

@taylorotwell taylorotwell merged commit 90464d3 into laravel:11.x Jul 18, 2024
28 checks passed
@spawnia spawnia deleted the preserve-nullability-in-collection-asserts branch July 18, 2024 20:12
@johanrosenson
Copy link
Contributor

Thank you @spawnia for improving it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants