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

Refactor Generators #4121

Merged
merged 2 commits into from
Jan 31, 2021

Conversation

mostafakhudair
Copy link
Contributor

@mostafakhudair mostafakhudair commented Jan 17, 2021

Refactor Generators commands to match with standard commands with additional useful options

All generators extends BaseCommand using GeneratorTrait

Fixes Controller extends PresenterController methods,

New command make:config to generates config file:
e.g. php spark make:config auth will generates:
Config::Auth - app\Config\Auth.php

Auto generate Entity class on make:model with option --return entity
e.g. php spark make:model users -return entity will generates:
App\Models::Users - app/Models/Users.php
App\Entities::Users - app/Entities/Users.php

New option --suffix to append component name to class name
e.g. php spark make:controller blog --suffix will generates:
App\Controllers::BlogController - app/Controllers/BlogController.php

New option --session used with make:migration to generates session migration instead of session:migration with two additional options --table, --dbgroup available only when --session option is called.
e.g. php spark make:migration --session will generates:
App\Database\Migration::CreateCiSessionTable - app/Database/Migration/*****_CreateCiSessionTable.php

  • Simplify code and clean unnecessary vars/methods

userguide will be updated in a separated PR

Securely signed commits

  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

closes #4055

Many thanks to @MGatner for letting this start, and alot alot alot of thanks to @paulbalandan for his friendly and helpful review and suggestions and of course for his powerful GeneratorCommand class and it's methods

@michalsn
Copy link
Member

It looks really nice. I will try to play with it next week. The only question is how big is the breaking compatibility we will have to deal with here.

@mostafakhudair
Copy link
Contributor Author

I think none bc we will have, while CommandGenerator class wasn't published yet, als deprecated is going fine, but i'm sure that am you will find some for me :D

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

In fine, I think the refactoring of GeneratorCommand has been given justice. Thumbs up for that. My suggestions were delicately applied and worries were resolved. I just need to play with this live to see any inconsistencies but Im currently pressed at work so I may take this week for that, unless someone will get to it before me. 😂 I have left only very minor cosmetic changes, btw.

My only concern is on test coverage. Modesty aside, GeneratorCommand and the commands are fully tested and covered. I am very keen on coverage esp. for my contributed code. So if there are uncovered lines, please add a test or two to cover those.

So far, I'll give this a provisional pass.

system/Language/en/CLI.php Outdated Show resolved Hide resolved
@paulbalandan
Copy link
Member

Please bring back the deprecated migrate:create command. We are not bumping major versions so that should not be deleted.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Here are my comments while tinkering with this locally. Also, I've annotated places not covered with tests. Just 8 lines so it would be easy to cover.

system/Language/en/CLI.php Outdated Show resolved Hide resolved
system/CLI/GeneratorTrait.php Outdated Show resolved Hide resolved
system/CLI/GeneratorTrait.php Outdated Show resolved Hide resolved
system/CLI/GeneratorTrait.php Outdated Show resolved Hide resolved
system/CLI/GeneratorTrait.php Outdated Show resolved Hide resolved
system/CLI/GeneratorTrait.php Outdated Show resolved Hide resolved
system/CLI/GeneratorTrait.php Show resolved Hide resolved
system/CLI/GeneratorTrait.php Show resolved Hide resolved
system/Commands/Generators/ScaffoldGenerator.php Outdated Show resolved Hide resolved
system/Commands/Generators/ScaffoldGenerator.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Jan 31, 2021

@mostafakhudair Are you around today? We are hoping to release soon and this needs to be part of the next version so we don't run into breaking changes with the current generator behavior.

I haven't followed this closely but it looks like you and @paulbalandan have this mostly ready?

@paulbalandan
Copy link
Member

This is mostly ready, I suppose. I just don't know why SQLSRV is failing for all versions.

I'll give this the last round of local checks. If @mostafakhudair isn't around, I can do the changes myself.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Apparently, I don't have push permissions to his fork. So I'll do the finishing touches in a separate PR.

My last concern is make:scaffold last-minute renamed to make:component. IMO the former name is more appropriate as it describes code generation of set of components. The proposed naming is ambiguous as it can refer to only a single component rather than a set.

@paulbalandan paulbalandan merged commit 19c2ef3 into codeigniter4:develop Jan 31, 2021
@MGatner
Copy link
Member

MGatner commented Jan 31, 2021

@paulbalandan thanks for getting this in. And @mostafakhudair thanks for the ton of work out into it! I'm glad this will make release.

@mostafakhudair
Copy link
Contributor Author

@MGatner thanks for your support
@paulbalandan thanks for your review and for the finishing touch and making this ready for upcoming release

@mostafakhudair mostafakhudair deleted the refactor-generators branch January 31, 2021 18:18
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.

4 participants