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

Rename make commands #41

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Rename make commands #41

wants to merge 3 commits into from

Conversation

AbbyJanke
Copy link
Contributor

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.

@welcome
Copy link

welcome bot commented Jun 11, 2018

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

@tabacitu
Copy link
Member

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

tabacitu commented Jun 22, 2018

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

tabacitu commented Jun 22, 2018

@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.

@lloy0076
Copy link

lloy0076 commented Jul 4, 2018

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

@lloy0076 lloy0076 added the Breaking Change This change will break things and may need to wait for the next Backpack release for integration. label Jul 4, 2018
@lloy0076
Copy link

lloy0076 commented Jul 4, 2018

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.

@tabacitu
Copy link
Member

tabacitu commented Jul 5, 2018

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

@OliverZiegler
Copy link
Contributor

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.

@tswonke
Copy link

tswonke commented Jul 7, 2018

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

tswonke commented Jul 7, 2018

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

@stale
Copy link

stale bot commented Sep 5, 2018

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 stale bot added the stale label Sep 5, 2018
@AbbyJanke
Copy link
Contributor Author

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?

@stale stale bot removed the stale label Sep 6, 2018
@stale
Copy link

stale bot commented Nov 5, 2018

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 stale bot added the stale label Nov 5, 2018
@tabacitu
Copy link
Member

tabacitu commented Nov 5, 2018

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)

@tswonke
Copy link

tswonke commented Nov 6, 2018

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.

@stale
Copy link

stale bot commented Jan 5, 2019

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 stale bot added the stale label Jan 5, 2019
@stale stale bot closed this Jan 20, 2019
@tabacitu tabacitu reopened this Mar 12, 2019
@stale stale bot removed the stale label Mar 12, 2019
@stale
Copy link

stale bot commented May 11, 2019

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

stale bot commented Jun 17, 2020

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 stale bot added the stale label Jun 17, 2020
@tabacitu tabacitu removed the stale label Jun 17, 2020
@stale
Copy link

stale bot commented Aug 16, 2020

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 stale bot added the stale label Aug 16, 2020
@tabacitu tabacitu removed the stale label Aug 17, 2020
@stale
Copy link

stale bot commented Nov 1, 2020

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 stale bot added the stale label Nov 1, 2020
@tabacitu tabacitu removed the stale label Nov 10, 2020
@stale
Copy link

stale bot commented Jun 26, 2021

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 stale bot added the stale label Jun 26, 2021
@tabacitu tabacitu removed the stale label Jun 28, 2021
@stale
Copy link

stale bot commented Nov 28, 2021

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 stale bot added the stale label Nov 28, 2021
@tabacitu tabacitu removed the stale label Nov 28, 2021
Copy link

stale bot commented Feb 11, 2024

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 stale bot added the stale label Feb 11, 2024
@pxpm pxpm removed the stale label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This change will break things and may need to wait for the next Backpack release for integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants