Artisan generators #26

jstoone · 2016-10-31T12:47:28Z

This is the initial work for what's been discussed in #25.

The good ol' todo

lindyhopchris · 2016-11-01T09:35:17Z

config/json-api.php

+    */
+    'generator' => [
+        'namespace' => 'JsonApi',
+        'by_resource' => true,


Can these be dasherized rather than underscored? I.e. by-resource. Just keeps it consistent with how the rest of the config file is written.

lindyhopchris

Looking really good so far. Have added a number of comments but this is definitely heading in the right direction!

lindyhopchris · 2016-11-01T09:50:30Z

src/Console/Commands/JsonApiGeneratorCommand.php

+    protected function getOptions()
+    {
+        return [
+            ['eloquent', 'e', InputOption::VALUE_OPTIONAL, 'Use eloquent as adapter.'],


So my preference would be to only register the Eloquent option if it is an option for the specific command. Checking $this->isIndependent would work I think.

The reason being is that if (for example) you do php artisan make:json-api:search -h it shows an Eloquent option even though the Eloquent option is not used by that command. Which I think is potentially confusing.

I appreciate this means you'll have to call each command individually rather than doing a foreach loop in the resource generator, but I think it's clearer for users to understand.

For the eloquent option, I think have --eloquent and --no-eloquent as boolean options. If --eloquent and --no-eloquent both return false (i.e. neither have been provided) then fall back to the value in config. I.e...

protected function isEloquent() { if ($this->isIndependent) { return false; } if ($this->option('no-eloquent')) { return false; } return $this->option('eloquent') ?: $this->useEloquent; }

lindyhopchris · 2016-11-01T09:53:05Z

src/Console/Commands/stubs/eloquent/schema.stub

+    protected $attributes = [
+        //
+    ];
+}


This stub should have the following in it:

/** * @return string */ public function getResourceType() { return self::RESOURCE_TYPE; }

lindyhopchris · 2016-11-01T09:55:43Z

src/ServiceProvider.php

+    protected function bindArtisanCommands()
+    {
+        foreach( $this->generatorCommands as $key => $command ) {
+            $this->app->singleton($key, function($app) use ($command) {


Interesting! I don't ever register commands as singletons. Is this recommended Laravel usage?

It is, if singleton makes sense, but I found out that it was redundant to register the commands as singleton instances, since the binding will happen in a more optimized way behind the scenes, when they get registered with artisan.

lindyhopchris · 2016-11-01T10:05:49Z

src/Console/Commands/JsonApiGeneratorCommand.php

+     *
+     * @var mixed
+     */
+    protected $isIndependant = false;


Typo in variable name - should be isIndependent

lindyhopchris · 2016-11-01T10:07:04Z

src/Console/Commands/JsonApiGeneratorCommand.php

+    protected function getStub()
+    {
+        if($this->isIndependant) {
+            return __DIR__.'/stubs/'.strtolower($this->type).'.stub';


Can we move the stubs directory to the root project folder. I have a strong belief that the src folder should just be for classes!

lindyhopchris · 2016-11-01T10:08:40Z

src/Console/Commands/stubs/eloquent/request.stub

+    protected $hasMany = [
+        //
+    ];
+}


For the Eloquent request stub, can we add the following:

/** * @var array */ protected $allowedFilteringParameters = [ 'id', ];

Because by default the search supports that as a filtering parameter.

lindyhopchris · 2016-11-01T10:10:31Z

PS: haven't used this review feature before! I do appreciate you're sharing unfinished work!!

jstoone · 2016-11-01T15:37:06Z

Thanks a bunch for giving good feedback on my code. I think you did very well on using the feature. :)

I'll keep you posted as I do progress.

jstoone · 2016-11-02T09:20:44Z

jstoone · 2016-11-03T17:39:54Z

Alright! The generators are now feature complete, and ready for you to lay your eyes upon them. 🎃

Change log since last commit-burst:

--no-eloquent-flag now works
You can now how choose between two namespace models:
- By resource name e.g: JsonApi\Tasks\Schema
- By class type e.g: JsonApi\Schemas\Task
The Search class now only gets generated if configured for eloquent
A bit of documentation added
Found a config item that still was using an underscore
- 'use_eloquent => 'use-eloquent'`

lindyhopchris

This is looking really great! Can't wait to use it in my applications.

A number of comments to address, then it'll be ready to merge. I've picked up on a few code-style things but I'm not overly worried about it - as I reformat the package code using PHP Storm when I'm preparing a release.

lindyhopchris · 2016-11-07T13:00:22Z

config/json-api.php

-    | JSON API Namespaces
-    |--------------------------------------------------------------------------
-    |
+    |-------------------------------------------------------------------------- | JSON API Namespaces |-------------------------------------------------------------------------- |


Can this heading be put back to the format that it was before?

lindyhopchris · 2016-11-07T13:02:26Z

config/json-api.php

+    |--------------------------------------------------------------------------
+    |
+    | With this package comes a set of handy generators. Which makes it possible
+    | to easily generate every class needed to implement an api resource.


Can this sentence be changed to:

This package supplies a set of handy generators. These make it possible to easily generate every class needed to implement a JSON API resource.

lindyhopchris · 2016-11-07T13:02:50Z

config/json-api.php

+    | we have included a few handy configuration options, so that the generators
+    | can follow your workflow.
+    |
+    | `namespace`       = The folder in which you will be storing everyting


typo: everything

lindyhopchris · 2016-11-07T13:07:03Z

config/json-api.php

+    |                           e.g. \App\JsonApi\Schemas\{User, Post, Comment}
+    |                           e.g. \App\JsonApi\Requests\{User, Post, Comment}
+    |
+    | `use_eloquent`    = Whether you are using Eloquent ORM in this app.


Suggested re-wording for the explanation here:

Whether your JSON API resources relate to Eloquent models or not. You can override the setting here when running a generator. If the setting here is `true` running a generator with `--no-eloquent` will override it; if the setting is `false`, then `--eloquent` is the override.

Don't know why this has not been marked as complete, but I've changed the text as mentioned. I hope I didn't misunderstand you. :)

lindyhopchris · 2016-11-07T13:09:37Z

config/json-api.php

+        'namespace' => 'JsonApi',
+        'by-resource' => true,
+        'use-eloquent' => true,
+    ]


requires a trailing comma for consistency with coding style.

lindyhopchris · 2016-11-07T13:41:01Z

stubs/abstract/schema.stub

+    {
+        return self::RESOURCE_TYPE;
+    }
+}


The following methods can be added to this stub:

/** * @param object $resource * @return mixed */ public function getId($resource) { // TODO } /** * @param object $resource * @return mixed */ public function getAttributes($resource) { // TODO }

lindyhopchris · 2016-11-07T13:43:15Z

stubs/eloquent/search.stub

@@ -0,0 +1,28 @@
+<?php
+
+namespace DummyNamespace


Missing semi-colon

lindyhopchris · 2016-11-07T13:44:01Z

stubs/independent/validators.stub

@@ -0,0 +1,43 @@
+<?php
+
+namespace DummyNamespace


Missing semi-colon

lindyhopchris · 2016-11-07T13:46:29Z

src/Console/Commands/HydratorMakeCommand.php

+     *
+     * @var string
+     */
+    protected $description = 'Create a new json-api resource hydrator';


On all of the descriptions, can we change json-api to JSON API

lindyhopchris · 2016-11-07T13:48:36Z

src/ServiceProvider.php

@@ -9,8 +9,7 @@
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,


Can you put this back to what it was?

jstoone · 2016-11-07T23:56:57Z

lindyhopchris · 2016-11-08T09:25:56Z

@jstoone awesome work - thanks so much for doing this, it's a fantastic addition to the package and something that I wouldn't have got to for a while because of work deadlines.

If you have any other proposal for additions to the package do let me know via issues! (Not saying you'd have to do the pull requests but would be great to hear your ideas!)

jstoone · 2016-11-08T10:25:22Z

@lindyhopchris you are very welcome! As I said before, it was a pleasure doing this and working with you. Let's see what we'll figure out next. :)

jstoone added 2 commits October 31, 2016 13:05

Initial implementation of artisan generators

0654066

Add configuration options

e62094a

lindyhopchris changed the base branch from master to develop November 1, 2016 09:30

lindyhopchris reviewed Nov 1, 2016

View reviewed changes

lindyhopchris requested changes Nov 1, 2016

View reviewed changes

Finished completing changes mentioned by @lindyhopchris review

004eae9

jstoone added 3 commits November 3, 2016 17:42

Implemented rest of the generator featureset

40a0c2a

Only add commands when app is running in CLI.

769d8a9

Fixed namespace bug.

b805dad

lindyhopchris requested changes Nov 7, 2016

View reviewed changes

Completed code review by @lindyhopchris

487ba97

lindyhopchris merged commit 1804aa5 into cloudcreativity:develop Nov 8, 2016

batdema mentioned this pull request May 18, 2018

Relationship named "store" #185

Closed

vlavlad mentioned this pull request Jun 16, 2019

Exception in updating resource via ContentNegotiator for multipart/form-data: Expecting request to contain a JSON API document. #369

Closed

brewers-dev-phil mentioned this pull request Sep 25, 2020

Exception Handler always returns blank 500 internal server error #555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Artisan generators #26

Artisan generators #26

jstoone commented Oct 31, 2016 •

edited

Loading

lindyhopchris Nov 1, 2016

lindyhopchris left a comment

lindyhopchris Nov 1, 2016

lindyhopchris Nov 1, 2016

lindyhopchris Nov 1, 2016

lindyhopchris Nov 1, 2016

jstoone Nov 3, 2016

lindyhopchris Nov 1, 2016

lindyhopchris Nov 1, 2016

lindyhopchris Nov 1, 2016

lindyhopchris commented Nov 1, 2016

jstoone commented Nov 1, 2016

jstoone commented Nov 2, 2016

jstoone commented Nov 3, 2016

lindyhopchris left a comment

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

jstoone Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

lindyhopchris Nov 7, 2016

jstoone commented Nov 7, 2016

lindyhopchris commented Nov 8, 2016

jstoone commented Nov 8, 2016

Artisan generators #26

Artisan generators #26

Conversation

jstoone commented Oct 31, 2016 • edited Loading

The good ol' todo

Choose a reason for hiding this comment

lindyhopchris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindyhopchris commented Nov 1, 2016

jstoone commented Nov 1, 2016

jstoone commented Nov 2, 2016

jstoone commented Nov 3, 2016

lindyhopchris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstoone commented Nov 7, 2016

Awesome review, awesome job!

Changelog

lindyhopchris commented Nov 8, 2016

jstoone commented Nov 8, 2016

jstoone commented Oct 31, 2016 •

edited

Loading