swiss-knife icon indicating copy to clipboard operation
swiss-knife copied to clipboard

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

Open gnutix opened this issue 1 year ago • 2 comments

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.

gnutix avatar Mar 19 '24 06:03 gnutix

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

staabm avatar Apr 03 '24 15:04 staabm

@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.

gnutix avatar Apr 04 '24 08:04 gnutix

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

Thanks for understanding

TomasVotruba avatar May 08 '24 14:05 TomasVotruba