laravel-ide-helper icon indicating copy to clipboard operation
laravel-ide-helper copied to clipboard

PHPDoc generation for models violates `laravel_phpdoc_separation` StyleCI rule

Open chescos opened this issue 2 years ago • 4 comments

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()

chescos avatar Dec 26 '21 13:12 chescos

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.

mfn avatar Dec 26 '21 21:12 mfn

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?

tomcoonen avatar Aug 04 '22 12:08 tomcoonen

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.

mfn avatar Aug 04 '22 12:08 mfn

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.

chescos avatar Aug 04 '22 13:08 chescos