v2 refactoring

[Gummibeer]

solves: #10

I think the biggest change is the drop of $map and therefor the custom value via an array map. The Values are also forced to lower-case.
To customize value & index override the getValue() or getIndex() method.

@brendt atm we only support the default index to retrieve an instance and also listed in getIndices(). Do you have an idea how to solve this? The moment I try to construct an instance and call getIndex() in resolveFromStaticMethods() I end up in an endless round because the construct calls isValue() & isIndex() which will call resolve().

The only way I could think about would be to call all the methods and there getIndex() after I build the map with default indices. Or do we drop make() with index support in total?

[brendt]

We should also add an UPGRADING entry, explaining all the breaking changes.

[brendt]

atm we only support the default index to retrieve an instance and also listed in getIndices(). Do you have an idea how to solve this? The moment I try to construct an instance and call getIndex() in resolveFromStaticMethods() I end up in an endless round because the construct calls isValue() & isIndex() which will call resolve().

Yeah, I know, this is the reason why the README states this when using anonymous value classes:

Note that one drawback of this approach is that the value of the enum is always the name of the static function, there's no way of mapping it.

Continuing with what you said:

The only way I could think about would be to call all the methods and there getIndex() after I build the map with default indices. Or do we drop make() with index support in total?

I believe the same problem happens if you're making a enum from a value that's overridden. We should look into a solution for the problem, as I think it's required to be able to construct an enum from its index.

Can we prevent the loop by building some sort of value/index cache first, before validating whether the enum is valid?

[Gummibeer]

@brendt I can try to find a way to initialize the mapping, call all custom value/index getter, override the initialized mapping and mark the class as initialized. An idea would be to have a boolean stating if the enum is fully resolved or not and if it isn't we won't call isValidIndex/Value().

[brendt]

@Gummibeer sounds like a good idea.

[Gummibeer]

The new unittest suite should demonstrate better what is possible and what's not. I've dropped the RecursiveEnum now because I haven't found a real scenario why I would need to create an instance of the current enum in a non-static method.
If this is a real-world scenario and required I will put the solution in __call() and corresponding unittest back in.
Except the two duplication enums I've also choose some more understandable and real enums instead of foo().

This is more something like a "let the code represent what i was thought for" than a real rewrite/refactoring. So the idea is the same and now everything should be consistent. So you can instantiate make() an instance by the name, value or index. The real values and indices from the getters are respected also for instantiation.

[Gummibeer]

v2.0.0-dev.1

[brendt]

Any change we can cache these array_combine calls so that this hasn't be done very time an enum is constructed?

[Gummibeer]

Yeah, will try to add some more poor-man caches to keep these values.

[Gummibeer]

Hey, adding the caches has no impact on performance. I've added a speed test and the uncached make() call needs the same time like the cached one.
And adding two static variables and two methods for a single call without any performance impact isn't a good trade!?

Spatie\Enum\OldTests\WeekDayEnumSpeedTest::can_make_faster_from_cache_than_build
Failed asserting that 1.4 is less than 1.4.

Spatie\Enum\OldTests\WeekDayEnumSpeedTest::can_load_stable_from_cache
Failed asserting that 0.4 is less than 0.38000000000000006.

Spatie\Enum\OldTests\WeekDayEnumSpeedTest::can_make_faster_from_cache_than_build
Failed asserting that 0.6 is less than 0.6.

[brendt]

You're right, let's keep it as is

[brendt]

@Gummibeer great PR! Could you maybe also elaborate how the recursive loop problem has been solved?

[Gummibeer]

@brendt I've dropped the recursive support because I don't see a usage/reason!?
If you want it I can add the fix again - pass the call from __call() to __callStatic().

[brendt]

Right, I forget, sorry! Dropping it is entirely fine!

As far as I'm concerned, all looks good! I'd say: feel free to merge this in and tag 2.0!

[Gummibeer]

@brendt I've updated the changelog and readme and also add it to the http://docs.spatie.be during https://github.com/spatie/docs.spatie.be/pull/398

could you give a final approve? and update the repo description link if the docs PR is merged?

[brendt]

Tom, looks really good! I've added one remark about the performance thing, it can be kept the way it is. I've asked the team to take a look at your docs PR. Meanwhile you're free to merge this and tag v2!

[Gummibeer]

https://github.com/spatie/enum/releases/tag/2.0.0

[brendt]

Congratulations!