language-formatters-pre-commit-hooks icon indicating copy to clipboard operation
language-formatters-pre-commit-hooks copied to clipboard

autofix with exit code 0 (if successful)

Open brody4hire opened this issue 1 year ago • 4 comments

I would personally favor this way thanks!

brody4hire avatar Oct 11 '23 17:10 brody4hire

@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

macisamuele avatar Oct 12 '23 02:10 macisamuele

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:

  • lint checking, where we check that the formatting is OK or not
  • auto-formatting with the --autofix option, where we would want to continue formatting other files in case we DID have to reformat at one point

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.

brody4hire avatar Oct 26 '23 04:10 brody4hire

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!

brody4hire avatar Oct 31 '23 03:10 brody4hire

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).

macisamuele avatar Nov 04 '23 13:11 macisamuele