-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
SA Checks: fromIterable, empty #96
Conversation
/** @var array<NewTKey, NewT> $emptyArray */ | ||
$emptyArray = []; | ||
|
||
return self::fromIterable($emptyArray); |
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 the only way to get around having template parameters for the empty array, otherwise PHPStan complains because it cannot figure out the template types:
301 Method loophp\collection\Collection::empty() should
return loophp\collection\Collection<NewTKey of
(int|string), NewT> but returns
loophp\collection\Collection<*NEVER*, *NEVER*>.
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.
Should we report that as a bug ?
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.
yeah leave it with me. Looks like a valid issue though maybe I thought it makes sense because it has no way to know where the template parameters are coming from
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! phpstan/phpstan#5065
Let's see what they say
* @template TKey of int | ||
* @template T of array{0: NewTKey, 1: NewT} | ||
* | ||
* @template NewTKey of array-key | ||
* @template NewT |
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 literally must've spent over an hour getting these type hints right 😄
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.
but it's beautiful when the static analysis finally turns green ✅ 😆
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.
damn looks like this is not actually supported yet in PHPStan 😄. Not sure why it was showing all good in another file
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.
trying to work around it makes this worse and incorrect, so I think I'll keep this. Psalm supports it and it passes, and PHPStan should support it soon: phpstan/phpstan#3931
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.
Can't wait for this to be supported !
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's now supported! phpstan/phpstan-src#673
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 job! 💯
- phpstan-docs-baseline.neon | ||
- phpstan-unsupported-baseline.neon |
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.
-
the docs baseline is just issues in
docs/
so nothing important but fixing them requires diving into other methods, which I will do anyway in future PRs. -
the unsupported baseline refers to the PHPStan unsupported feature I mentioned here. It's limited to the
unpack
operation and I expect we'll be able to get rid of it once@template T of array
is supported
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 and clean!
This is why I was using @psalm-template
By the way, what do you think about removing that of array-key
?
As this is a lazy library, the TKey can be of any type, so, I don't think I'm wrong by saying we can get rid of it. WDYT ?
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 you're right we could get rid of array-key
but I don't think it can only be done in one place because it will probably result in static analysis errors.
I'll look to prepare a PR after this to remove it everywhere and see how many errors we get 😄
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.
Yeah I did it in #95 maybe it could be a starting point.
@drupol btw what's the process for releases and how often do we do them in this project? |
There is actually no strict release process and I'm absolutely not against to setup one. If you want, I can make a point release this morning. Do you prefer to wait until this PR is merged or I do it before? |
This PR is ready from my point of view so let me know if there's anything left to address 😁 . |
4.0.7 released! |
Thanks again for your amazing contributions! |
This PR adds to the static analysis checks:
fromIterable
,empty
.tests/
in GrumPHPIterableIterator
Unpack
-> surfaced by thesquash
operation now thatfromIterable
has the right type hints