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

Implement fixable check #1

Closed
wants to merge 3 commits into from
Closed

Conversation

indykoning
Copy link

I've changed the code up to do a "Dry Run" first and point out all errors.
Using the FixableProcessResultProvider we can promt the user wether they want us to automatically fix it or not.

These changes also automatically add a message asking to review the changes made by pint
image

That screenshot is what is shown when automatic fixing is not enabled,
Otherwise the prompt asking wether to fix the changes is not shown, only the warning to review the changes and commit again.

This pulls the functionality more inline with composer_normalize and phpcs_checker.

However i can understand that what you have currently is how you intend it to use it, thus i leave wether you want to merge this or not up to you.

@JamesHemery
Copy link
Member

Hi @indykoning,

Thanks for your PR !

I think it's a good thing, but this way it drastically changes the current behavior.

With these changes, the files are no longer staged (git add) after the fix. This can be annoying in case of automatic fix during commits because you'll have to make a new a commit.

What do you think about adding an option auto_git_stage to keep this behavior ?

@indykoning
Copy link
Author

Yeah i think that'd be a good way to give people the choice between the 2 different ways of working. I'll work it in!

I will note though that if grumphp finds issues and auto fixes it it will actually prevent the commit from happening e.g. what happens with this change:

I've made a change to the routes/web:
image

I commit the file and let grumphp run
image
Pint mentions the failed style check with a diff of what should've been changed
image

It would ask you wether to fix it automatically (which i've set up to always say yes https://github.com/phpro/grumphp/blob/master/doc/parameters.md#:~:text=fixer.fix_by_default):
image

It cancels the commit and now i have the routes/web.php staged and unstaged
image

The unstaged file is the one with all the automatic fixes, so you can manually review them first with a git diff routes/web.php which excludes your changes (because it's staged) and only includes the automatic fixes.

If you're happy with it you can git add routes/web.php and commit again.
image
image

Only once grumphp runs successfully it will actually do the commit

@indykoning
Copy link
Author

indykoning commented Mar 13, 2023

The auto_git_stage flag has been added so now we're able to toggle between functionalities 🚀
I've also added the option to ignore_patterns which is a pretty powerful function to exclude specific files or folders depending on a pattern, in case a specific file or folder causes trouble (like tests, public, etc.)

@JamesHemery JamesHemery mentioned this pull request Jul 19, 2023
@JamesHemery
Copy link
Member

Hi @indykoning, sorry, I completely forgot about this PR!

I've reworked your implementation to better match our wishes.

In PR #3 I introduce two new configuration variables auto_fix and auto_stage which are by default configured to be activated in pre_commit. I've also copied your configuration variables preset and ignore_patterns.

@JamesHemery
Copy link
Member

Released in 0.0.2, thanks for your work !

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

2 participants