Skip to content
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

ignore UnnecessaryVarAnnotation #6242

Merged
merged 4 commits into from Aug 5, 2021
Merged

ignore UnnecessaryVarAnnotation #6242

merged 4 commits into from Aug 5, 2021

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Aug 4, 2021

This PR re-fix #3573
The original fix partially fixed the issue

@jkowalleck
Copy link

jkowalleck commented Aug 4, 2021

the #3573 has been fixed before but the fix was removed.

@orklah could you add a regression test, if possible, so the fix does not get removed again?

i tested my code regarding #3573 without your fix of psalm, and it reproduced #3573.
Then i tested my code with your fixed version of psalm and #3573 was gone.

@orklah
Copy link
Collaborator Author

orklah commented Aug 4, 2021

I don't think the fix was removed. It was just two different places and only one place was fixed before

@orklah
Copy link
Collaborator Author

orklah commented Aug 4, 2021

I'll check what's going on. Psalm considers the suppression as not used, it's weird

@weirdan
Copy link
Collaborator

weirdan commented Aug 4, 2021

@orklah if you manage to fix the unused suppression, mind checking if it also fixes those issues below?

https://github.com/vimeo/psalm/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+unusedpsalmsuppress

@orklah orklah marked this pull request as draft August 5, 2021 18:54
tests/AnnotationTest.php Outdated Show resolved Hide resolved
@orklah
Copy link
Collaborator Author

orklah commented Aug 5, 2021

I think the build should be green now. The issue with the weird UnusedSuppress was because psalm's tests don't enable find_unused_variables by default. This flag is required to emit UnnecessaryVarAnnotation in the first place so the suppression was useless without this flag. Seems unrelated to other issues but I have a better grasp of suppressions now, I may take some time to check those

@weirdan
Copy link
Collaborator

weirdan commented Aug 5, 2021

I think the build should be green now

You were in for a surprise then 🤣

@orklah
Copy link
Collaborator Author

orklah commented Aug 5, 2021

You were in for a surprise then 🤣

🤣 I launched phpunit on my machine but only fixed the failures, not the errors. I think I've just worn out my _ key by adding it to all those unused vars

EDIT: Yes! I did it! At last!

@orklah orklah marked this pull request as ready for review August 5, 2021 20:14
@orklah
Copy link
Collaborator Author

orklah commented Aug 5, 2021

should be ok now :)

@weirdan weirdan merged commit 3c8842f into vimeo:master Aug 5, 2021
@weirdan
Copy link
Collaborator

weirdan commented Aug 5, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot suppress UnnecessaryVarAnnotation
3 participants