Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix and improve overall PHPDocs#54489
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e6a8601 toccd7a67Compare
GromNaN left a comment
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 type of thecallable params should not be updated. Or possibly in the next minor 7.1.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
xabbuh commentedApr 4, 2024
This should target 7.1 as it's not a bugfix. |
alexandre-daubois commentedApr 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree but as it doesn't change any behavior in the code. Wouldn't it be easier for future upmerges and conflicts resolving to push this to 5.4? Also, some phpdocs are wrong, so I guess it can be considered as a bug fix? I'm not sure |
This also remove a few useless ones already inferred in the code
| * @param string[] $tokens the set of split tokens (e.g. COMP_WORDS or argv) | ||
| * @param $currentIndex the index of the cursor (e.g. COMP_CWORD) | ||
| * @paramint $currentIndex the index of the cursor (e.g. COMP_CWORD) |
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 intended.
I think we should rather remove all types everywhere if they duplicate the type defined in PHP (which is almost always the case, except for generic types). See#53363 (comment)
But if we do this, we should automate this using Rector or PHP CS Fixer.
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 an interesting point of view. Are we 100% we want to do this? Rector could definitely do the trick with this
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.
Yes we are :)
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.
Alright, I might close this PR in favor of one that removes redundant types as explained by Wouter. Should such PR target 5.4 or 7.1?
| * @param string$message Exception message to throw | ||
| * @param array$alternatives List of similar defined names | ||
| * @param int$code Exception code | ||
| * @param?\Throwable $previous Previous exception used for the exception chaining |
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.
for consistency with other similar changes:
| * @param?\Throwable$previous Previous exception usedfor the exception chaining | |
| * @param \Throwable|null$previous Previous exception usedfor the exception chaining |
| * | ||
| * @param mixed $format | ||
| * @param string $locale | ||
| * @param mixed $format |
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.
| * @param mixed$format | |
| * @param mixed$format |
alexandre-daubois commentedApr 8, 2024
Closing in favor of#54523 |
This also remove a few useless ones already inferred in the code and flipped non-canonical
@varoccurrences.