search

macisamuele / language-formatters-pre-commit-hooks

Collection of custom pre-commit hooks.
Apache License 2.0
112 stars 56 forks source link

autofix with exit code 0 (if successful) #188

Closed brodybits closed 3 months ago

brodybits commented 9 months ago

I would personally favor this way thanks!

macisamuele commented 9 months ago

@brodybits can I ask you to provide context on why this change is reasonable?

The reason why the tool returns failures if autofix fixed the code is to still signal, to pre-commit hook, that the files needed handling. This way the author will need to re-accept/apply the provided changes in order to be able to commit

brodybits commented 8 months ago

I found a couple of these tools (pretty-format-ini & pretty-format-toml) to be useful in a project that is NOT part of a GitHub build, in 2 different places:

I think and hope this should be clear. Yes we can very easily do something like add | true at the end to ignore the non-zero exit code when auto-formatting. But I think it would be nicer if we can get these updates.

Thanks for your consideration.

brodybits commented 8 months ago

Hey is there anything we can do to help move this forward?

I really hope I won't have to fork this to get this resolved.

Thanks in adavance!

macisamuele commented 8 months ago

The main target for this tool is the integration in pre-commit. Having autofix return exit status 0, will imply that pre-commit will consider the file well formatted and as such the staged changes will be committed.

This will defeat the most important component of pre-commit. This because as after the commit, the format-fixes won't be committed but rather present as unstaged changes on the repository. Essentially, we committed not pretty file and then we should eventually recommit (or commit amend) the fix of format.

For this very reasons I would be opposed to the idea to have exit status 0 in case a fix was placed.

If we were to make this optional via a cli argument (but this needs to be done across all the available tools) then I might consider this, but changing the default is a no go.

Furthermore, you presented a case where you would like to run the tool within lints to format code... I would highly recommend that the developer need to be notified about "automated" changes happened to the files is required (remember automatic formatting should not change the semantic of the code, but bug might exists). (The comment above is very high level as I don't know your environment and as such it might also be not correct).