Generators icon indicating copy to clipboard operation
Generators copied to clipboard

Rename make commands

Open AbbyJanke opened this issue 6 years ago • 28 comments

Renaming Backpack generator commands to follow more accurate Laravel schema:

backpack:crud-> backpack:make:crud
backpack:crud-controller -> backpack:make:controller
backpack:crud-model -> backpack:make:crud-model
backpack:crud-request-> backpack:make:crud-request
backpack:model -> backpack:make:model
backpack:request -> backpack:make:request
backpack:view -> backpack:make:view

I know it is a little longer to type, however I feel that it sticks with standard laravel console schema better and clarifies what it does better.

AbbyJanke avatar Jun 11 '18 22:06 AbbyJanke

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

  • if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
  • even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
  • not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
  • we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

-- Justin Case The Backpack Robot

welcome[bot] avatar Jun 11 '18 22:06 welcome[bot]

I agree the commands need an update. We really didn't think ahead when we created this 2 years ago.

We do have another issue, though - we have our own convention, ever since the installation commands. Every Backpack package has its own installation command under its package name. Ex:

php artisan backpack:base:install # installs backpack/base
php artisan backpack:base:add-sidebar-content "<li><a href='{{ url(config('backpack.base.route_prefix', 'admin').'/log') }}'><i class='fa fa-terminal'></i> <span>Logs</span></a></li>" # adds a sidebar item
php artisan backpack:crud:install # installs backpack/crud

And I think we should keep this convention going forward, and all Backpack packages (and extension) should prefix their commands with their package name. This way we can make sure we don't have overwritten commands:

backpack:crud-> backpack:make:crud -> backpack:generators:make:resource
backpack:crud-controller -> backpack:make:controller  -> backpack:generators:make:controller
backpack:crud-model -> backpack:make:crud-model -> backpack:generators:make:crud-model
backpack:crud-request-> backpack:make:crud-request -> backpack:generators:make:crud-request
backpack:model -> backpack:make:model -> backpack:generators:make:model
backpack:request -> backpack:make:request -> backpack:generators:make:request
backpack:view -> backpack:make:view -> backpack:generators:make:view

Please keep the above in mind. I don't think it's a good solution, but I think it's the only way we could change the commands while respecting all conventions.

Now let me go on a rant about "the best" way in my opinion:

tabacitu avatar Jun 22 '18 09:06 tabacitu

Backpack\Make

So how about... we create a new package to replace Backpack/Generators? :-) Crazy, I know. But hear me out. Different package, different name, different command namespace (php artisan backpack:make:smth). So it solves that problem. We could even register some intuitive shortcuts to it, like php artisan make:crud which I think would be waaay easier to remember.

It's also a different package, so you'd expect things to be different. No backwards compatibility needed. Solves that problem.

Right now in Generators we have a bunch of commands I don't think anybody uses. We can just keep the useful commands, mainly php artisan backpack:make:crud. Maybe even php artisan backpack:make:controller, php artisan backpack:make:model, php artisan backpack:make:request since they're going to be used by php artisan backpack:make:crud anyway. We have the code, so we might as well let people call it separately. Solves that problem.

Generated controllers use setFromDb() which is a nightmare - it works, but we can't push any updates to it without breaking people's apps. A better way to proceed would be to generate actual addField() and addColumn() code. We can modify and tweak that every day, and not break people's apps. In time, we would deprecate setFromDb() and I'll be able to sleep at night :-) Solves that problem.

Right now generated CRUDs don't have routes. You have to do it manually. But we now have a command for it, we can automatically generate the CRUD::resource(). Solves that problem.

Right now generated CRUDs don't insert an item in the menu. You have to do it manually. But we now have a command for it, we can automatically generate a sidebar item. Solves that problem.

To get a glimpse of how I see this happening, because I also see this php artisan backpack:make:crud command being more interactive & customizable than the current one, here's how I see the command and interaction right now:

php artisan backpack:make:crud 
#notice: no extra parameters; you don't have to remember if it's singular, plural, etc; just the command, which is simple to remember or can even be php artisan make:crud, but that would break the backpack convention

> ----------------------
> | Making  a new CRUD |
> ----------------------
> Please answer a few questions:
> 
> What is the TABLE NAME? (ex: products; db table where your entries are stored) 

tags

> What is your entity name, in PLURAL? (ex: products; used for showing buttons like Edit Products)

tags #sensible default is provided, just have to hit Enter most of the times

> What is your entity name, in SINGULAR? (ex: product; used for showing buttons like Add Product)

tag  #sensible default is provided, just have to hit Enter most of the times

> What you like to generate a ROUTE in routes/backpack/custom.php?

yes #defaults to yes, just have to hit Enter most of the times

> What you like to add a SIDEBAR ITEM in resources/views/vendor/backpack/base/inc/sidebar_content.blade.php?

yes #defaults to yes, just have to hit Enter most of the times

> What ICON would you like it to have?

fa fa-list #defaults to something, anything

> Would you like to open the files after generation? If so, please type your IDE handle (ex: subl, pstorm, vscode)

no #defaults to no

> --------------
> | Thank you! |
> --------------
> Generated App\Models\Tag.php
> Generated App\Http\Requests\TagRequest.php
> Generated App\Http\Controllers\Admin\TagCrudController.php
> Added route in config/backpack/custom.php
> Added sidebar item in resources/views/vendor/backpack/base/inc/sidebar_content.blade.php
> ------------------------
> Done! Please take a look at the generated files and modify as you like.
> ------------------------

Additionally we could have a long list of questions, for all kinds of customizations, but with a different flag, for example php artisan backpack:make:crud --verbose:


> [OPTIONAL] Where would you like the Model to be generated? (model namespace)

App\Models #sensible default is provided, just have to hit Enter most of the times

> [OPTIONAL] Where would you like the CrudController to be generated? (controller namespace)

App\Http\Controllers\Admin #sensible default is provided, just have to hit Enter most of the times

> [OPTIONAL] Where would you like your Request to be generated? (request namespace)

App\Http\Requests #sensible default is provided, just have to hit Enter most of the times

But that's less important. What I do think is important is that we do this AT THE RIGHT TIME. And I think that's when we render setFromDb() obsolete. Which I've been dying to do for a long long time - since we can't push any updates to it, ever, or we'd break people's apps. A better way to do the same thing as setFromDb() would be to generate actual addField() and addColumn() code, based on the table. Which is what I want this Backpack\Make package to do. So that would be the huge difference between Backpack\Make and Backpack\Generators - that's when I think it would make sense to change command syntax.

There. I said it. The entire plan :-) What do you think?

PROs:

  • this is a breaking change anyway; we'd change a command people use all the time, so they need to hear about this; so telling them to replace something from their require-dev wouldn't be an issue, it would be in the upgrade guide;
  • it will allow us to deprecate some commands that few people use, and that Laravel itself provides nowadays - make:model exists, make:request exists, backpack:view is useless I think;
  • it would allow people who don't agree with the new generator to still use the old one, for a while;
  • it will allow us to bring route generation and sidebar item generation functionality;
  • it would allow us to stop using that damn setFromDb() and, in time, deprecate it;

CONs:

  • it's a lot of work :-) But hey, nothing worth doing is easy, right?

tabacitu avatar Jun 22 '18 09:06 tabacitu

@OwenMelbz , @eduardoarandah , @lloy0076 , @OliverZiegler , @iMokhles if you guys have time to read my rant above, please do. I've included ALL my ideas about how we can improve CRUD generation.

I expect this to make it even faster to generate CRUD panels - down from 10 minutes to something like 10 seconds (once you have the migration/table ready, of course). So a HUGE improvement. Would love your opinions on this too.

tabacitu avatar Jun 22 '18 09:06 tabacitu

So does this make this PR completely redundant, @tabacitu? Or is it worth going through with it?

lloy0076 avatar Jul 04 '18 13:07 lloy0076

Also, this may completely break something that someone has written - why someone would script artisan commands is beyond me, I don't know however changing command names isn't to be done lightly.

@AbbyJanke @tabacitu If this goes through, the documentation in various OTHER places may neeed changing too.

lloy0076 avatar Jul 04 '18 13:07 lloy0076

@lloy0076 yes, that Make package would make this PR redundant. Yes, it would completely change the docs. But for the better, I think :-)

tabacitu avatar Jul 05 '18 14:07 tabacitu

So at this point I would suggest looking at https://github.com/webfactor/laravel-generators

I think this does everything you describe and can be extended for people’s own use.

The make:entity command is quite laravelly and for future updates there could get a more detailed command list (as currently there is only this one).

Furthermore you could use this as base for other generators.

OliverZiegler avatar Jul 07 '18 07:07 OliverZiegler

Thank you, @OliverZiegler

I tried to provide an option that you can define your own naming conventions by using your own naming classes.

The explanation for this part of the package is still missing in the readme - I will add it as soon as possible.

tswonke avatar Jul 07 '18 09:07 tswonke

I added the "naming" part in the readme of that package. I hope this will help - I'm not good in writing documentation...

tswonke avatar Jul 07 '18 23:07 tswonke

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Sep 05 '18 23:09 stale[bot]

Mm gonna toss this thought out: soo with the Nova release they are usung things like ‘nova:resource’ so maybe what we have currently is best option?

AbbyJanke avatar Sep 06 '18 00:09 AbbyJanke

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Nov 05 '18 01:11 stale[bot]

Oh. My. God. OH MY GOD!

@tswonke , @OliverZiegler , I like your laravel-generators packages so so much. Miles ahead of this one. Great job!

I think there are a few improvements needed for better usability (mostly documentation), but that’s to be expected - as a developer it’s difficult to intentionally “forget” how it works and put yourself in the shoes of someone using it for the first time. In the next few days I’ll hopefully be able to dedicate some time to using the package again and again, propose some changes and write the PRs. Awesome work guys!!!

After the few usability changes I have in mind, I’d love for your package to become the “official” generators package for Backpack, instead of this one + laracasts. Would that be ok with you?

(anybody else, if you haven’t tried their package yet - do it now. it works GREAT)

tabacitu avatar Nov 05 '18 15:11 tabacitu

Thank you @tabacitu ! :-)

I started the package primarily to optimize our own development process, but always had in mind that this could be useful for others too. That's why we tried to make it customizable, but we know that there is still work to do.

Your help and ideas are always welcome and of course we are pleased getting "official" ;-) - maybe after some issues have been solved.

tswonke avatar Nov 06 '18 10:11 tswonke

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Jan 05 '19 16:01 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar May 11 '19 10:05 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Sep 26 '19 07:09 stale[bot]

Up!

tabacitu avatar Oct 04 '19 07:10 tabacitu

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Dec 03 '19 08:12 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Feb 01 '20 08:02 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Apr 16 '20 06:04 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Jun 17 '20 05:06 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Aug 16 '20 09:08 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Nov 01 '20 07:11 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Jun 26 '21 03:06 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Nov 28 '21 09:11 stale[bot]

Hi there!

Is this still an issue? No activity in 60 days. I'm going to mark it as stale for now, and close it in 14 days if no further activity occurs. I know you guys are all busy, but if this is important to you please reply or something, so I know not to close it.

Thank you!

-- Justin Case The Backpack Robot

stale[bot] avatar Feb 11 '24 02:02 stale[bot]