Skip to content

[Symfony] Add MakeCommandLazyRector#1519

Merged
TomasVotruba merged 1 commit intomasterfrom
lazy-command
Jun 4, 2019
Merged

[Symfony] Add MakeCommandLazyRector#1519
TomasVotruba merged 1 commit intomasterfrom
lazy-command

Conversation

@TomasVotruba
Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba commented May 31, 2019

Closes #1497

What do you think @stloyd ?

Copy link
Copy Markdown
Contributor

@stloyd stloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great! Just some small changes needed :)

Comment thread packages/Symfony/src/Rector/Class_/MakeCommandLazyRector.php
Comment thread packages/Symfony/src/Rector/Class_/MakeCommandLazyRector.php Outdated
@TomasVotruba
Copy link
Copy Markdown
Member Author

I see. So just remove the argument?

@stloyd
Copy link
Copy Markdown
Contributor

stloyd commented Jun 1, 2019

If you find that call with parameter remove just the parameter and you will be safe ;)

@TomasVotruba
Copy link
Copy Markdown
Member Author

(@stloyd Sorry, I wanted to comment to reply you, but on mobile it dropped me somewhere else)

Copy link
Copy Markdown
Contributor

@stloyd stloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First tests are promising :)

Comment thread packages/Symfony/src/Rector/Class_/MakeCommandLazyRector.php
Comment thread packages/Symfony/src/Rector/Class_/MakeCommandLazyRector.php Outdated
@TomasVotruba TomasVotruba force-pushed the lazy-command branch 3 times, most recently from 5a31276 to 141eeb3 Compare June 3, 2019 16:22
@TomasVotruba
Copy link
Copy Markdown
Member Author

New update 👍

@stloyd
Copy link
Copy Markdown
Contributor

stloyd commented Jun 3, 2019

@TomasVotruba Works almost perfect! Just randomly (like one on 20/30) classes is not getting correct property being set:

325) src/Command/TestCommand.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -16,9 +16,7 @@
 {
     protected function configure()
     {
-        $this
-            ->setName('some:class:long-string-maybe-thats-the-reason-but-probably-not')
-            ->setDescription('do something')
+        $this->setDescription('do something')

Other than this random thing it looks you covered all our legacy cases 👍 🏆

@TomasVotruba
Copy link
Copy Markdown
Member Author

TomasVotruba commented Jun 3, 2019

@stloyd Thanks for deep testing! I think it will not be random.

Do you mean it removes the setName() method correctly but doesn't add the static property?

Could you get me the minimal anonymized PHP code that has this error so I can fix it?

Comment thread packages/Symfony/src/Rector/Class_/MakeCommandLazyRector.php
@stloyd
Copy link
Copy Markdown
Contributor

stloyd commented Jun 4, 2019

@TomasVotruba Just check latest version against our legacy codebase with over 400 (don't ask 🤓 ) commands being properly adjusted 😃 🚀

@TomasVotruba TomasVotruba changed the title [WIP] [Symfony] Add MakeCommandLazyRector [Symfony] Add MakeCommandLazyRector Jun 4, 2019
@TomasVotruba
Copy link
Copy Markdown
Member Author

TomasVotruba commented Jun 4, 2019

The more code the bigger amount of work Rector saves 👍

Thanks for testig and feedback. I wouldn't think about those edge cases myself, as I'm biased in single (my :D) approach.

@TomasVotruba TomasVotruba merged commit d8dbf06 into master Jun 4, 2019
@TomasVotruba TomasVotruba deleted the lazy-command branch June 4, 2019 11:47
@TomasVotruba
Copy link
Copy Markdown
Member Author

TomasVotruba commented Jun 4, 2019

Lets 🚢 it!

TomasVotruba added a commit that referenced this pull request Dec 18, 2021
rectorphp/rector-src@3ccc13f add test case with static on dynamic method (#1519)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Symfony Rector to make commands lazy

2 participants