[Config] Add conditional types to conditional builders #46581
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wouterj
commented
Jun 4, 2022
Comment on lines
147
to
148
| * @psalm-param T $value | ||
| * @psalm-return (T is array ? CLASS : static) |
There was a problem hiding this comment.
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.
let's still move this after @return in all cases :)
There was a problem hiding this comment.
Let's just say I was a bit unfocused yesterday 😄
nicolas-grekas
approved these changes
Jun 9, 2022
src/Symfony/Component/Config/Builder/ConfigBuilderGenerator.php
Outdated
Show resolved
Hide resolved
fbf15a8 to
e69e509
Compare
This allows static analysis tools to understand what type is returned.
e69e509 to
d8500c1
Compare
stof
requested changes
Jun 10, 2022
| @@ -285,7 +293,11 @@ public function NAME(array $value = []): CLASS | |||
| } else { | |||
| $body = $hasNormalizationClosures ? ' | |||
| /** | |||
| * @template T | |||
| * @psalm-param T $value | |||
There was a problem hiding this comment.
should be $VALUE to be replaced by the appropriate name.
| @@ -85,7 +93,11 @@ public function listObject($value = []) | |||
| } | |||
|
|
|||
| /** | |||
| * @template T | |||
| * @psalm-param T $value | |||
There was a problem hiding this comment.
shouldn't these all be just @param?
|
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 :) |
nicolas-grekas
added a commit
that referenced
this pull request
Aug 1, 2022
…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/a Replaces #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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.