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

Template customizing #120

Closed
wants to merge 25 commits into from
Closed

Template customizing #120

wants to merge 25 commits into from

Conversation

mixartemev
Copy link

@mixartemev
Copy link
Author

By the way, your styleCI service force me to make PSR-12 violation - to remove spaces around concatenating dot:)

Copy link
Member

@CristianLlanos CristianLlanos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your use case for changing the default templates?

If you're bothered with the timestamps, PR #55 will be included in the next release and you can always import the dev version of the package and use it from now.

@@ -25,7 +25,9 @@
|
*/

'path' => app_path('Models'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't change this line. If you need/want you models to be placed in a different path, you can customise it in your project.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why? In Laravel app models are stored and created by default exactly in app directory, not in app/Models.
Maybe it would be more correctly to keep Laravel defaults in default models config?

@@ -38,7 +40,9 @@
|
*/

'namespace' => 'App\Models',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Please, don't change this line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. In Laravel app models are stored and created by default exactly in App namespase.
https://github.com/laravel/laravel/blob/master/app/User.php

@@ -280,7 +284,7 @@
*/

'except' => [
'migrations',
'migrations', 'action_events', 'password_resets',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this table action_events created by Laravel itself?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes of course, it's Laravel service table created by default.

$defaultFile = $this->path([__DIR__, 'Templates', $name]);
$customTemplatePath = resource_path('generators');
$templatePath = file_exists($customTemplatePath) ? $customTemplatePath : __DIR__;
$defaultFile = $this->path([$templatePath, 'templates', $name]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reasoning behind renaming the Templates folder to templates?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually capitalized names are for directories containig capitalized php classes files, and lower case for directories containing other file which are also in lowercase.
IMHO that looks more consistently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 👍

@CristianLlanos
Copy link
Member

Btw, I'm using Laravel's Style CI configuration in this repository.

@mixartemev
Copy link
Author

My use case for changing the default template is custom phpdocs, codestyle formatting, maybe any coments..

@CristianLlanos
Copy link
Member

CristianLlanos commented Sep 24, 2018

Do you realise that a custom template wouldn't work without changing the corresponding code inside this package?

Let me elaborate a bit, if I were to add a {{ author }} at the top of each file which I'd like it to add a comment with my name. This tag alone wouldn't add the desired information to the generated models. Unless I add some code which does something with this tag, this would end up breaking all your generated models.

The way I see it, if we want to add some customisation to the model templates, that might need to go through a PR.

@mixartemev
Copy link
Author

Of course new tags will not work without changing the corresponding code inside package.
I was just talking about spaces, new lines, comments, but not about new tags.

@mixartemev
Copy link
Author

But probaly you are right, all this is better solved through PR.

*/
public function __construct(
Fluent $parentCommand,
Fluent $referenceCommand,
Model $parent,
Model $pivot,
Model $reference
Model $related
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you renamed this property? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For universality, because in all others relations - this property named 'related'.
I inject trait with comment() getter, who use this property.
Otherwise this trait isn't working and we need add getter in each relation - it will be not DRY:)

@CristianLlanos
Copy link
Member

Thanks for contributing. There are some many changes in this PR. I won't merge it since there are no tests backing us up to see whether something is breaking.

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.

None yet

2 participants