Skip to content

Conversation

promatik
Copy link
Contributor

Hi @jasonmccreary.

Until now Trace Command would read files from app path, or if set, it would read files from config('blueprint.models_namespace').

This PR adds the possibility to trace models outside app, for instance, developers may load files from .\vendor\... or .\packages\..., and they may provide multiple paths, comma separated.

php artisan blueprint:trace --path="vendor\spatie\laravel-permission\src\Models,packages\my-package\src\Models"
Traced 7 models

The appClasses function now loads the file content to get its namespace, previously it was using the path of the working folder (for instance, app or app\Models) and the filename, to infer App\Models\User.

Since now we can load files from other paths, like vendor\spatie\laravel-permission\src\Models we can't infer the namespace from that path, hence the file content is loaded to get the correct namespace Spatie\Permission\Models\Role.

I've also fixed the tests in order to receive the extra path argument, and added a Test to check if the option is received.

I tried to make the PR as short as possible, changing only the necessary lines.
Let me know if I miss something with this approach.

promatik added 2 commits July 25, 2021 00:24
appClasses now loads files to infer its namespace
Added test to validate the path is sent to tracer command
@jasonmccreary
Copy link
Collaborator

I understand the goal, but this opens the door to a lot of complexity. For example, when generating models, how will Blueprint know where to put them?

I'm open to this, but not if it means reworking foundational components and syntax. There's still a lot of value in generating a model, even if you need to move it to another namespace afterward.

@promatik
Copy link
Contributor Author

Hi @jasonmccreary, I may not be seeing the whole picture, but I think we don't need to rework anything else other than this.

This issue came up because I had the need to create Seeders and Factories for Models outside the app folder.
This allows me to run Trace command on those models, and then copy the contents to draft.yaml renaming the namespace to mine, so the seeders will be placed in project app folder.

If it happens that devs don't change anything, and run the build in the generated .blueprint content, the files will also be generated inside apps folder, but with all the folders representing the namespace.

models:
    Spatie\Permission\Models\Permission: { name: string, guard_name: string }
    Spatie\Permission\Models\Role: { name: string, guard_name: string }

Build command will generate this;

  • database/factories/Spatie/Permission/Models/PermissionFactory.php
  • database/factories/Spatie/Permission/Models/RoleFactory.php

If devs want it to be in root they just need to remove the namespace, build command with --only=seeders,factories will generate everything in its right place, with its right content.

  • database/factories/PermissionFactory.php
  • database/factories/RoleFactory.php
  • database/seeders/PermissionSeeder.php
  • database/seeders/RoleSeeder.php

I can't see any downsides on this, and beyond that by default everything works the same, I see this as an extra feature, an extra option to the Trace command.

Let me know if I'm missing something.

@jasonmccreary
Copy link
Collaborator

jasonmccreary commented Jul 31, 2021

I guess I'm not understanding the goal as Blueprint should do this already (your Spatie example). Can you provide a succinct example draft file, what Blueprint currently generates and what you want it to generate.

@promatik
Copy link
Contributor Author

Hi @jasonmccreary,

"Can you provide a succinct example draft file"

This is the point, I don't have a draft file, I want Blueprint Trace command to generate it for me, and it almost does the job.

Shortly, I want blueprint to trace vendor Models, so I can build factories and seeders for those existing Models without writing the draft.yaml myself.

Trace command does this! But not outside app folder.

Take Spatie Role and Permission Models as an example.
I want to use Trace to have the draft;

models:
    Spatie\Permission\Models\Permission: { name: string, guard_name: string }
    Spatie\Permission\Models\Role: { name: string, guard_name: string }

And from this draft I can run the build --only=seeders,factories to generate what I need.

Blueprint build works great with this draft.yaml and doesn't need any changes, the only changes needed are on this PR, on the trace command.

I hope I made it clear, let me know.

@jasonmccreary
Copy link
Collaborator

@promatik, I understand now. However, it still seems like an awkward way to achieve what you want - generate seeders/factories for non-app models.

You're effectively tracing a different path. Then copying that section of the .blueprint file into a draft.yaml, then running blueprint:build to generate just factories and seeders.

I feel like there is an opportunity to create a cleaner path here.

Out of curiosity, don't you still create these models within your own application for specific to this package,

@promatik
Copy link
Contributor Author

promatik commented Aug 4, 2021

You're effectively tracing a different path. Then copying that section of the .blueprint file into a draft.yaml, then running blueprint:build to generate just factories and seeders.

Exactly, that's exactly what I do, but everything is coded. And it works very well, except for what this PR tries to cover.

Out of curiosity, don't you still create these models within your own application for specific to this package.

No, models will be used from its source, it may be on vendor, many may be on packages folder too. Just to give you a little more context, this is a tool for devs to use, one of the features is exactly create seeders/factories for models, and those models can be anywhere.

I feel like there is an opportunity to create a cleaner path here.

If you have any idea, please let me know. As I see this, the current behavior is unchanged, Trace command just gets a new extra option.

@jasonmccreary
Copy link
Collaborator

@promatik, alright, I see what you're doing now. I still think it's an awkward way to generate seeders/factories for additional models.

With that said, there is valuing in tracing additional models to reference in draft files.

I will merge this, but have one request first. Please use an options array for path instead of a single comma delimited string.

@promatik
Copy link
Contributor Author

I did it @jasonmccreary, didn't know about options array, it's indeed much better.

php artisan blueprint:trace
Traced 5 models
php artisan blueprint:trace --path "vendor\spatie\laravel-permission\src\Models"
Traced 2 models
php artisan blueprint:trace --path "app\Models" --path "vendor\spatie\laravel-permission\src\Models"
Traced 7 models

@jasonmccreary
Copy link
Collaborator

So I understand it, if paths is set, these will be the only directories Blueprint traces? Or will it always trace the "models path"?

I assume the former based on the code, but want to ensure I understand it. Part of me wonders if the default behavior should be additive.

@promatik
Copy link
Contributor Author

Yes, if you define one or multiple path, only those will be scanned.

Part of me wonders if the default behavior should be additive.

In my case it wouldn't hurt to scan both folders.

Anyway I see this like a "pro" feature, if you define a path, you want to do something specifically with those models, at least in my case that is enough.

Let me know if you want me to change that behavior.

@promatik
Copy link
Contributor Author

By the way @jasonmccreary, I replaced Str::of() with other methods, to make this compatible with Laravel 6.
The commit is; 1d259f2

@jasonmccreary
Copy link
Collaborator

@promatik thanks. At first I though it should be additive. However, I agree this is a rather "pro" feature. So we'll assume the developer wants full control when using the --path option.

@jasonmccreary jasonmccreary merged commit 118125e into laravel-shift:master Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants