-
Notifications
You must be signed in to change notification settings - Fork 65
Allow omitting @param type #127
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
Conversation
a24cc02
to
66ae696
Compare
We should be able to do this without breaking BC. Because @param
without a type is essentially useless, I feel like we shouldn't mix it with other useful @param
tags that have a type.
So my suggestion is:
- Do not make
$type
insrc/Ast/PhpDoc/ParamTagValueNode.php
nullable. - Create a separate
TypelessParamTagValueNode
that can still carry$description
. PhpDocNode::getParamTagValues()
WIL NOT returnTypelessParamTagValueNode
, it needs to be filtered out.- Add
PhpDocNode::getTypelessParamTagValues()
. - I'll release that in minor phpdoc-parser version.
- Add some tests to phpstan-src that demonstrate this is no longer a parse error, and potentially get and make use of some data carried by
getTypelessParamTagValues()
(but I don't think we use parameter description anywhere).
Thank you!
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 the conclusion of #77 is that /** @param $foo */
shouldn't be a parse error either. The description can be an empty string, and if someone wants, they can have a rule to mark such tag as useless, but it would no longer be an objective bug to have such PHPDoc in the code...
So the grammar description + parser should be update to allow this tag.
1dcf081
to
d9b660c
Compare
Perfect, thank you!
wouterj
commented
Jun 8, 2022
Thank you Ondřej and Richard!
Wirone
commented
Jun 10, 2022
This one made our baseline files thinner by ~3k entries 🙈 I am not 100% convinced that this is good behaviour but I understand the reasoning behind it. Cheers! 🍻
p-golovin
commented
Jun 16, 2022
Is it possible to revert the old behavior by configuration flag?
Why?
p-golovin
commented
Jun 16, 2022
It was useful for us to detect this errors because they are usually show old code with bad type hinting (in our code base). We check and fix them step by step. So we need to stay at 1.7.11 version before will fix them all.
@p-golovin As said earlier in this thread, you'll get the same errors on level 6 (missing parameter typehint).
p-golovin
commented
Jun 16, 2022
@ondrejmirtes Thank you. We'll try to raise level to 6 (now we make fixes on level 3).
As said earlier in this thread, you'll get the same errors on level 6 (missing parameter typehint).
Really? We're on level 6 and upgrading caused many "ignored ... not found", so we had to regenerate baselines and maaaaaaany ignores were removed. 🤔
EDIT: remember, do not comment when too tired to understand 😉
wouterj
commented
Jun 16, 2022
see #77 (comment)
Uh oh!
There was an error while loading. Please reload this page.
Fixes #77