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

feat(support): add enums support #878

Merged
merged 26 commits into from
Feb 28, 2025

Conversation

gturpin-dev
Copy link
Contributor

This PR follows the discussion on Discord https://discord.com/channels/1236153076688359495/1323681600768315472
It add enums support using various traits to enable useful features for dealing with enums

Copy link
Member

@innocenzi innocenzi left a comment

Choose a reason for hiding this comment

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

I allowed myself to review before it's ready because I like the idea of this PR.

I love some enum helper methods, but I think they are too many. Specifically, I don't think InvokableCases is useful, and I don't feel like Comparable::is and isNot are.

@brendt
Copy link
Member

brendt commented Jan 9, 2025

I like the idea, and many of the methods, but I think it should be one trait: IsEnum, or IsImprovedEnum or HasEnumMethods or something alike. I don't think splitting it into separate traits adds value.

@gturpin-dev
Copy link
Contributor Author

but I think they are too many. Specifically, I don't think InvokableCases is useful, and I don't feel like Comparable::is and isNot are.

My intend is to create a set of enums support that's firstly useful internally, but for end users, so I think we must provide some useful methods or DX even if we don't use them internally.
But this idea can be challenged, tell me :)

I like the idea, and many of the methods, but I think it should be one trait: IsEnum, or IsImprovedEnum or HasEnumMethods or something alike. I don't think splitting it into separate traits adds value.

I agree, but to support as many people as possible, I have split things in multiple traits if people don't want to import everything.
And for the most case if we want to import everything we have the Enumerates trait which import every traits, did you missed it ?

@brendt
Copy link
Member

brendt commented Jan 10, 2025

if people don't want to import everything.

I think it's an edge case we don't need to support :) It's better to be in line with StringHelper and ArrayHelper: one class/trait with lots of methods

@gturpin-dev
Copy link
Contributor Author

I think it's an edge case we don't need to support :) It's better to be in line with StringHelper and ArrayHelper: one class/trait with lots of methods

I'm ok to be straightforward with this, so should we stay with the name Enumerates ? Or do you prefer things you mentionned ?
Consider that StringHelper and ArrayHelper names are currently being challenged in https://discord.com/channels/1236153076688359495/1318231202166997062

@brendt
Copy link
Member

brendt commented Jan 17, 2025

I'm ok to be straightforward with this, so should we stay with the name Enumerates ? Or do you prefer things you mentionned ?

Good question. Our current naming strategy for traits involves prefixing with Is or Has. But I can't really think of a good name atm…

  • IsEnum
  • IsImprovedEnum
  • HasImprovedEnumMethods

IDK

@gturpin-dev
Copy link
Contributor Author

Same here, so I'll start by replacing Enumerates with IsEnum for now

@brendt
Copy link
Member

brendt commented Feb 22, 2025

Cool! I'm still undecided on whether it makes sense to have four traits which are combined into one. I'd like the simplicity of having everything in one trait, unless there's a very good reason not to.

@brendt
Copy link
Member

brendt commented Feb 22, 2025

Also: tests now seem to be fine after merging main :)

@innocenzi
Copy link
Member

I asked Claude because I'm not good at naming, and here's what he suggests:

  • Accessible -> HasCaseAccessUtils (I'd use just HasAccessUtils)
  • Comparable -> HasComparisonUtils
  • HelperMethods -> HasEnumerationUtils
  • InvokableCases (sorry 😉)
  • IsEnum -> HasEnumUtils, IsExtendedEnum, HasEnumExtensions

I do prefer the word "util" over "helper", but maybe for consistency we should go with what's already used in Tempest, so "helper".

@gturpin-dev
Copy link
Contributor Author

To be sure of what to do and what to decide @brendt @innocenzi

  1. Traits combination decision:
    Should we combine separate traits into one? ( both are ok for me )

    • Pro: Keeps simplicity
    • Con: Developers can't use individual enum features
  2. Naming conventions:
    The IsEnum name fits well with the rest of the codebase
    We currently have more Helpers than Utils even if I prefer Utils too
    But what if we rename ArrayHelper and StringHelper ( which is in discussion ) ?

  3. Apart from this, should I remove the InvokableCases part ?

@innocenzi
Copy link
Member

Traits combination decision

Let's let @brendt decide. imo both are fine but I understand the will to have just one trait.

The IsEnum name fits well with the rest of the codebase

It does but the name is too generic, it should be more specific

But what if we rename ArrayHelper and StringHelper (which is in discussion)?

Let's go with "helper" for consistency, if we rename that later we'll do it in bulk

Apart from this, should I remove the InvokableCases part?

I personally definitely think so!

@brendt
Copy link
Member

brendt commented Feb 26, 2025

Traits combination decision

Let's have one trait for now 👍

It does but the name is too generic, it should be more specific

IsEnumHelper? If possible, I would really like to keep our naming convention of Is or Has for traits. I think Helper makes sense, since we have it in other places as well. We could debate whether Helper should be renamed, but I think that's outside the scope of this PR. I'd say, let's go with IsEnumHelper, and have another debate after this PR on what we want our helpers to be named

Apart from this, should I remove the InvokableCases part?

Yes, let's remove

@gturpin-dev gturpin-dev marked this pull request as ready for review February 26, 2025 15:26
@brendt
Copy link
Member

brendt commented Feb 26, 2025

Ping me when ready btw :)

@gturpin-dev
Copy link
Contributor Author

@brendt then it's ready for me👌
Does @innocenzi wants to review something else ?

@innocenzi
Copy link
Member

LGTM 👍

@brendt
Copy link
Member

brendt commented Feb 28, 2025

Thanks for all the effort you put into this @gturpin-dev ! Let's merge :)

Would you be able to also send a PR to the docs about it? It could go on https://tempestphp.com/docs/framework/primitive-helpers/

@brendt brendt mentioned this pull request Feb 28, 2025
@brendt brendt merged commit 964d55a into tempestphp:main Feb 28, 2025
65 checks passed
@gturpin-dev
Copy link
Contributor Author

@brendt Sure, I'll check to make input there 🎉

Thanks for pointing out the locations 👌

@gturpin-dev gturpin-dev deleted the feat/support/enums branch February 28, 2025 14:59
@brendt
Copy link
Member

brendt commented Mar 1, 2025

I've added a docs entry for this :)

jonkerw85 pushed a commit to jonkerw85/tempest-framework that referenced this pull request Mar 19, 2025
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