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

[5.8] Enh: `FactoryMakeCommand` updated to generate more IDE friendly code #28188

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
5 participants
@klimov-paul
Copy link
Contributor

commented Apr 11, 2019

Update 'make:factory' command to generate code, which is more consistent regardless to other generators:

  • import model via use statement instead of usage fully qualified class name inline.
  • added PHPDoc hint for $factory variable, facilitating better IDE type-hinting and static analysis

With this patch command php artisan make:factory -m Foo FooFactory generates following code:

<?php

/* @var $factory \Illuminate\Database\Eloquent\Factory */

use Faker\Generator as Faker;
use App\Foo;

$factory->define(Foo::class, function (Faker $faker) {
    return [
        //
    ];
});

@driesvints driesvints changed the title Enh: `FactoryMakeCommand` updated to generate more IDE friendly code [5.8] Enh: `FactoryMakeCommand` updated to generate more IDE friendly code Apr 11, 2019

@@ -1,6 +1,9 @@
<?php

/* @var $factory \Illuminate\Database\Eloquent\Factory */

This comment has been minimized.

Copy link
@ahinkle

ahinkle Apr 11, 2019

Contributor

Please remove this DocBlock. #24647.

This comment has been minimized.

Copy link
@ejunker

ejunker Apr 16, 2019

Is this correct? I think DockBlocks have to start with /** and this is just /*

http://docs.phpdoc.org/guides/docblocks.html

This comment has been minimized.

Copy link
@klimov-paul

klimov-paul Apr 16, 2019

Author Contributor

Such var description should be recognized correctly by any modern IDE. It is recognized by PHPStorm at least.
In the same time having this as PHPDoc block (e.g. `/**``) may cause problem with PHP code style fixers as they consider this to be a PHPDoc, which is not bound to function or class definition, and thus is incorrect.

@klimov-paul

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

This has requested and denied many times.

If some changeset constantly reappear in PRs, perhaps it means there is a point behind it, and it should be reconsidered from different angle.

Without the doc comment IDE consider factory code as the one containing an error and does not provide type-hinting in case you with to add a state definition:
no-comment

With the doc comment IDE consider code to be corrent and provide the type-hint for the factory methods:
has-comments

Please remove this DocBlock

Sorry, would not do this. You are free to edit the PR manually or close it altogether, I see not point to spent any more efforts in this.

@taylorotwell taylorotwell merged commit 1300902 into laravel:5.8 Apr 12, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I'll just give in on this, heh. I think it kinda harms the "tranquility" of the file having some IDE centric docblock in there but I'm just tired of fighting that battle and maybe I'm wrong anyways. 🤷‍♂️

@ahmadmayahi

This comment has been minimized.

Copy link

commented May 7, 2019

Too late to comment on this, but I disagree with such additions.
This is a trick rather than an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.