Bugfix: List postrectors in the list of applied rules #7300
Conversation
simonschaufi
force-pushed
the
issue-9373
branch
from
f15c14d to
1d4f4d0
Compare
simonschaufi
force-pushed
the
issue-9373
branch
from
1d4f4d0 to
0340447
Compare
|
|
||
| // notify this rule changing code | ||
| $rectorWithLineChange = new RectorWithLineChange(self::class, $node->getStartLine()); | ||
| $this->file->addRectorClassWithLine($rectorWithLineChange); |
There was a problem hiding this comment.
add logic to verify that removeImportsFromStmts apply the change, if no change, don't add to the list.
There was a problem hiding this comment.
The challenge is that none of these post rectors have test coverage...
There was a problem hiding this comment.
You can create e2e test for it, add to
https://github.com/rectorphp/rector-src/tree/main/e2e
and register to
rector-src/.github/workflows/e2e.yaml
Lines 26 to 43 in df88764
There was a problem hiding this comment.
I don't even understand how the ClassRenamingPostRector supposed to work and also the UseAddingPostRector
There was a problem hiding this comment.
I will try create separate alternative PR :),
No need to close this PR for now, you may be faster than me so understand the flow early, and got the solution.
There was a problem hiding this comment.
Added an e2e test but I don't know how to make use of the UseAddingPostRector. Maybe you can contribute some code examples?
simonschaufi
force-pushed
the
issue-9373
branch
from
0340447 to
ae83860
Compare
| $removedUses = $this->renamedClassesDataCollector->getOldClasses(); | ||
| $node->stmts = $this->useImportsRemover->removeImportsFromStmts($node->stmts, $removedUses); | ||
| $stmts = $this->useImportsRemover->removeImportsFromStmts($node->stmts, $removedUses); | ||
| if ($stmts !== $node->stmts) { |
There was a problem hiding this comment.
objects collection can't be compared like this, use something like $hasChanged flag instead
There was a problem hiding this comment.
how would you do it as I can't access the $hasChanged within the UseImportsRemover
There was a problem hiding this comment.
removeImportsFromStmts can return bool, then return verify it, no need to reassign since objects always by reference.
There was a problem hiding this comment.
I found a cleaner solution. Please have a look.
simonschaufi
force-pushed
the
issue-9373
branch
2 times, most recently
from
7d1713c to
94e0a70
Compare
simonschaufi
force-pushed
the
issue-9373
branch
from
94e0a70 to
bf9e6b8
Compare
| if ($node instanceof FileWithoutNamespace || $node instanceof Namespace_) { | ||
| $removedUses = $this->renamedClassesDataCollector->getOldClasses(); | ||
| $node->stmts = $this->useImportsRemover->removeImportsFromStmts($node->stmts, $removedUses); | ||
| if ($removedUses !== []) { |
There was a problem hiding this comment.
It already never empty since checked on shouldTraverse method.
The $this->useImportsRemover->removeImportsFromStmts($node->stmts, $removedUses); can be updated to return bool, use flag like $hasChanged.
Yes, object is by reference, so you can verify if it has changed, no need to reassign on
This case.
There was a problem hiding this comment.
Can you please do this refactoring in a separate isolated PR so that I can then use it in my PR? The return value is an array and unsetting a value in an array is not done by reference afaik. I would appreciate your help here!
There was a problem hiding this comment.
I checked the code, this PR seems cause various warning on unit test
WARNING: On fixture file "conflict_aliased_with_docblock.php.inc" for test "Rector\Tests\Issues\AutoImport\AutoImportTest"
File not changed but some Rector rules applied:
* Rector\PostRector\Rector\ClassRenamingPostRector
.....
WARNING: On fixture file "skip_doctrine_aliased.php.inc" for test "Rector\Tests\Issues\AutoImport\AutoImportTest"
File not changed but some Rector rules applied:
* Rector\PostRector\Rector\ClassRenamingPostRector
see:
https://github.com/rectorphp/rector-src/actions/runs/17867691953/job/50813890782?pr=7300#step:5:20
There was a problem hiding this comment.
I will try create alternative fresh PR, since NameImporter may also needs refactoring as well.
There was a problem hiding this comment.
I create WIP new PR for it:
2 participants
Resolves: rectorphp/rector#9373