-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
Inheritance issues with Implement and Extend #169
Comments
Hi @benr77 you are right, I created a little test about the behavior and we need to check inside extend class other dependencies. Broken test to validate the expected behavior:
|
Hi @benr77, we have discussed this bug / new feature and we think that at the moment we would like to avoid implementing a parser that finds all dependencies inherited by other classes because this can open a lot of scenarios that can be difficult to manage (example vendors), and performance problems. What do you think? |
Hey folks! thanks for your amazing work! Recently, we've introduced phparkitect in Sylius - Sylius/Sylius#13426. It worked really nice. After few days, I've noticed, that we had one class out of namespace scope, so I fixed it and try to protect us in the future with creation of a new rule - Sylius/Sylius@7772229 What I would like to achieve, is to be sure that everything that implements |
Hi @lchrusciel we have discussed it many times and at the moment we are checking only the single class without building the full tree of inheritance. Checking recursively through inherited classes increases complexity but also the execution time of the tool. |
I totally understand your performance concerns and I get your point. Perhaps, I will find time to check the potential for this change on my project. For now, I'm ok with the current expectation even tho it is not what I aimed to achieve in the first place. Perhaps this tool is not mature enough for us to use it on Sylius or perhaps we are just use it wrongly. Either way, keep up good work, I bet many people still benefit from it :) |
Hi @lchrusciel we are working on this PR #239 to solve your problem by getting dependencies recursively. At the moment I tried to add in local into Sylius this rule:
Is this rule what you expect from the tool? |
I'm grateful for your efforts. As I see PRs it is really tough work. thanks a lot 👍 |
@lchrusciel it's a delicate thing the recursive search, for this reason I am thinking to merge this PR but with a different method to give to users two options:
What do you think? |
From my perspective, we are still using your tool and plan to use it even more (we've scheduled some modularity work). In this context (so assignment of class to given bounded contexts and not leaking to others) it works great. Thanks for that! Perhaps, I was just overshooting with my expectations. I still think that it would be great to ensure that classes in a given folder/namespace fulfil some interface expectations, but maybe it is too much for now |
Bug Report
Summary
Do the
Extend
andImplement
classes handle multiple levels of inheritance?E.g. I have this line, to pick up Symfony form types:
However, it's ignoring all these classes, because they extend
AbstractType
which in turn implementsFormTypeInterface
Expected behavior
I would expect the check to consider interfaces implemented, or classes extended from, all the way up the inheritance tree.
Thanks
The text was updated successfully, but these errors were encountered: