Elao / PhpEnums

:nut_and_bolt: Extended PHP 8.1+ enums features & specific integrations with frameworks and libraries
MIT License
326 stars 27 forks source link

make it easier to create enum from "dumb" enums from other libraries #93

Closed mathroc closed 4 years ago

mathroc commented 4 years ago

a lot of libraries have enum classes that are only contants bags (eg: https://github.com/Mangopay/mangopay2-php-sdk/blob/master/MangoPay/EventType.php or https://github.com/symfony/http-kernel/blob/master/KernelEvents.php)

I’d like to tranform those into Elao\Enum\EnumInterface but I can’t extend both the 3rd party class and Elao\Enum\Enum so I have to reimplement Elao\Enum\EnumInterface (by copying, the content of Elao\Enum\Enum).

It’d be nice if this was available as a trait:

class EventType extends \MangoPay\EventType implements EnumInterface
{
    use EnumTrait;
    use AutoDiscoveredValuesTrait;
}
ogizanagi commented 4 years ago

I don't think relying on extending a 3rd party "enum" class is a good idea for this. We could perhaps propose such a thing:

class EventType extends Enum
{
    private const INHERIT_CONSTS_FROM = \MangoPay\EventType::class;

    use AutoDiscoveredValuesTrait;
}

where the AutoDiscoveredValuesTrait would declare values from the public consts in \MangoPay\EventType. Then instantiating such an enum would be like:

EventType::get(\MangoPay\EventType:: KycCreated);

or we could imagine a command generating a proper enum class from a "dumb enum".

in practice, I did create such enums wrapping another third-party "dumb enum", but it was also expliciting the one values I cared about in my application, not just blindly inherit all. So in the end, I'm just not sure if it deserves a new feature.

mathroc commented 4 years ago

I don't think relying on extending a 3rd party "enum" class is a good idea for this. We could perhaps propose such a thing:

class EventType extends Enum
{
    private const INHERIT_CONSTS_FROM = \MangoPay\EventType::class;

    use AutoDiscoveredValuesTrait;
}

where the AutoDiscoveredValuesTrait would declare values from the public consts in \MangoPay\EventType. Then instantiating such an enum would be like:

EventType::get(\MangoPay\EventType:: KycCreated);

I don’t mind either way. It just seems more complicated with INHERIT_CONSTS_FROM to me and I don’t get what advantages it has

or we could imagine a command generating a proper enum class from a "dumb enum".

in practice, I did create such enums wrapping another third-party "dumb enum", but it was also expliciting the one values I cared about in my application, not just blindly inherit all. So in the end, I'm just not sure if it deserves a new feature.

That’s understandable. I just thought it could be an option because of how easy it should be to implement / maintain (just move the content of Elao\Enum\Enum into a trait and use that trait in back in Elao\Enum\Enum instead.

mathroc commented 4 years ago

another quite easy way to do that would be to add a getSourceClass() method to AutoDiscoveredValuesTrait. then it would only be a matter of overriding this method, something like:

<?php declare(strict_types=1);

namespace App\Infra\MangoPay;

use Elao\Enum\AutoDiscoveredValuesTrait;
use Elao\Enum\Enum;
use MangoPay\EventType;

class HookType extends Enum
{
    use AutoDiscoveredValuesTrait;

    public function getSourceClass(): string
    {
        return EventType::class;
    }
}

imho, It’s not as nice because it does not extend the source class but it could be enough

ogizanagi commented 4 years ago

@mathroc : I was doing the same for the ReadableEnum for consistency (pushed to your PR), but I realized traits might not really fulfill your feature request entirely. Often such "dumb" enums classes are declared finals (see KernelEvents for instance), so moving the code to traits won't help...

imho, It’s not as nice because it does not extend the source class but it could be enough

Why would you care about extending the source class actually?

mathroc commented 4 years ago

Why would you care about extending the source class actually?

I can’t remember where but I swear I saw an enum with helper methods on it.

but honestly it would be good enough with the overridable getSourceClass()