Skip to content

feat: add shield:model command #558

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

Merged
merged 7 commits into from
Jan 5, 2023

Conversation

paulbalandan
Copy link
Member

Supersedes and closes #491

@kenjis kenjis added the enhancement New feature or request label Dec 13, 2022
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Renaming it MyUserModel means that factories won't pick it up automatically when searching for UserModel. This seems a loss to me, maybe not for the library itself but for any supporting code.

@kenjis
Copy link
Member

kenjis commented Dec 14, 2022

UserModel instead of MyUserModel seems better for the default sample.
There is no need to prefix My.

@kenjis
Copy link
Member

kenjis commented Dec 14, 2022

Oh, I get an error if I specify UserModel. It's a bug.

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

When I tried with UserModel as the generated filename, I think the only issue was the class name conflict. I've made suggestions here to fix that in the code (though not the docs).

I think this will fix @MGatner concerns, which I agree with.

Other than that, my only suggestion might be to provide a default classname at the cli prompt.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I haven't run this but the code looks good. Thanks for making that change.

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for taking this one over.

@datamweb
Copy link
Collaborator

datamweb commented Jan 4, 2023

It's a bug.

@kenjis waiting for you. Did your worries go away?

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

Thanks all.

@datamweb datamweb merged commit aba50df into codeigniter4:develop Jan 5, 2023
@paulbalandan paulbalandan deleted the shield-model-spark branch January 5, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants