Fix #247: Implements blueprint:build --only and --skip

[axit-joost]

Closes #247

This is an implementation of the --only and --skip command line options for the blueprint:build command.

Both commands take a comma separated list of 'file classes' (for the lack of a better word), those being:

• controllers
• factories
• migrations
• models
• routes
• seeders
• tests
• events
• requests
• jobs
• mails
• notifications
• resources
• views

The command passes the string values to Blueprint\Builder::execute() as additional, optional parameters $only and $skip. The action body will convert these strings to arrays, and passes those arrays to $blueprint->generate() alongside of the $registry. This action iterates over the registered $this->generators and for each one call output(), again passing the $tree, $only and $skip.

Each registered generator implements Blueprint\Contracts\Generator. The contract is updated to include array $only, array $skip in the output() signature, which demands all of the generators adhere to this.

Inside the output() action of each generator, I have wrapped the output generation inside an if statement that references a $this->shouldGenerate() method, passing the $only and $skip to that method, which returns a boolean value whether it should generate or not.

Currently, the shouldGenerate() utilizes in_array() calls, using a hardcoded string to identify the 'file class'. Perhaps this should be extracted to a protected instance variable, but I am unsure what to call it.

With regards to the test suite: of course this blew up everything, as everything Mocked needed to be modified. In this Work in Progress I've managed to get the test suite to green again, but I still need to implement test cases to verify the proper workings of both command line options.

Now, before I do that, I thought it might be wise to put it under early review before continuing.

I am looking forward to all your feedback, so that I can modify/proceed accordingly.

[dmason30]

Seems like a nice addition 🚀

However, I am not sure adding the same shouldGenerate logic to every generator is the correct approach? Why not just have a method that returns the type string? e.g.

Blueprint\Contracts\Generator interface;

public function type();

In each of the generators e.g. Blueprint\Generators\TestGenerator;

public function type() 
{
     return 'tests';
}

Then the shouldGenerate logic should be handled in the Blueprint::generate method and getting the type for each generator from the above.

I maintain a addon that swaps out the TestGenerator for a different one and there is also a nova addon. The suggested implementation above could be easier to understand for future addon development.

Both yours and this implementation will allow present and future addons could use their own types which is cool 😎.

One other issue specifically for the TestGenerator is that it requires controllers in order to generate any tests. Which maybe an argument to have a types method instead that returns an array of strings which for the TestGenerator would be ['controllers', 'tests'].

[axit-joost]

Thank you @dmason30, this was exactly the kind of feedback I was looking for. I'll take a stab at it probably somewhere this week, and also do the testing.

I have never tested command line utilities before, so I'm still looking at the best way to go about this. Do you have any pointers for me in that regard? Perhaps @jasonmccreary could also chip in on what would be the right way to go about this?

Thanks all!

[axit-joost]

@dmason30 I took your suggestion and applied it. The Builder class now houses a modified version of shouldGenerate that takes a $generator->types() array of strings representing the types of files generated, and returns a boolean that is used by the if condition to determine whether or not it should call the output on the generator.

That is indeed a much nicer, and more DRY approach, than having that shouldGenerate() in each and every generator.

I have also added, as per your suggestion, the array of strings approach and added ['controllers', tests'] to the TestGenerator. Although I am not entirely convinced of its usefulness. I do get what you are saying, and I do believe that there might be use cases that have use for this, so: it's there.

The only thing missing is the tests. As mentioned earlier, I am not sure how to approach this in the test code. I'm looking at StartCommandTest that uses a Mock filesystem, but that is more like a Unit test. I'm looking more at a Feature test that runs the blueprint:build command with a specific draft.yaml from the fixtures and specific options, to be able to assert whether or not files are being generated and output.

If anyone can make a suggestion, that would be great.

[axit-joost]

When testing by hand on a blank project, using the pascal-case fixture as draft.yaml file, I stumbled upon the fact that the FormRequestGenerator has logic inside of it that checks if a Request output file already exists and skips it if it does.

Upon inspection, it seems that all Statement Generators have this code. Just wondering: why is this code there, and for what purpose? Why wouldn't you want to overwrite by default?

Also, found in ViewGenerator.php:

                        // TODO: mark skipped...

So, I think there once was a thought to have a generator marking 'skipped', for instance by setting $output['skipped'] = $path; so that the build command's output can clearly output that files have indeed been skipped that would otherwise have been generated?

If that is the case, I think the Generators themselves should determine whether or not they 'shouldGenerate', so that we can set the skipped action to allow for this use case.

Or, we could simply add a new issue to that effect?

[jasonmccreary]

@axit-joost, I think certain resources make sense to skip, not overwrite. For example, form requests, views, jobs, mailables, etc.

Most of Blueprint uses references and assumptions based on convention. So if one of those references/resources already exists, it should not overwrite it with a default version.

As such, I do consider this different from what you're proposing. Furthermore, having every generator be responsible for knowing if it should be skipped or generated seems like a harder approach than Blueprint knowing not to run a generator.

To make it easier, I wouldn't expect you to be able to skip low-level components. My expectation is you could skip things like controllers, models, views, tests, factories, etc. But not mailables, jobs, etc. The latter would lead to Blueprint generating incomplete/invalid code anyway.

[axit-joost]

@jasonmccreary thanks for your feedback.

Gathering from your response, I compiled this list:

Generator	Allow --only or --skip	Auto-skips not to overwrite
Controller	X
Factory	X
Migration	X
Model	X
Route	X
Seeder	X
Test	X
Event		X
FormRequest	X	X
Job		X
Mail		X
Notification		X
Resource	X	X
View	X	X

To verify: The Generators that haveX-marked for Allow --only or --skip SHOULD be responsible for knowing if it should be skipped or generated.

The downside, in my opinion, is that you will have a shouldGenerate() method in all of those Generators, which is not very DRY. However, I do concur with your stance of not being able to skip over low level stuff like Jobs and Mailables, because it would basically amount to incomplete and invalid code.

However, for a best of both worlds, we could alternatively change the output of the $generator->types() method on the generators that do not have the X above to just be ['controllers'], because they are all statements from the controllers: section of the draft.

If you then would --skip=controllers it would skip the controllers and the statements. If you would do --only=controllers, it would generate the controllers AND all of the lower level stuff that the controllers would require, therefore generating code that is complete and valid.

(Sorry for being this lengthy again, I just want to be thorough)

[jasonmccreary]

I don't really know what you mean about $generator-types(). However, your description of how to proceed seems we're in alignment.

All I will say is I always look for the minimal change. So while I appreciate the contribution, I will not merge something that overly complicates the generators or changes the current output.

I see these options as disabling generation of certain resources. We can trust a developer who specifies these options knows the code which is or is not generated.

[axit-joost]

I think this is done.

I believe this hits the sweet spot of:

• Being a minimal change that does not overly complicates the generators or changes the current output: the only addition is a public function types(): array which returns an array of strings that denote the types of files generated. This concern is added to the Generator Contract.

• The code is DRY: Instead of repeating the same logic in every generator, we have a single shouldGenerate() on the Blueprint class, which is called in the generate() method. This also keeps the changes to the actual generators minimal.

• The code does not change the output contents: as a matter of fact, if you use Blueprint same as before, nothing changes. It does what it says on the tin and skips and limits accordingly.

• The feature exempts low-level components: The user is unable to address Events, Mailables, Jobs and Notifications to be skipped or limited individually. Instead, it can be skipped or limited using --skip=controllers or --only=controllers for wholesale skipping/only generation.

• The code is tested: The test suite is extended to test both the --only as well as the --skip option for both single as well as multiple values (comma separated).

The only thing is that my local php-cs-fixer may have done something to the preferred whitespace (e.g. concat spacing, et al.). So you might want to hit 'ignore whitespace' when reviewing the code.

If you want me to fix this, or any other issues, please let me know.

[jasonmccreary]

Cool. Merged. I'll let it marinate on master for a few days and then tag/announce it. Feel free to point your dependency at dev-master to test it out more. 👍

[adnoh]

it's a nice feature. would be great if you can extend the documentation and the information on the command help text by the list of types one can use. If you aren't used to the internal types of blueprint - like me - you'r lost and need to dig into git to find it.
What I would have use for is to regenerate only for example one specific controller - so I would like to give the controllername or filename to just regenerate this one

[bhanwarpsrathore]

@jasonmccreary This is nice feature to be used, but its not there in documentation. Updating those will be useful. Thanks in advance