Skip to content

Conversation

@Mirroar
Copy link
Contributor

@Mirroar Mirroar commented Jun 22, 2022

This as support for typehinting in array return types as described in #21. It doesn't add support for array shapes like array{foo: int, bar: string}, which is also supported by PHPStan. But it's enough to allow both tools to work together on a project.

Copy link
Contributor

@MichelHartmann MichelHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Mirroar, thank you very much for taking your time to provide this pull request.
It looks great, and I will be happy to approve it.
I only found two tiny pieces, I would suggest we get sorted in advance...

}

foreach ($matches['nested'] as $index => $match) {
if (!empty($matches['generic'][$index])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the nature of empty this expression would be false for strings like "0".
Consider checking for null and equality to '' instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$matches['generic'][$index] will always either be completely empty because it's an optional expression, or include at least the characters <>, so this should be fine.
I'll still change it if you'd like me to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I would still prefer the explicit check for '' in order to get rid of the infamous empty().

@Mirroar
Copy link
Contributor Author

Mirroar commented Jun 27, 2022

Hi @MichelHartmann, I've applied the suggestions we talked about.

Copy link
Contributor

@MichelHartmann MichelHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Mirroar, thank you very much.

@MichelHartmann MichelHartmann merged commit 7e2ccbb into flyeralarm:master Jun 27, 2022
seb-h-k pushed a commit that referenced this pull request Aug 26, 2022
…s tests are failing

This reverts commit 7e2ccbb, reversing
changes made to 5bac909.
seb-h-k added a commit that referenced this pull request Aug 29, 2022
Revert "Merge pull request #22 from Mirroar/phpstan-array-generics" a…
@Mirroar Mirroar deleted the phpstan-array-generics branch November 23, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants