mamuz / PhpDependencyAnalysis

Static code analysis to find violations in a dependency graph
http://mamuz.github.io/PhpDependencyAnalysis/
MIT License
561 stars 45 forks source link

Add type renamer visitor with example #9

Closed garex closed 8 years ago

garex commented 8 years ago

Valuable when you have very long namespaced names like "Fully\Qualified\Class\Name\To\NamespaceFilter" and you want to see them on diagrams like "Something\Short"

With example added.

mamuz commented 8 years ago

Hi @garex,

this feature already exists, see https://github.com/mamuz/PhpDependencyAnalysis/wiki/5.-Plugins#write-your-own-namespace-filter and.. https://github.com/mamuz/PhpDependencyAnalysis/wiki/3.-Configuration#available-options

garex commented 8 years ago

Hmm... Then it's described bad. I think it should be already implemented and be in complect.

mamuz commented 8 years ago

Modifying FQCN before namespace aggregation can lead to wrong results

garex commented 8 years ago

@mamuz Where to better make it? May be it should be implemented in another place? Like some filter before diagramming?

Also as I understand correctly, when we add this filter as a last visitor — it applies last so there could not be wrong results at all. In which situation it's possible?

mamuz commented 8 years ago

Each visitor is filtering the FQCN after collecting a node. Filtering is used for the aggregation. When the visitor modifies the namespace in the wrong way, then the aggregation produces wrong results.

see: https://github.com/mamuz/PhpDependencyAnalysis/blob/master/src/Parser/Visitor/AbstractVisitor.php#L127

Thats why the manipulation of FQCN have to be implemented here: https://github.com/mamuz/PhpDependencyAnalysis/blob/master/src/Parser/Filter/NodeName.php#L98

garex commented 8 years ago

So do you think it's good to have something inbuilt here with easy configuration?

I dont' think my current implementation is good. I can reimplement it as an NamespaceFilterImpl and update the docs.

mamuz commented 8 years ago

The value of having a dedicated class for that is, you can filter in any way. One way can be using regular expressions another way can be mapping with or without configuration.

So I think, a builtin implementation with easy configuration does not meet all customer needs. Trying to meet all conceivable requirements leads to overengineering.

Another option can be to create an own repository for plug-ins only. There you can provide several filters and visitors for dedicated use cases.

garex commented 8 years ago

@mamuz So why do you have then all those implementations at https://github.com/mamuz/PhpDependencyAnalysis/tree/master/src/Parser/Visitor ? Let's move em to external repo!

One strong reason to have built-in easy configured solution is that currently docs and examples are not obvious. "Write your own Namespace Filter" not leads to a way of "Modifing Long Classes Names".

Also where does another future lazy user will find those external-hosted addons? In google? Are you serious? ))

mamuz commented 8 years ago

According to moving the other optional visitors to another repo: :+1:

Improving the docs: :+1:

Finding addons: just add a new entry to composer suggest section.

garex commented 8 years ago

According to moving the other optional visitors to another repo: :+1:

No!!! I understand the way you suggest but strongly disagree. Why do you force lib user to think about extenal dependencies, read suggestions...

Regarding reading suggestions. Do you know how many it could have in typical complex project? Tons of! So I propose to leave them inside. IF you still thinks it would be better to move to another repo — then pls extract those addons in another repo and I will send PR with more correctly implemented version there.

mamuz commented 8 years ago

You are right. Actually it was not planned, but I like to separate things cause of complexity and all the other "*bilities".

garex commented 8 years ago

So pls let me know when you will extract separate repo. In the meantime I can reimplement more adequate version of this renamer. Let there be config with two modes: simple str_replace and preg_replace. I will send another PR to this repo to review. When you will approve implementaion && repo will be ready — we will move it there.

mamuz commented 8 years ago

That's cool. So feel free to make pr's to this repo regardless to split or not to split. Thanks for your feedback.