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

PHPDoc generation for models violates laravel_phpdoc_separation StyleCI rule #1288

Closed
chescos opened this issue Dec 26, 2021 · 4 comments · Fixed by barryvdh/ReflectionDocBlock#8 · May be fixed by #1377
Closed

PHPDoc generation for models violates laravel_phpdoc_separation StyleCI rule #1288

chescos opened this issue Dec 26, 2021 · 4 comments · Fixed by barryvdh/ReflectionDocBlock#8 · May be fixed by #1377

Comments

@chescos
Copy link

chescos commented Dec 26, 2021

The PHPDoc generation for models (php artisan ide-helper:models) generates the following:

 * @property \Illuminate\Support\Carbon|null $created_at
 * @property \Illuminate\Support\Carbon|null $updated_at
 * @method static \Illuminate\Database\Eloquent\Builder|User newModelQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|User newQuery()

However, this violates the laravel_phpdoc_separation StyleCI rule, which is a default rule for the StyleCI Laravel preset (which is used as a default for Laravel itself). There should be a linebreak between the @property declarations and @method declarations, like this:

 * @property \Illuminate\Support\Carbon|null $created_at
 * @property \Illuminate\Support\Carbon|null $updated_at
 * 
 * @method static \Illuminate\Database\Eloquent\Builder|User newModelQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|User newQuery()
@mfn
Copy link
Collaborator

mfn commented Dec 26, 2021

Personally I don't consider anything of this "official", there is no "official laravel code style" AFAICS, the best we've is https://github.com/matt-allan/laravel-code-style but that's not official either.

Also: just because someone is using Laravel does not mean they're bound to using that code style. Same would be true if someone was Symfony but not their CS etc. (fun fact: at least when it comes to php-cs-fixer, there's an official Symfony code style rule set).

A good project has fixing CI as part of their pipeline somewhere anyway, therefore I'd say: if you care about code style, make sure you have it in your pipeline and then it doesn't matter what ide-helper generated.

If we change it, others might complain that we changed it. To me it's a non-issue, but you can of course try a PR if you want.

@tomcoonen
Copy link

This also conflicts with Laravel Pint now, maybe that is enough reason to fix it now?
Let me know if we can draft a PR?

@mfn
Copy link
Collaborator

mfn commented Aug 4, 2022

Personally (<- achtung: personal opinion only!): I don't see the point investing time here.

Even though many use Laravel, does not mean everyone uses the same code style (be it laravel/pint , matt-allan/laravel-code-style Jubeki/laravel-code-style or pure PSR-12 or custom), so everyone will have a different opinion on this one.

To me, the only true solution is: run an automatic code fixer as part of your CI/CD pipeline, then it's a non-issue.

@chescos
Copy link
Author

chescos commented Aug 4, 2022

This package is specifically designed for Laravel, and Laravel does now have official style guidelines (since Pint), so I'd argue that the produced PHPDoc should adhere to the official style guidelines.

Of course, not everyone will stay with the official default. But the majority will.

And yes, a CI/CD pipeline also solves this issue, but I think it's a bad solution to require users to use a CI pipeline if they want to use this package without running into conflicts with the default Laravel installation.

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