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

[Proposal] Add artisan command class generators #25

Closed
jstoone opened this issue Oct 28, 2016 · 17 comments
Closed

[Proposal] Add artisan command class generators #25

jstoone opened this issue Oct 28, 2016 · 17 comments

Comments

@jstoone
Copy link
Contributor

jstoone commented Oct 28, 2016

It's hacktober, and I still have not made any pull-requests. And since I really like your work on this project, I'd love to contribute! Therefore i propose the following feature.

Since Laravel has started to amp up their use of generators significantly version by version, I have been thinking if you're interested in having one or more generators as artisan commands covering creating:

  • Schema
  • Hydrator
  • Request
  • Search
  • Validators

Lingo proposal

php artisan make:api:resource Task --attributes="body,completed"

php artisan make:api:schema Task/Schema
php artisan make:api:hydrator Task/Hydrator
php artisan make:api:request Task/Request
php artisan make:api:search Task/Search
php artisan make:api:validators Task/Validators

Since it varies where developers want to locate their JSON API related classes e.g:

App\Api\{Schemas,Requests,Hydrators, ...}\Task;
                                         \People;
                                         [...]
or
App\Api\Task\{Schema,Request,Hydator, ...};

This could be configured within the config files.

Any feedback is very welcome, and please do tell if you feel like this doesn't fit the project. Because then I'll be extracting this to a separate companion package. :)

@jstoone jstoone changed the title Adding artisan command class generators [Proposal] Add artisan command class generators Oct 28, 2016
@lindyhopchris
Copy link
Member

Hi! More than happy for you to contribute and adding generators to this package would be a great addition. Thanks for your offer of help!

It'd be great to work out how this will be implemented. I think having separate generators for each "unit" would be really useful, but it'd also be great (if possible) one command that generates all the units at once. I.e. if you're adding Tasks to your application and know you'll need all of the units, you can generate them in one go. (Under the hood that'd just run the separate unit generators.)

I wasn't entirely sure what make:api:resource was in relation to the other generators you listed?

Here were some of my other thoughts:

  • I was wondering whether make:json-api:* would be a better namespace just to ensure no collision with other api packages?
  • For the schema and hydrator commands, it would be useful to have an Eloquent switch so that the generator knows whether to generate one that extends the Eloquent schema/hydrator or not. (Probably Eloquent could be the default with an option to say not Eloquent?)
  • Yes, definitely support the different namespacing conventions that you listed, and yes, that should be configurable via the json-api.php config file

Let me know your thoughts!

@jstoone
Copy link
Contributor Author

jstoone commented Oct 28, 2016

Seems like our implementation thoughts are very well aligned, that makes me happy. :)

The make:api:resource is actually what you mentioned that will generate the entire suite of units for the given resource e.g Tasks.

  • make:json-api:* sounds like a good idea
  • I would say defaulting to Eloquent is a good idea, and adding a --bare or --raw flag would make the class not extend Eloquent
  • Now it's just figuring out what the default namespace should be. Maybe App\JsonApi\ ?

A few thoughts

  • Also a feature we could check out later is something like the php artisan event:generate, that generates resources based on config namespace, by looking at the eloquent-adapter['map']-attribute
  • We could maybe add generator related configs in a new json-api-generator.php config file?

@lindyhopchris
Copy link
Member

Great!

I think for the Eloquent option it would be useful to have the default in the config. The reason being if you are not using Eloquent at all, then it'd be really annoying to always have to provide it to the generator. So maybe there's an --eloquent and a --no-eloquent option (as the naming is clearer) but if neither is provided then the default from config is used. I.e. the option allows you to override whatever the default is in your config.

make:json-api:resource sounds good.

The default namespace (obtained via config) should be JsonApi. This is the namespace I've used for the demo app and all the apps in which I've used this package, so I'd like to keep that as the default. I've never written a Laravel generator before, so don't know if it's possible to pull the application namespace in somehow... i.e. if you've renamed the App namespace.

In terms of App\JsonApi\Schemas\Tasks vs App\JsonApi\Tasks\Schema - I think this should be a pod option in config... with the former being false and the later being true. I'd suggest a default of true because again that's how it is in the demo app.

In terms of the config, I don't think there's too many settings in total (unless you disagree!) so maybe keep them under a generator key in the current json-api.php config. If it starts to get too big we can separate out.

For info, if you're starting any work on this then it should be branched off the develop branch. Keep the questions coming as needed!

@jstoone
Copy link
Contributor Author

jstoone commented Oct 29, 2016

I like where this is going!

  • I agree on changing the option name to the more explicit --eloquent and --non-eloquent
  • Regarding the application namespace, Taylor got us covered! :)
  • I agree that the App\JsonApi\Tasks\* should be default, as well as the base JsonApi.
  • Yes, let's keep the config in json-api.php and extact the generator stuff if needed.
    • I'll be adding the generator stuff at the bottom of the file under the custom adapters, agree?
  • I've branched out from develop and created a branch named artisan-generators, hope that is suitable naming for you as well. :)

Let the hacking begin!

@jstoone
Copy link
Contributor Author

jstoone commented Oct 31, 2016

Okay, so now the basic generators have been created and are now working. I'll just finish up some refactoring, and then I'll publish the PR.

As of now it only works using eloquent, but I'll post a features list on the PR to track what's missing and what's not.

Questions:

  • There are some of the classes that by default is non-dependant on eloquent and the eloquent related flags will be ignored, is this acceptable behavior?
    • Request
    • Validators
  • Since AbstractSearch seems coupled with Eloquent by itself, should this only be generated when use_eloquent => true ?
  • Are we interested in being able to do the following when creating an entire resource:
    • `php artisan make:json-api:resource Task --only=schema,request
    • `php artisan make:json-api:resource Task --except=search

This is great fun, a lot of good edge-cases to solve :)

@lindyhopchris
Copy link
Member

Great, glad it's fun - this is going to be a fantastic addition, so looking forward to getting it into the package.

In answer to your questions:

  1. If you're not extending from an abstract class it might be best to not have the flags for those two specific commands as they'd be confusing as they'd have no effect. But if you need to just ignore them then that's acceptable and it's fine for the make:json-api:resource to just ignore the flags when it comes to those two specific units.
  2. Yes, only generate an AbstractSearch if Eloquent is true as it is totally coupled at the moment.
  3. Yes, definitely interested in that. It would make sense if you wanted a resource but with only one of the units missing to use the except flag, but I also like the only flag too.

Hope that helps!

@jstoone jstoone mentioned this issue Oct 31, 2016
25 tasks
@jstoone
Copy link
Contributor Author

jstoone commented Nov 1, 2016

I agree, generators are such a great tool, and amps up productivity a lot.

As you can see I've created a PR, which is not the finished version. Since it's quite a lot of code to take in at once, I thought publishing while in an early state would make it easier for you (or others) to give feedback while in development.

Looking forward to hearing what you think.

@lindyhopchris
Copy link
Member

Hi! Have seen the pull request but haven't had time to look through it in detail. Will do so today and get back to you!

@jstoone
Copy link
Contributor Author

jstoone commented Nov 3, 2016

Hey @lindyhopchris, the generator is now feature complete and ready to rock! 🎸

I'd love to get your feedback! Even the small things like missing punctuation in docblocks or whatever.

I hope you like it so far!

@lindyhopchris
Copy link
Member

Hey! Thanks for this - I'll hopefully get a chance to look at it today.

@lindyhopchris
Copy link
Member

lindyhopchris commented Nov 8, 2016

Merged into develop. I need to review to check if this will be released as 0.5.1 or 0.6.0. I believe it can be released as 0.5.1 (which is my preference) as it should be non-breaking but need to double check how it runs if there's no generator config in the json-api.php file.

Things to do:

  • check how the generators behave without config
  • update change log
  • update upgrade guide
  • do release

@jstoone
Copy link
Contributor Author

jstoone commented Nov 8, 2016

It will fail. But Laravel gives you an option to merge the users config with the package default config files.
In that way the user will inherit the package configurations, until overridden.

PR is here: #28

@lindyhopchris
Copy link
Member

Yeah, I went for the generator using a default value when loading the value from config:
fad0559

Not sure about the merging the config because of the amount of other really application specific config that the json-api.php file contains. Will have to try that out in a few different scenarios to see if it causes any problems or if it's ok.

@jstoone
Copy link
Contributor Author

jstoone commented Nov 9, 2016

Sounds super good! I am super excited about seeing my contribution getting merged to develop! Looks like you're about to get ready to release? 🎉

@lindyhopchris
Copy link
Member

Yes, had been thinking I'd get to it today but might be tomorrow now

@jstoone
Copy link
Contributor Author

jstoone commented Nov 11, 2016

Sure thing, don't stress. :)

@lindyhopchris
Copy link
Member

Released as v0.5.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants