Skip to content

Remove redundant type in @param occurrences #54523

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove redundant type in @param occurrences #54523

wants to merge 1 commit into from

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Apr 8, 2024 •
edited
Loading

Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

This decision was stated in many places, here is the most recent (I guess).

A Rector rule did the job, however it was not accepted in the upstream:

Still I applied it + PHP CS Fixer phpdoc_align with vertical to get this result. Finally, an issue has been reported to Jetbrains so PHPStorm may supports this syntax: https://youtrack.jetbrains.com/issue/WI-76947/Allow-param-to-omit-the-type. 🤞

@carsonbot carsonbot added this to the 7.1 milestone Apr 8, 2024
@alexandre-daubois alexandre-daubois mentioned this pull request Apr 8, 2024
Closed
@derrabus
Copy link
Member

derrabus commented Apr 8, 2024

How's tool support for this syntax these days?

@alexandre-daubois
Copy link
Member Author

  • PHPStorm struggles with this, hope my issue in Jetbrains' Youtrack gets some attention!
  • Psalm seems ok, as it didn't complain in this PR
  • PHPStan handles it well: https://phpstan.org/r/e471345c-3f0b-4a71-a593-c72639017700
  • phpDocumentor defines the type as optional so it's also fine
  • VSCode seems also fine with it

Am I missing something? I'd be happy to test more!

@norkunas norkunas mentioned this pull request Apr 12, 2024
Open
@nicolas-grekas
Copy link
Member

Please allow me to close here as this would need some automation to enforce in the long run. At the moment, this isn't worth the review+conflict-resolution time this will create IMHO.

@alexandre-daubois
Copy link
Member Author

Fair enough 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN left review comments

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@welcoMattic welcoMattic Awaiting requested review from welcoMattic welcoMattic is a code owner

@kbond kbond Awaiting requested review from kbond kbond is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@OskarStark OskarStark Awaiting requested review from OskarStark OskarStark is a code owner

@jderusse jderusse Awaiting requested review from jderusse jderusse is a code owner

Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
5 participants
@alexandre-daubois @derrabus @nicolas-grekas @GromNaN @carsonbot