-
-
Notifications
You must be signed in to change notification settings - Fork 440
implement types and constructor property promotion for utilities #1124
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
This reverts commit 890fe11.
|
I really think all property constructer promotions should be on a new line like, even if its only one: public function __construct(
private LoggerInterface $logger = null,
) {
}it increases readability a lot. Not sure what you think @weaverryan |
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
agree'd |
| * use statement would appear as "use Some\Class::class as 'ZXY'". It is ok | ||
| * to mix non-aliases classes with aliases. | ||
| * | ||
| * @param string[]|array<string, string> $classesToBeImported |
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.
@OskarStark - over in symfony, do you guys use any of the #[ArrayShape] attributes instead of the param notations?
Something like https://blog.jetbrains.com/phpstorm/2020/10/phpstorm-2020-3-eap-4/#arrayshape or similar?
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.
Can't remember
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.
No, Symfony is mostly sticking to phpDocumentor's types and annotations (with some occasional Psalm types).
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.
Thank you Wouter 😃
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.
Thanks Wouter! I'm getting curious about using #ArrayShape from PHPStorm, or "cloning" it in MakerBundle's NS so we can use it here. We have quite a few multi-dimensional arrays that I've run across when refactoring the code over the past couple of weeks - I'm having to jump back and forth to figure out what the array is supposed to look like.
On the flip side, this could be a sign we should be using a simple object representation instead of a complex array in those cases...
| { | ||
| $this->metadata = $metadata; | ||
| public function __construct( | ||
| private ClassMetadata|LegacyClassMetadata $metadata |
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.
Note to self: we may be able to remove the LegacyClassMetedata - depends on doctrine version constraints...
Co-authored-by: Ryan Weaver <weaverryan+github@gmail.com>
| { | ||
| $this->phpCompatUtil = $phpCompatUtil; | ||
| public function __construct( | ||
| private PhpCompatUtil $phpCompatUtil |
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 should always end with a ,, it makes future diffs easier to read, also there is a PHP-CS-Fixer rule
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.
Good catch! thanks Oskar. Done in #1127
No description provided.