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

feat: Add a --with-exit-status flag #270

Merged

Conversation

dshafik
Copy link
Contributor

@dshafik dshafik commented Apr 25, 2024

This adds a small feature, --with-exit-status that will exit with either 1 or 0 if there are changed files or not. Essentially this is the same as running with the --test flag except that it also changes the files.

This is helpful for tools such as Captain Hook (or really any pre-commit hook) which rely on the exit code to know if there were changes, so that it will fail and allow amending the commit manually.

it should be noted that while it seems the prevailing consensus is that you should just run pint and then automatically add the changed files to the commit, however if you have additional working changes (e.g. you only staged part of the file, or made changes after staging it) then those changes would now be inadvertently included in the commit.

Happy to add tests if this is a feature you feel would be worth contributing.

Docs PR: laravel/docs#9606

@@ -43,6 +43,7 @@ protected function configure()
new InputOption('dirty', '', InputOption::VALUE_NONE, 'Only fix files that have uncommitted changes'),
new InputOption('format', '', InputOption::VALUE_REQUIRED, 'The output format that should be used'),
new InputOption('cache-file', '', InputArgument::OPTIONAL, 'The path to the cache file'),
new InputOption('with-exit-status', '', InputOption::VALUE_NONE, 'Only fix files that have uncommitted changes'),
Copy link
Member

Choose a reason for hiding this comment

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

I think the description here might be incorrect (copied and pasted from a few lines above?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, didn't even see that. I'll fix that up and write any relevant tests as there seems to be interest in this feature.

@driesvints
Copy link
Member

Hey @dshafik, thanks for the PR! I'll mark this as draft for now as you're still working on this. Feel free to mark as ready when you need another review.

@driesvints driesvints marked this pull request as draft April 26, 2024 07:25
@dshafik dshafik marked this pull request as ready for review April 26, 2024 09:07
@dshafik
Copy link
Contributor Author

dshafik commented Apr 26, 2024

@driesvints Fixed up the one issue and added tests, this is ready for review now 🙏🏻

app/Commands/DefaultCommand.php Outdated Show resolved Hide resolved
@taylorotwell taylorotwell merged commit c3855d3 into laravel:main Apr 26, 2024
6 checks passed
@taylorotwell
Copy link
Member

Thanks!

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.

4 participants