search

rectorphp / swiss-knife

Swiss knife in pocket of every upgrade architect!
https://getrector.com
MIT License
72 stars 8 forks source link

[finalize-classes] Feature request: skip classes with an `@final` annotation #17

Closed gnutix closed 2 months ago

gnutix commented 4 months ago

I have a few classes that should not be inherited, but are sometimes mocked in tests. For these, I use a @final annotation, supported by PHPStan, so that no one can inherit them. It would be nice if this script would take that into account.

staabm commented 3 months ago

could you provide a failing test-case or maybe even a test and a fix?

gnutix commented 3 months ago

@staabm @TomasVotruba I added the missing unit tests in #20, along with a failing test for @final.

However, I'm not sure how to implement a fix properly. It would probably require a new dependency, likely to phpstan/phpdoc-parser ?

In terms of design, I guess \Rector\SwissKnife\PhpParser\NodeVisitor\NeedForFinalizeNodeVisitor is the class that should check if there's a @final annotation, but it does not have access to such a parser currently, and I'm not sure this dependency should be added, given there's already one in \Rector\SwissKnife\Analyzer\NeedsFinalizeAnalyzer.

I was also surprised to discover that \Rector\SwissKnife\Command\FinalizeClassesCommand has an option to skip mocked classes (which I would have gladly used), and that this logic is not implemented within NeedForFinalizeNodeVisitor, but within the CLI command itself. :shrug:

Anyway, I'm not very sure of the separation of concerns between those classes, so I'm curious of your inputs before going any further. Or if you want to take over the PR, feel free to do so.

TomasVotruba commented 2 months ago

Closing as this command should be strictly native-PHP based. Ref https://github.com/rectorphp/swiss-knife/pull/20#issuecomment-2100759996

Thanks for understanding