Skip to content
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

Complete baking of enums. #972

Merged
merged 10 commits into from
Jan 2, 2024
Merged

Complete baking of enums. #972

merged 10 commits into from
Jan 2, 2024

Conversation

dereuromark
Copy link
Member

@dereuromark dereuromark commented Dec 28, 2023

Completes #968 and #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

  • both enums incl the cases if given
  • map those to the baked table
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:

  • Bake entity correctly in terms of annotations (currently still int and string every 2nd time for some reason)
  • Bake index and view correctly, as those also are using format() and h() instead of ->label().

@ADmad
Copy link
Member

ADmad commented Dec 28, 2023

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
Copy link
Member

ADmad commented Dec 28, 2023

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

@dereuromark
Copy link
Member Author

dereuromark commented Dec 28, 2023

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

  • DriverFeatureEnum
  • as well as the test enums in core

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
Copy link
Member Author

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.

{
$formatted = [];
foreach ($cases as $case => $alias) {
$alias = mb_strtoupper(Inflector::underscore($alias));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to underscore? Developers could input their choices as snake_case or UPPER_CASE. Do we also need to support mixedCase?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All cases should be possible now, as only the case is touched, not the value

src/Command/EnumCommand.php Outdated Show resolved Hide resolved
src/Command/ModelCommand.php Show resolved Hide resolved
@ADmad
Copy link
Member

ADmad commented Dec 30, 2023

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
Copy link
Member

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
Copy link
Member Author

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

@othercorey
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

ADmad commented Dec 31, 2023

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
Copy link
Member

ADmad commented Dec 31, 2023

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
Copy link
Member

ADmad commented Dec 31, 2023

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

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

@dereuromark
Copy link
Member Author

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

  • CamelBacked
  • key value switching as requested

@othercorey
Copy link
Member

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
Copy link
Member Author

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

@dereuromark dereuromark merged commit c7cb269 into 3.next Jan 2, 2024
6 of 8 checks passed
@dereuromark dereuromark deleted the 3.next-bake-enums branch January 2, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants