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

Add more precise types for the prophesize method #63

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

stof
Copy link
Member

@stof stof commented Mar 13, 2024

This matches the type used in Prophecy as the method is forwarded to it.

This matches the type used in Prophecy as the method is forwarded to it.
* @psalm-param class-string|null $classOrInterface
* @template T of object
* @phpstan-param class-string<T>|null $classOrInterface
* @phpstan-return ($classOrInterface is null ? ObjectProphecy<object> : ObjectProphecy<T>)
Copy link

@jseparovic1 jseparovic1 Mar 13, 2024

Choose a reason for hiding this comment

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

Suggested change
* @phpstan-return ($classOrInterface is null ? ObjectProphecy<object> : ObjectProphecy<T>)
* @return ($classOrInterface is null ? ObjectProphecy<object> : ObjectProphecy<T>)

Does it require phpstan specific annotation? This way I assume it could work with both psalm and phpstan.
https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types#conditional-return-types

Update
Never mind i see that underlaying method is also using phpstan-return.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both phpstan and psalm will read the annotation with the prefix of the other tool if they don't have their own prefixed annotation (but with a different error handling as they will skip an annotation they cannot support instead of reporting an error).
The reason they do that is to avoid forcing all libraries to provide duplicate annotations to let downstream projects benefit from their type when being analyzed.

The general rule is that libraries should apply the prefix corresponding to the tool they use themselves (so that their own analysis report invalid annotations). And as I'm more a phpstan user than a psalm one myself, I would be using phpstan when adding a static analysis setup in this package (which I will probably do at some point).

I'm not using @return here because conditional types are supported by psalm and phpstan but not by other tools dealing with phpdoc, which might then be confused about them.

Choose a reason for hiding this comment

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

Thank you for the explanation! Obviously I wasn't aware on how they approach this but it makes sense as writing two types of anotations would be a maintenance burden.

@stof stof merged commit 0604e27 into phpspec:master Mar 13, 2024
8 checks passed
@stof stof deleted the generic_types branch March 13, 2024 11:18
Jean85 added a commit to facile-it/paraunit that referenced this pull request Mar 18, 2024
@InvisibleSmiley
Copy link

Can we have a release including this change/fix please?
I'd like to get rid of master branch dependencies and after phpstan-propehcy (which just released version 1.0.1) this is the last blocker on the way toward a "stable" Prophecy dependency tree.

@alexander-schranz
Copy link

@stof do you think we could create a release with this one?

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.

4 participants