Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config] Add conditional types to conditional builders#46581
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
| * @psalm-param T $value | ||
| * @psalm-return (T is array ? CLASS : static) |
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.
Using vendor prefixed annotations as e.g. PhpStorm has no support for conditional types yet afaik. This allows using@return with something everyone understands, while providing the necessary extra details to static analysis tools.
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.
let's still move this after@return in all cases :)
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.
Let's just say I was a bit unfocused yesterday 😄
Uh oh!
There was an error while loading.Please reload this page.
fbf15a8 toe69e509CompareThis allows static analysis tools to understand what type is returned.
e69e509 tod8500c1CompareUh oh!
There was an error while loading.Please reload this page.
| $body =$hasNormalizationClosures ?' | ||
| /** | ||
| * @template T | ||
| * @psalm-param T $value |
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.
should be$VALUE to be replaced by the appropriate name.
| /** | ||
| * @template T | ||
| * @psalm-param T $value |
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.
shouldn't these all be just@param?
wouterj commentedJul 30, 2022
I've re-submitted this PR against the 6.2 branch:#47127 As the config builder class has had a refactor, creating a new PR was easier :) |
…wouterj)This PR was merged into the 6.2 branch.Discussion----------[Config] Add conditional types to conditional builders| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets |Fix#46546| License | MIT| Doc PR | n/aReplaces#46581 , submitted to 6.2 instead. This matches with the changes proposed in#47016 and the config builder contained quite a big refactor for comments in 6.x so this avoids nasty conflicts.Original description:> One of the main benefits of using the new PHP config builders in application code is the ability to use PHP community tooling to autocomplete and validate the configuration. In this light, I think it makes sense to add the correct PHPdocs to make Psalm and PHPstan understand the config builders.>> On top of that, as these config classes are live generated, this is not something that can be easily stubbed in the special PHPstan and Psalm Symfony plugins.Commits-------ce1087e [Config] Add conditional types to conditional builders
Uh oh!
There was an error while loading.Please reload this page.
On second thought, one of the main benefits of using the new PHP config builders in application code is the ability to use PHP community tooling to autocomplete and validate the configuration. In this light, I think it makes sense to add the correct PHPdocs to make Psalm and PHPstan understand the config builders.
On top of that, as these config classes are live generated, this is not something that can be easily stubbed in the special PHPstan and Psalm Symfony plugins.