Skip to content

[Symfony53] Add CommandDescriptionToPropertyRector #245

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

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

JohJohan
Copy link
Contributor

See #229

@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 14, 2022

Thanks 👍

What Symfony version support the name and the description?
As I read the Symfony blog, it's 2 different versions, thus 2 different configs and rules.

@JohJohan
Copy link
Contributor Author

I think from 5.3 they support this: https://symfony.com/doc/5.3/console.html (but that is not maintained) so i would say 5.4/6.0 and 6.1

The blog post you probably found: https://symfony.com/blog/new-in-symfony-5-3-lazy-command-description

What do you mean by

thus 2 different configs and rules.

Should i use different configs to test this behavior? And is there some docs about that?

@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 14, 2022

Symfony 5.3 is for the $defaultDescription, yes 👍

The $defaultName is available since Symfony 3.4: symfony/symfony#23887


Should i use different configs to test this behavior? And is there some docs about that?

The docs can be found here: https://github.com/rectorphp/rector/tree/main/docs

But there is no description rule yet, AFAIK. So the new rule and tests are needed.

@JohJohan
Copy link
Contributor Author

Ah like that i can do that but takes a bit more time

final class SunshineCommand extends Command
{
protected static $defaultName = 'sunshine';
protected static $defaultDescription = 'sunshine description';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to force this order? Right now its adding the defaultDescription as first property.
So first $defaultName and then $defaultDescription

Copy link
Member

Choose a reason for hiding this comment

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

You'd have to look for existing properties on the Class_ and if there's a property of "defaultName", add a new property after it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added function addDefaultDescriptionProperty to do it, you may resolve this if it is okay

@samsonasik
Copy link
Member

You can run:

vendor/bin/rector && composer fix-cs 

To make CI green.

@JohJohan
Copy link
Contributor Author

Thanks @samsonasik i did and squashed my commits into 1 commit.

@samsonasik
Copy link
Member

String_ and ClassConstFetch check no longer needed, can be removed as already checked by ExprAnalyzer.

Also, please update PR Title to something like:

[Symfony53] Add CommandDescriptionToPropertyRector

@JohJohan JohJohan changed the title 229 Add fixtures to check description is correctly placed as property [Symfony53] Add CommandDescriptionToPropertyRector Sep 15, 2022
Add a rule to move command description setter to property
@JohJohan
Copy link
Contributor Author

Check, thanks for the feedback 👍

@TomasVotruba
Copy link
Member

Thank you 👏

@TomasVotruba TomasVotruba merged commit 113df7c into rectorphp:main Sep 19, 2022
@JohJohan JohJohan deleted the 229 branch October 2, 2022 07:38
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.

3 participants