Skip to content

Conversation

arnedesmedt
Copy link
Contributor

Enum now only allows string as a type.

This PR allows you to change the type of the enum. I'll had to make it backwards compatible, so I didn't change the constructor: I used a setter (setType) instead of passing the type as an extra constructor parameter. It was also impossible to add an extra parameter because of the spread parameter (...entries).

Instead I'll just cast the entries while changing the type.

@arnedesmedt arnedesmedt force-pushed the multipleEnunTypes branch 2 times, most recently from 615b660 to 80ee8dc Compare October 18, 2019 08:50
@codeliner
Copy link

I think we can change the enum constructor to:

public function __construct(...$entries, string $enumType = JsonSchema::TYPE_STRING)
 {
        //@TODO assert type
       //@TODO assert all entries match the type
        $this->entries = $entries;
 }

Removing entries type hint is ok, because passing string entries still works fine and a new optional argument defaulting to string type ensures BC for existing enum types. This way we keep the enum type simple.

@arnedesmedt
Copy link
Contributor Author

I've also tried this but then the vardiac error appeared (it has to be the last parameter). See https://wiki.php.net/rfc/variadics

@arnedesmedt
Copy link
Contributor Author

The Syntactic restrictions section

@arnedesmedt
Copy link
Contributor Author

And switching positions of the arguments in the constructor isn't backwards compatible...

@codeliner
Copy link

ah of course, that's not possible 🤔

What if we introduce an enum class for each type? The setType + casting solution solves the problem but is confusing.
I'd even add a StringEnumType and deprecate EnumType later

@arnedesmedt
Copy link
Contributor Author

Ok, that's a good idea! I'll create a enum for string and int. That will probably be the most used enums, I think.

@arnedesmedt
Copy link
Contributor Author

Here we go,

I've created two enum types.

@codeliner
Copy link

cool, that looks good 👍

@codeliner codeliner merged commit a286671 into event-engine:master Oct 18, 2019
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.

2 participants