Skip to content
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

[Core] Remove RectifiedNode value object for RectifiedAnalyzer #3079

Merged
merged 29 commits into from Nov 20, 2022

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Nov 19, 2022

With consecutive last passed Rector rule correctly handled at PR:

The RectifiedNode value object that previously used to save last previously run Rector rule no longer needed, so it can be removed
https://github.com/rectorphp/rector-src/pull/3079/files#diff-5c5469b5b3ba8256b58b9d9bf66fe5873b03597965174036968f5b0d1513a565

It require verify phpstan_cache_printer attribute exists to decorate attribute CREATED_BY_RULE when refactor() call returns null.
It require connect refactored Node with existing Nodes for previous and next node.

By this, we finally get rid of unnecessary save Node and Rector rule into object as it already in CREATED_BY_RULE 🎉 🎉 🎉

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik
Copy link
Member Author

Rebased.

@samsonasik
Copy link
Member Author

PHPStan notice is unrelated;

Run vendor/bin/phpstan
Note: Using configuration file /home/runner/work/rector-src/rector-src/phpstan.neon.
   0/394 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   0%
 394/394 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

Error: Ignored error pattern #Parameter \#1 \$class of method Rector\\BetterPhpDocParser\\PhpDocInfo\\PhpDocInfo<PHPStan\\PhpDocParser\\Ast\\Node\>\:\:(.*?)\(\) expects class\-string, string given# was not matched in reported errors.
 -- --------------------------------------------------------------------------- 
     Error                                                                      
 -- --------------------------------------------------------------------------- 
     Ignored error pattern #Parameter \#1 \$class of method                     
     Rector\\BetterPhpDocParser\\PhpDocInfo\\PhpDocInfo<PHPStan\\PhpDocParser\  
     \Ast\\Node>\:\:(.*?)\(\) expects class\-string, string given# was not      
     matched in reported errors.      

@samsonasik
Copy link
Member Author

Ok, the PHPStan notice seems can be ignored.

@samsonasik
Copy link
Member Author

It seems the PHPStan notice in rector-symfony:

➜  rector-symfony git:(main) composer phpstan
> vendor/bin/phpstan analyse --ansi --error-format symplify

 ! [NOTE] The Xdebug PHP extension is active, but "--xdebug" is not used. This may  
 !        slow down performance and the process will not halt at breakpoints.       

Note: Using configuration file /Users/samsonasik/www/rector-symfony/phpstan.neon.
 394/394 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                    
 [WARNING] Ignored error pattern #Parameter \#1 \$class of method                   
           Rector\\BetterPhpDocParser\\PhpDocInfo\\PhpDocInfo<PHPStan\\PhpDocParser\</>
           Ast\\Node>\:\:(.*?)\(\) expects class\-string, string given# was not     
           matched in reported errors.                                              
                                                                                    

Script vendor/bin/phpstan analyse --ansi --error-format symplify handling the phpstan event returned with error code 1

@samsonasik
Copy link
Member Author

PHPStan notice fixed at rectorphp/rector-symfony#277

@samsonasik
Copy link
Member Author

Restart build.

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@samsonasik
Copy link
Member Author

I found that phpstan_cache_printer check not really needed, only verify against Expr is enough 👍 099c914

@samsonasik
Copy link
Member Author

samsonasik commented Nov 20, 2022

@TomasVotruba Finally 🎉 🎉 🎉 , I found the real solution, after refactor(), the refactored Node need to immediatelly connected with its existing Node previous and next node. 5cbe7a9

@samsonasik
Copy link
Member Author

All checks have passed 🎉 🎉 🎉 @TomasVotruba I think it is ready 🎉 🎉 🎉

@samsonasik
Copy link
Member Author

I added test to prove that existing addNodeBeforeNode() on a rule usage keep working 👍 a165e83

@samsonasik samsonasik marked this pull request as draft November 20, 2022 07:53
@samsonasik samsonasik marked this pull request as ready for review November 20, 2022 07:57
@samsonasik
Copy link
Member Author

samsonasik commented Nov 20, 2022

I also added test fixture to prove addNodeAfterNode() on Next Stmt when there is already another stmt already, which the next stmt will be inserted between node and existing next node 9561e66 👍

@samsonasik
Copy link
Member Author

All checks have passed 🎉 🎉 🎉 @TomasVotruba I think it is ready 🎉 🎉 🎉

@TomasVotruba
Copy link
Member

Thank you 👍😊 It looks like a lot of effort, great job 👌

Feel free to merge when ready 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants