-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Drop psalm #18340
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
Drop psalm #18340
Conversation
@@ -74,7 +74,7 @@ class Cache | |||
* class names. | |||
* | |||
* @var array<string, string> | |||
* @psalm-var array<string, class-string> | |||
* @phpstan-var array<string, class-string> |
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.
with stuff like that can't it be merged into the normal @var
phpdoc block?
I believe @var
and @return
phpdoc definitions support stuff like class-string and generics
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.
at least phpstan understands it. don't know how our api docs generator handles it though
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.
We would have to check how far also IDE are in understanding the more stan based annotations.
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.
laravel uses it for quite some time
https://github.com/laravel/framework/blob/ff4f657ca9d9af36e021a67c6eeb97949a0e27d0/src/Illuminate/Support/Facades/Exceptions.php#L45
e.g. look at this insane definition
https://github.com/laravel/framework/blob/ff4f657ca9d9af36e021a67c6eeb97949a0e27d0/src/Illuminate/Database/Eloquent/PendingHasThroughRelationship.php#L50-L63
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.
Yeah, thats exactly how you dont do it :) Ever.
It means you lose all capability of your IDE to give you feedback at real time.
Especially since it is easy enough to add them prefixed on top for STAN tools to also work, and basically have the perfect tradeoff.
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.
with stuff like that can't it be merged into the normal @var phpdoc block?
Yes it can be merged. These annotations most likely predate the support for this form in phpstorm. Hence the prefix.
I ran a simple replace of @psalm-
to @phpstan-
. Not going to review each annotation manually :) We can clean them up as and when anyone is working on that part of the code.
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.
At least with the free PHP-Tools VS Code extension it does autocompletion correctly for class-string definitions
I can't tell how other LSP's handle that (like Neovim's) but PHPStorm definitely understands 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.
class-string is fine
I think the other examples you showed are bit too crazy :)
Hi guys. Just for my personal knowledge, is it possible to know what led you to this choice? |
|
Addresing it isn't a priority for them either vimeo/psalm#11340 (comment). There's also issues like this psalm/phar#20 |
Thank you so much for the explanations. I also learned something new from this community today :D |
No description provided.