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

Auto generate models #1163

Merged
merged 9 commits into from
Mar 15, 2021
Merged

Auto generate models #1163

merged 9 commits into from
Mar 15, 2021

Conversation

netpok
Copy link
Contributor

@netpok netpok commented Feb 27, 2021

This pull request allows generating model helpers automatically after migrations executed.

Basically a newer version of #842, started from the current master branch.

Based on the comment by barryvdh the config option is simplified it is named post_migrate and it is either false to disable this feature or sets the parameters for the "ide-helper:model" command. It supports both "--nowrite" and the ["--nowrite" => true] format.

The trigger was updated to use the MigrationsEnded event because Laravel will run "incomplete" commands like migrate:roll as migrate:rollback.

The other helper file generation was dropped in favor of composer post update commands.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, I've a question (inline) though, thanks

src/Listeners/GenerateModelHelper.php Show resolved Hide resolved
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mfn mfn requested a review from barryvdh March 13, 2021 09:05
@netpok
Copy link
Contributor Author

netpok commented Mar 15, 2021

I came across a project which uses the spatie/laravel-multitenancy package, thus running the basic artisan ide-helper:generate does not suffice.

Solutions in my mind:

  1. Relaxing the constraint by requiring the full command to be entered, e.g: 'tenants:artisan "ide-helper:models -N" --tenant=1' would run php artisan tenants:artisan "ide-helper:models -N" --tenant=1 and maybe even allowing multiple commands e.g:['artisan command 1', 'artisan command 2', ...]. This solution would mean dropping array parameter compatibility, but I don't see that would be used too much.
  2. Allowing to use a custom action, e.g.: allowing to set a callback which defaults to the current implementation GenerateModelHelper::generateWith(callback); in AppServiceProvider
  3. This is an edge case which the user should handle on it's own, e.g: by overriding the generator listener, which should be possible with the current code by using $app->register(GenerateModelHelper::class, CustomImplementation::class);

@mfn
Copy link
Collaborator

mfn commented Mar 15, 2021

This solution would mean dropping array parameter compatibility, but I don't see that would be used too much.

At least I can say I agree to that assertion :)

I think I would say, the bottom line for all of this would be improved DX for users of this library. Seems only 1) really fulfills this, once you "have to code" I think it gets worse. Also creates more dependencies on the internal code, which we might rather change one day…

@netpok
Copy link
Contributor Author

netpok commented Mar 15, 2021

I pushed a commit which implements option 1.

@netpok netpok requested a review from mfn March 15, 2021 13:16
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment, otherwise LGTM!

@@ -32,6 +35,13 @@ class IdeHelperServiceProvider extends ServiceProvider implements DeferrableProv
*/
public function boot()
{
if ($this->app['config']->get('ide-helper.post_migrate', false)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the other change and the clarification! I think for consistency, we also should default to [] here now:

Suggested change
if ($this->app['config']->get('ide-helper.post_migrate', false)) {
if ($this->app['config']->get('ide-helper.post_migrate', [])) {

Copy link
Contributor Author

@netpok netpok Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought leaving this as false since its a condition but ok, changed it.

Oh, I could have just accepted the change here...

@mfn
Copy link
Collaborator

mfn commented Mar 15, 2021

@barryvdh IMHO good to merge!

@barryvdh barryvdh merged commit 202395b into barryvdh:master Mar 15, 2021
mfn added a commit that referenced this pull request Mar 15, 2021
@mfn mfn mentioned this pull request Mar 15, 2021
9 tasks
barryvdh pushed a commit that referenced this pull request Mar 15, 2021
* chore: mention the fix from #1005

* chore: mention #1163
kodiakhq bot referenced this pull request in WesleyKlop/vote-system Mar 16, 2021
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 &quot;<a href="https://github.com/PSR12"><code>@​PSR12</code></a>&quot; 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 />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=barryvdh/laravel-ide-helper&package-manager=composer&previous-version=2.9.0&new-version=2.9.1)](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>
fatihdirikman added a commit to fatihdirikman/Laravel-IDE-Helper that referenced this pull request Jan 7, 2022
renaforsberg824 added a commit to renaforsberg824/ide-helper-laravel-developer that referenced this pull request Oct 5, 2022
lisadeloach63 added a commit to lisadeloach63/ide-helper-reso-laravel that referenced this pull request Oct 7, 2022
sadafrangian3 pushed a commit to sadafrangian3/ide-helper-laravel that referenced this pull request Oct 18, 2022
smile1130 added a commit to smile1130/laravel-IDE that referenced this pull request Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants