cakephp / bake

The Bake Command Plugin
Other
111 stars 101 forks source link

Complete baking of enums. #972

Closed dereuromark closed 10 months ago

dereuromark commented 10 months ago

Completes https://github.com/cakephp/bake/pull/968 and https://github.com/cakephp/bake/pull/969

Now fully supports baking enums including cases.

Examples

implicit cases for string:

bin/cake bake enum UserStatus inactive,active

explicit cases for string:

bin/cake bake enum UserStatus inactive:i,active:a

implicit cases for int:

bin/cake bake enum UserStatus inactive,active -i

explicit cases for int:

bin/cake bake enum UserStatus active:1,inactive:0

On top of that now also bake all works completely out of the box when you set a [enum] marker + optional case config as comment:

            ->addColumn('status', 'tinyinteger', [
                'default' => 0,
                'limit' => 2,
                'null' => false,
                'comment' => '[enum] inactive:0,active:1'
            ])
            ->addColumn('gender', 'string', [
                'default' => null,
                'limit' => 10,
                'null' => true,
                'comment' => '[enum] male,female,diverse'
            ])

bin cake bake all Users

will generate

enum UserStatus: int implements EnumLabelInterface
{
    case INACTIVE = 0;
    case ACTIVE = 1;
    ...

enum UserGender: string implements EnumLabelInterface
{
    case MALE = 'male';
    case FEMALE = 'female';
    case DIVERSE = 'diverse';
    ...
$this->getSchema()->setColumnType('status', \Cake\Database\Type\EnumType::from(\App\Model\Enum\UserStatus::class));
$this->getSchema()->setColumnType('gender', \Cake\Database\Type\EnumType::from(\App\Model\Enum\UserGender::class));

If this sounds fine as is, I will add a few more test cases. Just wanted to get some feedback on it first.

Also, open tasks:

ADmad commented 10 months ago

bin/cake bake enum UserStatus i:inactive,a:active

IMO it should be bin/cake bake enum UserStatus inactive:i,active:a as that's how named args work in PHP, so name:value feels more natural.

ADmad commented 10 months ago

Also instead of using uppercase case names I would use capitalized case names.

dereuromark commented 10 months ago

Also instead of using uppercase case names I would use capitalized case names.

Arent those kind of constants? I orientied myself on core files here actually, according to blame also the ones you added: https://github.com/cakephp/cakephp/blame/5.x/src/Http/Cookie/SameSiteEnum.php#L20-L22 Same for

So I guessed that we are now using UPPER_CASE instead of CamelCase here as convention.

Whatever we decide on, we probably want to have a code sniffer rule for it.

dereuromark commented 10 months ago

IMO it should be bin/cake bake enum UserStatus inactive:i,active:a as that's how named args work in PHP, so name:value feels more natural.

I was more thinking of how we all know arrays work with "key/case" => "value/alias" transferring here into those. But in the end I am also fine with another interpretation of it.

ADmad commented 10 months ago

I orientied myself on core files here actually, according to blame also the ones you added

I have always preferred CamelCased names but probably went with the consensus opinion :). Since the couple of core enums use upper case I guess that's what we have settled on.

othercorey commented 10 months ago

I orientied myself on core files here actually, according to blame also the ones you added

I have always preferred CamelCased names but probably went with the consensus opinion :). Since the couple of core enums use upper case I guess that's what we have settled on.

UPPERCASE enum cases usually come from other languages where convention is to use underscores to separate words in the case:

MY_CASE while php would typically use MyCase with no underscore.

dereuromark commented 10 months ago

The current understanding is mainly that those are close to constants. Thus the uppercase. We could still change the convention maybe.

othercorey commented 10 months ago

The current understanding is mainly that those are close to constants. Thus the uppercase. We could still change the convention maybe.

I'm happy to treat enums as constants if that is the convention.

markstory commented 10 months ago

My personal preference would be to use CamelCase for enum values. But I also recognize that uppercase is more aligned with conventions for constants.

Are there other projects we should be looking at to share conventions with?

dereuromark commented 10 months ago

Good question The official docs list them as camel: https://www.php.net/manual/en/language.enumerations.backed.php

But then again, it seems a bit harder to read what is the enum (class), what is the case and what is the property..

ADmad commented 10 months ago

Enums can have constants. Can't think of a use case for them but if someone did have them then they wouldn't be able to easily distinguish between the constants and case.

ADmad commented 10 months ago

Constants in Enumerations MAY use either PascalCase or UPPER_CASE capitalization. PascalCase is RECOMMENDED, so that it is consistent with case declarations.

https://github.com/php-fig/per-coding-style/blob/master/spec.md#9-enumerations

ADmad commented 10 months ago

Use UpperCamelCase for enumeration cases (e.g. InputArgumentMode::IsArray);

https://symfony.com/doc/current/contributing/code/standards.html

dereuromark commented 10 months ago

Hmm then would recommend switching core first And then add a code sniffer In the meantime I can address in this PR:

othercorey commented 10 months ago

Enums can have constants. Can't think of a use case for them but if someone did have them then they wouldn't be able to easily distinguish between the constants and case.

Constants in Enumerations MAY use either PascalCase or UPPER_CASE capitalization. PascalCase is RECOMMENDED, so that it is consistent with case declarations.

Funny they don't try to distinguish them either.

dereuromark commented 10 months ago

I also tested in in real life with int + string + nullable each. Seems to work all fine, incl rebake