Skip to content

Conversation

@AlessandroMinoccheri
Copy link
Member

This PR should solve problem 1 of this issue #307 about aliases

@AlessandroMinoccheri AlessandroMinoccheri added the enhancement New feature or request label Jan 1, 2023
@AlessandroMinoccheri AlessandroMinoccheri added this to the v1 milestone Jan 1, 2023
@AlessandroMinoccheri AlessandroMinoccheri changed the title wip Parsing aliases in docblocks also Jan 1, 2023
@AlessandroMinoccheri AlessandroMinoccheri changed the title Parsing aliases in docblocks also [WIP] Parsing aliases in docblocks also Jan 2, 2023
@AlessandroMinoccheri AlessandroMinoccheri force-pushed the alias-docblock branch 2 times, most recently from 8308c7e to a0c0d41 Compare January 3, 2023 14:29
@AlessandroMinoccheri AlessandroMinoccheri changed the title [WIP] Parsing aliases in docblocks also Parsing aliases in docblocks also Jan 3, 2023
Copy link
Collaborator

@fain182 fain182 left a comment

Choose a reason for hiding this comment

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

I am not expert about this part of the code, i left a couple of minor fixes, but apart from that for me it's ok 👍


$violations = new Violations();

$notHaveDependencyOutsideNamespace = new DependsOnlyOnTheseNamespaces('MyProject\AppBundle\Application');
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable name is slightly wrong, but maybe we can test it more directly without using a rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

FIxed!

}

if (null === $node->type) {
if (null === $node->type || !($node->type instanceof FullyQualified)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the null === $node->type because is already in the second clause of the OR....

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed!

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants