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.

Sign up for GitHub

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

Jump to bottom

Enhancement: Implement InstanceofStrategy #100

Merged
merged 1 commit into from Oct 28, 2020
Merged

Enhancement: Implement InstanceofStrategy #100

merged 1 commit into from Oct 28, 2020

Conversation

localheinz
Copy link
Contributor

@localheinz localheinz commented Oct 16, 2020 •
edited

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?

@OskarStark and I ran into an issue in a project that requires psr/http-message, and where we use

use Psr/Http;

if ($response instanceof Http\Message\ResponseInterface) {
}

However, running

$ composer-unused

results in psr/http-message not being used, so I figured I take a look.

Does that make sense?

@localheinz localheinz force-pushed the feature/instanceof branch 4 times, most recently from d853835 to d8350d8 Compare October 16, 2020 07:35
@localheinz localheinz force-pushed the feature/instanceof branch 2 times, most recently from fb53040 to 3955714 Compare October 16, 2020 07:50
localheinz
localheinz commented Oct 16, 2020
View reviewed changes
src/Parser/PHP/Strategy/InstanceofStrategy.php
$class = $node->class;

return [
$class->toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this method return an array of class names or an array of namespace names?

Copy link
Member

Choose a reason for hiding this comment

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

This should be the FQN of the class.
The method name is changed in #89 to extractSymbolNames which is more fitting.

Copy link

Choose a reason for hiding this comment

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

based on this question I am trying to improve phpdocs to make these point clear:

#111

@localheinz localheinz force-pushed the feature/instanceof branch 4 times, most recently from d39533e to 3e0f239 Compare October 16, 2020 08:02
@icanhazstring
Copy link
Member

Does that make sense?

For me absolutely 👍

@OskarStark
Copy link
Contributor

Does that make sense?

Same for me 👍

localheinz
localheinz commented Oct 16, 2020
View reviewed changes
tests/Integration/Parser/PHP/NodeVisitorTest.php Outdated
],
'Instanceof' => [
'expectedUseNamespaces' => [
'Bar\Baz',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the tests pass, but this should be Foo\Bar\Baz as well.

Do you have an idea how to resolve the absolute class name from the relative class name Bar\Baz, @icanhazstring?

Copy link
Member

Choose a reason for hiding this comment

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

I did something similar in #89.
https://github.com/composer-unused/composer-unused/pull/89/files#diff-0e5fd3b0f38e3517fafece6f83d2ef2b5af2256f267e53a53db137a2746856b3R29

Basically, you enter the file, save the namespace, and go further in the AST.
I think you can check for isFullyQualified on the $node and prepend the namespace.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see what you are doing.
Maybe you even found a bug that is pretty old ;)

I think the problem is that "relative" symbols are not parsed correctly.
As they are not combined with a partial import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I comment this case out for now, then?

Copy link
Member

Choose a reason for hiding this comment

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

Shall I comment this case out for now, then?

Yes, and if you don't mind you could then also create a new issue for this, so maybe I can give it a try next week :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, commenting out passes this tests, but does not solve the problem @OskarStark and I have.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. Maybe we should let this PR open to support instanceof recognition and try to fix the underlying bug with partial imports respectively relative class names.

After this is fixed, your test should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, commenting out passes this tests, but does not solve the problem @OskarStark and I have.

I mean I can for now use the FQCN instead of a partial one 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Either way, it should be fixed 😉
But if that solves your problem, for now, I would merge this PR and see If we can fix this afterward.

@localheinz localheinz force-pushed the feature/instanceof branch 3 times, most recently from cc57eea to 91f1cf0 Compare October 16, 2020 08:31
@localheinz localheinz marked this pull request as ready for review October 16, 2020 08:31
@icanhazstring icanhazstring merged commit 8e3770e into composer-unused:main Oct 28, 2020
@localheinz localheinz deleted the feature/instanceof branch October 28, 2020 11:38
@localheinz
Copy link
Contributor Author

Thank you, @icanhazstring!

@icanhazstring
Copy link
Member

Merged and tagged with 0.7.5. Thanks 👍

@staabm staabm mentioned this pull request Oct 28, 2020
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staabm staabm staabm left review comments

@icanhazstring icanhazstring icanhazstring left review comments

@OskarStark OskarStark OskarStark left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@localheinz @icanhazstring @OskarStark @staabm