barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.17k stars 1.16k forks source link

PHPDoc generation for models violates `laravel_phpdoc_separation` StyleCI rule #1288

Closed chescos closed 1 year ago

chescos commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

Tjaitil commented 3 months ago

Any update on this? I see that the missing PR #1377 is not merged. But from the comments it seems to be rejected by the reviewers. Not been any update on that since feb 2023.

Since then laravel 11 has shipped with laravel pint installed by default. That makes it three major versions (9, 10, 11) where it is included by default.

I will +1 the arguments made in favor of adding this. If the package has an opt-out in the config it will also be compatible with people not using laravel pint preset or any other formatter.