Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Narrow existing return types on private/internal/final/test methods#42213
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
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedJul 21, 2021
(psalm issues are false positives, with a slight code improvement inef84e66) |
src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/AnnotationsCacheWarmerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
f08a602 to97e8f39Comparenicolas-grekas commentedAug 3, 2021
PR is ready @symfony/mergers |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
e302e94 tob2de6c3Comparenicolas-grekas commentedAug 9, 2021
PR rebased and ready. |
Uh oh!
There was an error while loading.Please reload this page.
b2de6c3 to4ff47dfCompare4ff47df tod67a927Compare
chalasr left a comment
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.
LGTM. I need to take a nap now 😅
This PR was merged into the 6.0 branch.Discussion----------Add back ``@return` $this` annotations| Q | A| ------------- | ---| Branch? | 6.0| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -In#42213 I removed those annotations and tried to convinced myself and others that we could do so without loosing too much.I was about to send a PR to remove all remaining similar annotations. When I reviewed the patch I created for that, I stopped on `ItemInterface::tag()` (which is just an example):https://github.com/symfony/symfony/blob/444d43fa11990092c53ba54849bb1df36225adcf/src/Symfony/Contracts/Cache/ItemInterface.php#L52-L57If we remove that annotation on the interface, all implementations will have to deal with the return value of the call to `tag()`. Whereas with ``@return` $this`, the contracts are clear: no need to, implementations are expected to mutate and return the current object. I don't think this would be appropriate in this example./cc `@nikic` as this might be some nice food for thoughts to consider adding `$this` as a possible native return type.Commits-------7fafe83 Add back ``@return` $this` annotations
More progress from#42149