-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Generate PHPDoc for laravel 8.x factories #1074
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing 😢
@mfn I think that something wrong in pipeline all tests passed with me. can you review please 🙄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is this here https://github.com/barryvdh/laravel-ide-helper/pull/1074/checks?check_run_id=1208372323#step:5:32
The test is L8 with --prefer-lowest
, i.e. Laravel 8.0.0
The other test, which works using --prefer-stable
is always the latest Laravel 8 version.
They changed/fixed something with the namespace resolution of factories between 8.0.0 and 8.*, that's why the test does not work in all environments.
Currently it only checks for L8, when it would need to check for an higher version…
Personally I feel this is all too much and the test suite simply shouldn't test with "Laravel 8.0.0" or any other "first release of Laravel", this is simply besides reality for this package.
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. See also barryvdh#1074 (review)
I think Also, I don't think you last commit is necessary => but let's wait first if the other change is accept, it will simplify things and the wrong test I believe. |
@mfn the issue is the |
Just no-op update from my side: I've not forgotten about it, just pretty busy at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, sorry for the long wait.
I took a look at the approach and it seems fine.
However there are a few things to iron out and, as of right now, with the newest Laravel dependencies, the test won't even be run anymore.
Thank you
src/Console/ModelsCommand.php
Outdated
$traits = class_uses(get_class($model), true); | ||
if (in_array('Illuminate\\Database\\Eloquent\\Factories\\HasFactory', $traits)) { | ||
$modelName = get_class($model); | ||
$factory = get_class($modelName::factory()); | ||
$factory = '\\' . trim($factory, '\\'); | ||
if (class_exists($factory)) { | ||
$this->setMethod('factory', $factory, ['...$parameters']); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM but I would like to see this de-nested/early returned. I think it will improve readability and maintenance.
Similar to what the first condition already does:
if (!in_array()) {
return;
}
if (!class_exists) {
return;
}
…setMethod;
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Done 👌
You resolve this one but didn't give any feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolve this one but didn't give any feedback
Frankly, I didn't know that there was a way to check out the version other than this, so i commit your review 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahmed-aliraqi I suggested to change the logic of the code slightly to be more read- and maintainable, see https://github.com/barryvdh/laravel-ide-helper/pull/1074/files#discussion_r534503449 . Do you think you still have time to do this? Would be much appreciated! Thank you 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfn sure
@mfn All Done 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
@ahmed-aliraqi can you please also add a changelog entry? thank you! |
Is everything ready now? @mfn |
Yes, already approved from my side! |
@barryvdh Hi, I 'd love to see these changes. When is the next release? |
Bumps [barryvdh/laravel-ide-helper](https://github.com/barryvdh/laravel-ide-helper) from 2.9.0 to 2.9.1. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/barryvdh/laravel-ide-helper/releases">barryvdh/laravel-ide-helper's releases</a>.</em></p> <blockquote> <h2>v2.9.1</h2> <h3>Added</h3> <ul> <li>Generate PHPDoc for Laravel 8.x factories <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1074">#1074 / ahmed-aliraqi</a></li> <li>Add a comment to a property like table columns <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1168">#1168 / biiiiiigmonster</a></li> <li>Added <code>post_migrate</code> hook to run commands after a migration <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1163">#1163 / netpol</a></li> </ul> <h3>Fixed</h3> <ul> <li>Error when generating helper for invokable classes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1124">#1124 / standaniels</a></li> <li>Fix broken ReflectionUnionTypes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1132">#1132 / def-studio</a></li> <li>Relative class names are not converted to fully-qualified class names <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1005">#1005 / SavKS</a></li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/barryvdh/laravel-ide-helper/blob/master/CHANGELOG.md">barryvdh/laravel-ide-helper's changelog</a>.</em></p> <blockquote> <h2>2021-03-15, 2.9.1</h2> <h3>Added</h3> <ul> <li>Generate PHPDoc for Laravel 8.x factories <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1074">#1074 / ahmed-aliraqi</a></li> <li>Add a comment to a property like table columns <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1168">#1168 / biiiiiigmonster</a></li> <li>Added <code>post_migrate</code> hook to run commands after a migration <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1163">#1163 / netpol</a></li> </ul> <h3>Fixed</h3> <ul> <li>Error when generating helper for invokable classes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1124">#1124 / standaniels</a></li> <li>Fix broken ReflectionUnionTypes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1132">#1132 / def-studio</a></li> <li>Relative class names are not converted to fully-qualified class names <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1005">#1005 / SavKS</a></li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/8d8302ff6adb55f8b844c798b8b1ffdee142f7e5"><code>8d8302f</code></a> chore: add missing changelog entries (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1176">#1176</a>)</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/202395b50f31325911fd045993c2b4d2851b8bf6"><code>202395b</code></a> Auto generate models (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1163">#1163</a>)</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/c5c9b2b9517696e1d17ce64790be42b8bb76a4d2"><code>c5c9b2b</code></a> Add comments like table columns. (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1168">#1168</a>)</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/c5e18beff1a2933913f86d56281581566e5c8705"><code>c5e18be</code></a> Fix Relative class names are not converted to fully-qualified class names (FQ...</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/6c4dcd9049e7e9aabc78673de7d1b9d428f3dc10"><code>6c4dcd9</code></a> Merge pull request <a href="https://github.com/barryvdh/laravel-ide-helper/issues/1164">#1164</a> from wilsenhc/readme-fix</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/aaade4eab932efe3841306ad7faf9ef1a8cef391"><code>aaade4e</code></a> Fix typo in README</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/b21324b20e24a3c36bedc184b4cf273bfa0a2d32"><code>b21324b</code></a> Merge pull request <a href="https://github.com/barryvdh/laravel-ide-helper/issues/1156">#1156</a> from ZaidBarghouthi/fix-zb-extra-quote</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/db83fb8b6ba38ea0b1d28197e898b4ff0d9581d0"><code>db83fb8</code></a> Remove extra single quote</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/4f42af1fa1f6265174ef97c36e0ae7beb2596c29"><code>4f42af1</code></a> Apply "<a href="https://github.com/PSR12"><code>@PSR12</code></a>" in php-cs-fixer (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1150">#1150</a>)</li> <li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/f0959c1184c6f8f9e50afa6d9b6c1eb7691ae3ae"><code>f0959c1</code></a> Update changelog and also automagically apply style fix (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1140">#1140</a>)</li> <li>Additional commits viewable in <a href="https://github.com/barryvdh/laravel-ide-helper/compare/v2.9.0...v2.9.1">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
Summary
This PR. allow to generate auto completion for
HasFactory
trait see #1055Type of change
Checklist
composer fix-style