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

add phpstan typechecked phpdocs #111

Closed
wants to merge 10 commits into from
Closed

Conversation

staabm
Copy link

@staabm staabm commented Oct 28, 2020

All Submissions:

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Adding some phpdocs to make it easier to contribute. the question came up in a recent PR and having this types defined makes it easier for humans and also tools to reason about the code.

#100 (comment)

@@ -25,6 +25,7 @@ public function meetsCriteria(Node $node): bool
/**
* @param Node&ClassConstFetch $node
* @return array<string>
* @phpstan-return array<class-string>
*/
public function extractNamespaces(Node $node): array
Copy link
Author

Choose a reason for hiding this comment

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

I am wondering whether these methods should read $class->toCodeString() as it seems toString will not always return a proper FQCN, per phpdocs in

https://github.com/nikic/PHP-Parser/blob/1d1bc8a36459d67acef508b952dad097960b110e/lib/PhpParser/Node/Name.php#L87-L105

Copy link
Member

Choose a reason for hiding this comment

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

Hm but as it states toCodeString just used toString
Question is: Is this overridden in some specific types?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it is changed depending on Relative or FullyQualified nodes.
That would prepend \\ or namespace\. Which I don't know if it's good.

But it could be a possibility for resolving the problem @OskarStark and @localheinz faced in #100 where
"Relative" classes were not recognized correctly.

@staabm staabm closed this Dec 30, 2020
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.

None yet

2 participants