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

Improve @psalm-internal and prevent usage of IssueBuffer::add(). #8165

Merged
merged 1 commit into from
Jun 25, 2022

Conversation

AndrolGenhald
Copy link
Collaborator

I finally got around to investigating why I'm getting ParseErrors from vendor files, so I decided to make sure stuff like that won't happen again.

  • Improve @psalm-internal
    • Allow multiple namespaces/identifiers
    • Allow specifying class names
    • Allow specifying functions and methods
  • Make IssueBuffer::add() internal so that it can't be used accidentally.

@AndrolGenhald AndrolGenhald requested a review from orklah June 25, 2022 07:14
@AndrolGenhald AndrolGenhald added enhancement release:feature The PR will be included in 'Features' section of the release notes and removed enhancement labels Jun 25, 2022
@AndrolGenhald
Copy link
Collaborator Author

I suppose it might be somewhat debatable whether these ParseErrors should show for vendor files or not, it's a mystery to me why composer is keeping symfony/process installed when it requires PHP >=8.1, but if it were actually used it would crash and Psalm wouldn't notice. On the other hand, it's entirely possible to have some implementations depend on PHP version and conditionally require them, so parsing errors that can never actually show up should definitely not be shown for vendor files.

@orklah
Copy link
Collaborator

orklah commented Jun 25, 2022

Looks fine :)

About vendors, I'd say the ideal behaviour would be to crash if we encounter a docblock(or signature) that contains wrong elements (at method level or on class level for templates for example). The rest of the errors are debatable...

The issue is that stubs can make a mess in the flag that says a symbol is from vendors or not (because even if you were able to say the symbol is originally from vendors, what about the symbols within the stubs. Are they vendors too?)

Let me know if it's ready to merge and I'll do it

@AndrolGenhald
Copy link
Collaborator Author

Go ahead 👍

@orklah orklah merged commit a4ab664 into vimeo:4.x Jun 25, 2022
@orklah
Copy link
Collaborator

orklah commented Jun 25, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants