-
Notifications
You must be signed in to change notification settings - Fork 6
Adds all the enums #25
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
Conversation
…d names - Constants must follow UPPER_SNAKE_CASE pattern - Only string and int values are allowed - Ensures enum integrity at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JasonTheAdams This looks great so far, thank you for putting it together!
I saw in #20 that it says to add the Implementor enums, but I wasn't sure which those were — perhaps the ones not in the
Providers
domain?
Exactly, the idea is that everything related to the extender API only is in the Providers
namespace, and everything else is considered part of the implementer API.
I wasn't sure where to put the
AbstractEnum
class, so I made aCommon
domain that's strictly for things like this.
That sounds perfect to me 👍
One thing the Claude did when generating this that I thought was interesting was go the extra mile in making these truly act like 8.1 enums is by being strictly equivocal with its instances.
I love it! It does feel like a hack, and singleton has several problems generally, but this is a perfect example of where the general best practice does not apply. This is a really creative and pragmatic solution, which I don't see any problems with, since it's stateless enum classes anyway.
One higher-level question I have (which relates to the last point and overall implementation of this PR) is: Do we want to mandate use of the enum classes for implementers and extenders (they would use e.g. ProviderTypeEnum::fromCloud()
)? Or do we want to encourage them to use the raw values and handle enum safety under the hood (they would use e.g. ProviderTypeEnum::CLOUD
)?
src/Common/AbstractEnum.php
Outdated
/** | ||
* Create an enum instance from a value, throws exception if invalid | ||
* | ||
* @param string|int $value The enum value | ||
* @return static | ||
* @throws InvalidArgumentException If the value is not valid | ||
*/ | ||
public static function from($value): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a few thoughts here regarding documentation, which apply to the entire PR. Let's discuss them here for how we want to proceed.
While we follow PSR coding standards, to my knowledge they don't specify requirements for documentation. However, the WordPress Coding Standards do. I'm not saying we need to follow the WordPress Coding Standards for documentation, but I think we should discuss each of the requirements and whether we want to embrace them for this project or not. It's crucial that we define our documentation requirements for this project early on so that code added adheres to them without later requiring tedious refactors.
Here are the points to consider:
- WordPress requires a description for the
@return
tag. I think this is just as crucial and helpful as it is to have description for@param
tags, so I think we should require that here too. - WordPress requires a
@since
annotation for almost everything (e.g. methods, class properties, classes, interfaces, traits). I personally think this is really useful as it allows to know at a glance when a certain thing was added. So I would suggest to require that for this project too.- How we would handle it is a related question. What we've done in other projects (e.g.
WordPress/performance
) is to always put@since n.e.x.t
on anything newly added, so that it can later be replaced with the concrete version as part of release preparation, as at that point the release number will be clear.
- How we would handle it is a related question. What we've done in other projects (e.g.
- WordPress requires multiple
@param
tags for a single method to have its individual parts (type, name, description) indented equally. I personally like that for readability, but obviously a matter of preferene. We do need to decide for yes or no though. - WordPress requires pretty much any description to end in a period / full stop. I'm personally very used to that, so I prefer it, but again a matter of preference. Yes or no?
- WordPress requires method and function descriptions to start with a third-person verb (e.g. "Creates" instead of "Create"). Again a matter of preference. Yes or no?
Please let me know your thoughts. Whatever we decide for all of these points we should document somewhere in docs
or CONTRIBUTING.md
, related to the coding standards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it comes to conventions such as @since n.e.x.t
, I'm perfectly happy adding that to the CONTRIBUTING doc. I think requiring things like docs for @return
also fit there.
For things like spacing the @param
, this is a direct deviation from the PSR standard, which means we'd actually be fighting any tools that are trying to adhere to the standard. For these I think we'd be working against ourselves, even though I agree that the indenting looks nice.
In short, for things that PSR-12/PER don't care about (e.g. full stops at the ends of comments and @since
conventions), I think it's worth documenting and doing. But I'd avoid contradicting the standards themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. Let's go with what you're saying - I wasn't aware that indenting actively contradicts PSR.
In other words, we'll stick to the WordPress conventions for documentation, except where they conflict with PSR. That's probably the most intuitive way to summarize it.
So should I remove the enums in
Right?? I actually really like it! It's impressive how close we can actually get this to be like 8.1 enums!
My inclination would be that in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JasonTheAdams Looks close! I think the missing pieces to update are to align with the documentation based on what we're discussing in #25 (comment) and add other popular AI integrations to .gitignore
per #25 (comment).
So should I remove the enums in
Providers
in this PR? Or let them be for now?
I think let's leave them in. Wasn't originally in the scope of this PR, but you already did the work, so we can be pragmatic about it - the code is very much similar to the other enums anyway :)
One higher-level question I have (which relates to the last point and overall implementation of this PR) is: Do we want to mandate use of the enum classes for implementers and extenders (they would use e.g.
ProviderTypeEnum::fromCloud()
)? Or do we want to encourage them to use the raw values and handle enum safety under the hood (they would use e.g.ProviderTypeEnum::CLOUD
)?My inclination would be that in the
Providers
namespace we require use of the enums. For the Implementor APIs we allow either the enum or the underlying value. I've done that pattern in the past and like it. It allows for the more advanced devs to have clearly coupled code and simpler use cases to just go the primitives route.
That sounds great to me, let's do that.
Could you maybe also add the points about documentation requirements and enum usage (per the above) to existing docs? Or, if you prefer, feel free to open a separate PR for that.
Back to you, @felixarntz! I resolved all the feedback and made #29 to add the documentation standards to the docs. I'll add the enum stuff to the docs in another PR. |
@JasonTheAdams Reviewing now... I think the point in #25 (comment) is still missing in the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost good to go, just a few last nit-picks. Can you please address these, and more importantly #25 (comment)?
After that, consider it approved.
Co-authored-by: Felix Arntz <[email protected]>
Co-authored-by: Felix Arntz <[email protected]>
This adds all the enums within the architecture. I saw in #20 that it says to add the Implementor enums, but I wasn't sure which those were — perhaps the ones not in the
Providers
domain? Otherwise, they're all here!I wasn't sure where to put the
AbstractEnum
class, so I made aCommon
domain that's strictly for things like this. Open to other ideas! Otherwise, I tried to make theAbstractEnum
work like the PHP 8.1 enum type as closely as possible, as I felt that would make things feel more consistent and easier to upgrade in the future. Similarities:$enum->value
to get the underlying value$enum->name
to get the name of the enum (i.e. the constant name)MyEnum::from()
make an enum, but get an exception if there's no matchMyEnum::tryFrom()
to make an enum, returns null if there's no matchThere are other non-standard methods because you can't use the comparison operator with these, and this has instances whereas enums technically do not. But I tried to make it similar and easy to make child classes of.
Singelton behvior?
One thing the Claude did when generating this that I thought was interesting was go the extra mile in making these truly act like 8.1 enums is by being strictly equivalent with its instances. That is, this is true:
It does this by caching instances based on the value inside a class static property. It's pretty clever. Since enums are immutable, it isn't possible to change one and have it cause issues elsewhere. Curious if we like this or if it's too voodoo.