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

bump psalm, fix some issues on level 6 and improve type coverage #8409

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

orklah
Copy link
Contributor

@orklah orklah commented Jan 3, 2021

This PR does the following:

  • bump psalm to last version
  • fix some issues on level 6
  • improve type coverage by adding types when needed

@VincentLanglet
Copy link
Contributor

Can you also fix this: psalm/psalm-plugin-doctrine#84 (comment) ?

@@ -230,7 +230,7 @@ class ClassMetadataInfo implements ClassMetadata
* hierarchy. If the entity is not part of a mapped inheritance hierarchy this is the same
* as {@link $name}.
*
* @var string
* @var class-string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @var class-string
* @var string
* @psalm-var class-string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for this? PHPDocumentor and PHPStorm both understand this notation now. Is there another tool we want compatibility with that doesn't support this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone with an older version of PHPStorm ? Someone with another IDE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greg0ire WDYT? Should we keep class-string in @psalm annotations? What's the policy for doctrine repos?

Copy link
Member

@greg0ire greg0ire Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Doctrine repositories we use @psalm annotations historically, because of PHPStorm, but we might want to relax that, I hear that indeed PHPStorm understands these now. I'll ask about this internally, but I have no strong opinion since I'm not a PHPStorm user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Maybe that's something that could be enforced via CS rules, one way or another

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing this, we are not yet ready to enforce it, but we do think we should no longer require contributors to add prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks!

I added the psalm prefix for now but I'll keep that in mind for next time :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, also, as pointed out internally by @ostrolucky, there are some tricky cases like cases involving template that should stay with prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. Also phpstorm doesn't yet read all annotations. Array shapes for example will be reported as an error, even more so with list (for example list<string, array{string, string}> will be a undefined class list)

@greg0ire greg0ire merged commit f0ad5f7 into doctrine:2.9.x Jan 8, 2021
@greg0ire
Copy link
Member

greg0ire commented Jan 8, 2021

Thanks @orklah !

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

Successfully merging this pull request may close these issues.

4 participants