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

[PHP 8.1] Add basic support for Spatie Enums to native Enums #653

Merged
merged 8 commits into from
Aug 12, 2021

Conversation

mallardduck
Copy link
Contributor

Per the title, this add support for the most basic usage of Spatie Enums.

This does not cover usages of Spatie Enums where a values() method is use to override the automatic values. However it does (from my understanding) mimic the experience one would expect with only using the DocBlocks to define enum cases for their enums.

It also doesn't bother to remove the import of the spatie Enums since that seems to match what the MyCLabsClass version of the rector.

@Gummibeer
Copy link

Awesome! 🥳
Thanks for taking care Dan! 🙏

composer.json Outdated Show resolved Hide resolved
@TomasVotruba
Copy link
Member

Thank you for nice and clean code. It's joy to read ☺️
One little detail and it's ready to go

Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

I think the Enum suffix should be kept to avoid manual file rename.

@mallardduck
Copy link
Contributor Author

@samsonasik - I suppose that's a fair view of things.
People will just have to de-suffix and rename it manually.

Well unless there's a way to have rector rename the files too. Then maybe we just do that. 🤔

@mallardduck
Copy link
Contributor Author

@TomasVotruba - Am I mistaken or would the UpdateFileNameByClassNameFileSystemRector rector potentially cover correcting the filename to match? I'm guessing it depends on if the enum is considered a ClassLike node?

@samsonasik
Copy link
Member

samsonasik commented Aug 12, 2021

The EnumFactory used by other rule: MyCLabsClassToEnumRector which kept the Enum suffix, the functionality should be same for consistency.

You may need to use Renaming and Restoration rules to rename class and file.

@mallardduck
Copy link
Contributor Author

mallardduck commented Aug 12, 2021

@samsonasik - I disagree that this rector needs to match how MyCLabs rector works. These are both distinct and opinionated packages providing Enum features before native Enums. As such I think matching the individual packages convention will make sense.

One of the creators/maintainers of the Spatie package @brendt wrote an article about how they will be updating their Spatie Enums to native Enums. And that included the removal of redundant *Enum suffix on the enum - makes perfect sense since we don't suffix our classes as *Class.

EDIT: Link to Article: https://stitcher.io/blog/php-enums

@mallardduck
Copy link
Contributor Author

I guess put a different way, what's the Rector philosophy on providing rectors of varying levels of aggressiveness?

Personally I want a rule that will update all the things for me and I don't like redundant suffixes. So having one rector that gets me part the way there doesn't feel like a solution to me. At that point I'd just do this all manually since it will still require me to rename the enum and file.

TomasVotruba and others added 3 commits August 12, 2021 16:23
Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
…xture/some_class.php.inc

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
@TomasVotruba TomasVotruba changed the title Add basic support for Spatie Enums [PHP 8.1] Add basic support for Spatie Enums to native Enums Aug 12, 2021
@samsonasik
Copy link
Member

I think rename class name is not this rule's responsibility, you can use Renaming and Restoration rules for renaming class and file used along with this rule.

@TomasVotruba
Copy link
Member

I think rename class name is not this rule's responsibility, you can use Renaming and Restoration rules for renaming class and file used along with this rule.

👍 Agreed, the rule should be doing one job at a time.

Let's merge this as it is to keep it simple and iteratate improvements 🙂 Thank you

@mallardduck
Copy link
Contributor Author

the rule should be doing one job at a time.

Fair enough - that's why I was seeking to understand the philosophy here.

@samsonasik samsonasik enabled auto-merge (squash) August 12, 2021 14:35
@samsonasik samsonasik merged commit 48121fb into rectorphp:main Aug 12, 2021
@samsonasik
Copy link
Member

Thank you @mallardduck

@TomasVotruba
Copy link
Member

Fair enough - that's why I was seeking to understand the philosophy here.

Undrestood. We try to follow unix philosophy. The oldest rules try to do many things at once, but last 2 years we try to go as narrow as possible, while keeping the main idea together.

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.

4 participants