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

Normalize phpenum type #1375

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

shmax
Copy link
Contributor

@shmax shmax commented May 4, 2023

I'm starting to make heavier use of PhpEnumType, and its lack of configurability is starting to become an issue. Someone added support for a custom name recently, but I need the same support for 'description', and rather than start an optional argument chain I thought I'd see what you think about just doing the same config-based setup we do for everything else. In the spirit of brevity, I added a static fromPath helper that will allow that same one-liner feel (or we can ditch it). A backwards-compatible solution using a union type on the first argument may be possible, but I figure it might be best to just rip the band-aid off. Let me know what you think.

src/Type/Definition/PhpEnumType.php Outdated Show resolved Hide resolved
src/Type/Definition/PhpEnumType.php Outdated Show resolved Hide resolved
src/Type/Definition/PhpEnumType.php Outdated Show resolved Hide resolved
tests/Type/PhpEnumType/MyCustomPhpEnum.php Show resolved Hide resolved
tests/Type/PhpEnumTypeTest.php Outdated Show resolved Hide resolved
@shmax
Copy link
Contributor Author

shmax commented May 5, 2023

Thanks for the comments. 👍 I'm happy with this if you are.

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Looks good, I am happy to include this in the next major version. It will be a few months until then, if you really want this feature included in the library before then we can discuss ways to include it in a non-breaking way.

@shmax
Copy link
Contributor Author

shmax commented May 5, 2023

if you really want this feature included in the library before then

Nah, I don't think there's any rush; in the meantime, I can just continue to access the public description property.

I am curious to know how you might propose to preserve bc...

@shmax
Copy link
Contributor Author

shmax commented Oct 1, 2023

Merged latest. Still no rush, or anything, but it should be ready for next release.

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

Successfully merging this pull request may close these issues.

None yet

2 participants