search

wearejust / grumphp-extra-tasks

A set of extra tasks for grumphp
MIT License
23 stars 6 forks source link

Use non-blocking result in case if found code style issues #3

Closed PatchRanger closed 7 years ago

PatchRanger commented 7 years ago

Problem

In case if PHP-CS-Fixer finds code style issue, it becomes failed rejecting the commit. It is misleading as auto-fixing should automatically deal with it.

Reason

We should return non-blocking failed result. It means that we show the result is bad - but nevertheless we found a way to automatically fix it and commit was approved.

Solution

I've just correct that behavior: after the 2nd run (which is actually fixing, following the 1st run (which is just scanning)), we return non-blocking failed result with all the context and errors from the 1st run. I've checked locally - it does the job perfectly.

ceesvanegmond commented 7 years ago

@PatchRanger Hmmm maybe add an configuration option for this? I think alot of developers would want to see the changes to the files by the CS fixer. Could you add an configurable option for this? Then i'll merge it.

PatchRanger commented 7 years ago

I think alot of developers would want to see the changes to the files by the CS fixer.

@ceesvanegmond They are present: that's why we need to pass context & errors from the 1st run to the non-blocking failed result - to see all the changes. Please keep in mind, that such behavior still only working with configuration option diff: true set - nothing changed at all.

Could you add an configurable option for this?

I wouldn't consider this PR as adding new enhancement - in my view, it's a bugfix. Instead of rejecting failed commit, it returns non-blocking failed result. In case if diff: true set, it also shows all of the fixes done by PHP-CS-Fixer - is that what you're requesting configurable option for? Thus I don't think such behavior is a matter of choice - that is how it should be working and it shouldn't be configurable/switchable.

Did I repond to your remarks?

ceesvanegmond commented 7 years ago

@PatchRanger Thanks for the explanation, totally agree on you with this. Is OK with you to tag is at v1.3? Let me know.

Thanks for the pull request 👍

PatchRanger commented 7 years ago

@ceesvanegmond Yes, it's ok, I wouldn't mind. Thank you for your time.

ceesvanegmond commented 7 years ago

@PatchRanger See release v1.3