barryvdh / laravel-ide-helper

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

Have smart reset keep property comments too #1218

Open amcsi opened 3 years ago

amcsi commented 3 years ago

Although the --smart-reset option does keep the description of the model itself, when it comes to the comments for each property, those all get wiped unfortunately.

So I may have a model for which the ide-helper generates docblocks like this:

/**
 * App\User
 *
 * @property int $id
 * @property string $identifier
 * @property string $name
 **
class User extends Model {}

I may want to add a comment about the model, and some properties that are not self-explanatory, which is also the expected result for when rerunning the artisan command with --smart-reset:

/**
 * A user that can log into the system.
 *
 * @property int $id
 * @property string $identifier External UUID identifier of the user if it came from the 3rd party system.
 * @property string $name
 **
class User extends Model {}

However, running php artisan ide-helper:models --write --smart-reset actually yields this:

/**
 * A user that can log into the system.
 *
 * @property int $id
 * @property string $identifier
 * @property string $name
 **
class User extends Model {}

^ you can see that the class title/description was kept, but the comment for the $identifier property was removed.

Now I know that I could just not use --smart-reset or --reset and the comment would be kept. However in the large project that I work with we have a script that calls the command with --smart-reset automatically, because it is really helpful overall in keeping the model type docblocks up-to-date in case we may forget to manually e.g. change an existing property's type, which is something that wouldn't be done automatically if not using --reset or --smart-reset.

Bonus points for if multi-line property comments could be kept, at least for those following lines that start with additional whitespace, e.g.:

/**
 * A user that can log into the system.
 *
 * @property int $id
 * @property string $identifier External UUID identifier of the user
 *   if it came from the 3rd party system.
 * @property string $name
 **
class User extends Model {}

Here are the old PRs in which smart reset was introduced: #629, #875

mfn commented 3 years ago

Can you use database comments?

Because it's supported when you use this in your migration $table->string('foo')->comment('this is foo');

amcsi commented 3 years ago

@mfn for one, there's not SQL Server support for comments in Laravel, and that's one of the DBs we use. And besides, creating an entire migration just to add comments to a column in the DB can be a pretty heavy thing. Just commenting the property locally is a much more lightweight thing to do that's more practical in most cases.

Is that the reason the docblock comments get wiped out, ide-helper wants to try and populate the comment with the comment found in the DB, even if the DB engine doesn't support it? If so, then it would be nice if ide-helper kept local comments for a column property in the case that the comments for that column are empty.

mfn commented 3 years ago

even if the DB engine doesn't support it?

TBH I don't know, we use DBAL for that and might not know the difference? šŸ¤” https://github.com/barryvdh/laravel-ide-helper/blob/867a6fd9b4e4e582428e5eba5cf41f6707eeec8d/src/Console/ModelsCommand.php#L485

But I'm no DBAL expert.

amcsi commented 3 years ago

@mfn you're right, actually. But my other point still stands about how it's often not practical to have to create a migration just to keep a little comment on a property docblock.

mfn commented 3 years ago

IDK, I do šŸ˜„

Joke aside, I understand what you mean.