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

add config option to remove the overwrite models file prompt #1224

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

powellblyth
Copy link

Summary

This adds a config option called `` that removes the Do you want to overwrite the existing model files? prompt and always answers "yes" when running the command.
This is backwards compatible, as omitting the config or setting it false leaves exactly as it was

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@mfn
Copy link
Collaborator

mfn commented May 21, 2021

Can't the same achieved with passing --write or what is the difference?

@powellblyth
Copy link
Author

Can't the same achieved with passing --write or what is the difference?

Yes it can be achieved that way, but you have to remember the command line switch (or teach your team). Consistent behaviour can be achieved through config.

I wrote this for laziness, but imagine an agency where several different projects had several different standards for where this meta should go, this would help ensure that updating the model was the preferred path.

@mfn
Copy link
Collaborator

mfn commented May 21, 2021

TBH instead of adding "yet another config" option for such a opinionated thing I would suggest using a script definition to run to achieve this, there are so many easy options (composer.json, Makefile, phing).

Personally I also can't remember them and probably neither does any in my team maybe even know the exact nature of the commands and args. We just have exactly this and no issue with; it's phing in my case:

  <target name="ide-helper" description="Generate _ide_helper.php">
    <!-- ensure cache is empty before running code -->
    <exec command="${artisan} clear-compiled" passthru="true"/>
    <exec command="${artisan} ide-helper:generate" passthru="true"/>
    <exec command="${artisan} ide-helper:models --write --reset" passthru="true"/>
    <exec command="${artisan} ide-helper:meta" passthru="true"/>
    <!-- ensure we did't leave a cache file from running the ide-helper -->
    <exec command="${artisan} clear-compiled" passthru="true"/>
  </target>

But that's just my 2c

@powellblyth
Copy link
Author

TBH instead of adding "yet another config" option for such a opinionated thing I would suggest using a script definition to run to achieve this, there are so many easy options (composer.json, Makefile, phing).

Personally I also can't remember them and probably neither does any in my team maybe even know the exact nature of the commands and args. We just have exactly this and no issue with; it's phing in my case:

  <target name="ide-helper" description="Generate _ide_helper.php">
    <!-- ensure cache is empty before running code -->
    <exec command="${artisan} clear-compiled" passthru="true"/>
    <exec command="${artisan} ide-helper:generate" passthru="true"/>
    <exec command="${artisan} ide-helper:models --write --reset" passthru="true"/>
    <exec command="${artisan} ide-helper:meta" passthru="true"/>
    <!-- ensure we did't leave a cache file from running the ide-helper -->
    <exec command="${artisan} clear-compiled" passthru="true"/>
  </target>

But that's just my 2c

a good 2c, or in my case about £0.015

For future historians, I've added

    "ide-helper": "./vendor/bin/sail artisan ide-helper:models --write"

to my composer-json file and can run this using composer ide-helper

You are right it's very opinionated, but I suppose that's why I put it into config so as not to force my opinion.
It does solve my use case. I'll leave the PR here a little longer in case someone needs it, unless someone tells me it definitely won't be merged.

Thanks for the tip, I had not looked at composer scripts before.

Copy link

@Messhias Messhias left a comment

Choose a reason for hiding this comment

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

I tested locally with provided example and worked without issues, but I guess could have more examples.

But I don't see the reason also to have a lot tests since this one passed through without breaking the current package (y).

@mfn
Copy link
Collaborator

mfn commented May 26, 2021

I'm 👎 adding a separate config for which is already a switch available which can be added to any kind of scripting tool to share in a team.

@barryvdh
Copy link
Owner

barryvdh commented Aug 4, 2021

Yeah bit conflicted. I only ever use the --write flag, but I feel it's kinda breaking BC, that's why I added the prompt.

@Messhias
Copy link

Messhias commented Aug 4, 2021

Yeah bit conflicted. I only ever use the --write flag, but I feel it's kinda breaking BC, that's why I added the prompt.

So it's better to we update the PR or exclude it?

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.

None yet

4 participants