feat: fail on fix by default

[movabo]

Rationale behind each of the changes:

• fix = true: autofix now changed to whether the fixes are automatically applied and commited, which then next line then defines: fail_on_fix = !autofix
• Removing stash = "git": That option, together with (implicit) stage = true had the effect that the autofix changes were directly staged to the commit. The stash, however seemed to be empty when testing locally, making it hard to verify what the autofix actually did. Therefore I changed it to the recommended behavior (by deleting this line and adding)
• stage = false: This seems to me the most intuitive behavior. Now, no changes are made to the staged code. Changes by autofix are directly visible to with git diff and need to be added manually (but I'd argue that should simply be part of the workflow).

[stempler]

Trying to understand what effect the changes will have:

Default workflow

New default workflow would be to apply fixes, but fail the commit so the changes can be reviewed.

If you have unstaged changes when committing, the fixes will also run on these, meaning compared to before changes that are not intended to be committed yet are processed, including potential work in progress.
The documentation doesn't mention that this also applies to checks, but could also be an oversight in the docs and would need to be tested. If it is the case it can have the effect that a commit is rejected based on unstaged changes.

The previous behavior though is similar if there is a check failing that requires running the fix. Running hk fix after the failed commit uses the default settings (stage=false,stash=none) and thus has the same effect related to applying the fix.

TLDR: It should be tested if checks run on unstaged changes and can present commits.

I'll try that out to check the behavior.

Autofix workflow

Automatically apply fixes and commit.

I think for this we would still want stage and stash settings as before, so fixes are included in the commit (stage) and unstaged changes are not processed. What do you think @movabo ?

[movabo]

If you have unstaged changes when committing, the fixes will also run on these, meaning compared to before changes that are not intended to be committed yet are processed, including potential work in progress.

Yes.

The documentation doesn't mention that this also applies to checks, but could also be an oversight in the docs and would need to be tested. If it is the case it can have the effect that a commit is rejected based on unstaged changes.

That's true.

TLDR: It should be tested if checks run on unstaged changes and can present ^{(I guess you meant "prevent")} commits.

From my test that I just did this seems true.

Unfortunately, it seems that the combination of fix = false, fail_on_fix = true, and stash = "git" doesn't seem to work. For me, it did not fix a simple indentation fix. So from what I gathered that means we have the following three options:

1. Keep the current behavior
2. Use the behavior in this commit, meaning that a non-staged file could prevent a commit and that non-staged WIP files could be modified
3. Use behavior stage = true and stash = "git", meaning that it'd be hard to find out which code was modified by the fix-step

After further investigating, I find case 2 to be the worst case: It can prevent commits even though the code to be commited is fine.
Personally, I'd say case 3 is the most favorable, because honestly: When I see that spotless does not work, I'll run spotlessApply and that's it anyway. As long as we can make sure that the fix command does not break code (which for me is the case for simple code formatting), I'd be for case 3.

[stempler]

What speaks against fail_on_fix = true with stage = false for the default workflow?
According to the docs for fail_on_fix this is how it is intended to be used.
You said

Removing stash = "git": That option, together with (implicit) stage = true had the effect that the autofix changes were directly staged to the commit

but I did not find any hint that the stash and stage settings are somehow dependent.

[movabo]

What speaks against fail_on_fix = true with stage = false for the default workflow?

That's definitely a fourth possibility I missed mentioning. I dislike this configuration because then again, unstaged changes could prevent a commit.
I think what I dislike quite a bit is not using the staged option and unfortunately, that option does not work with stashing.

[stempler]

What speaks against fail_on_fix = true with stage = false for the default workflow?

Ok, tested this now and this basically results in the fix not being applied, so no difference really to the current default.

Option 1 would be the safe option, where the fixes are applied manually (e.g. hk fix) and it can be checked then what exactly is changed. The changes can be reverted if needed and the configuration adapted to suppress unwanted behavior.

Option 3 for me is very much like the current "autofix" option with the only difference being that the commit is not created in case a fix is applied, but there is also no possibility to see what was changed exactly. So I would not see an advantage in it over the current "autofix" approach.

I think all-in-all I would tend to stick with the current behavior due to the transparency. In my experience so far it was OK to need to run hk fix from time to time and not really something that bothered me.
AI could be instructed via conventions or hooks to apply formatting automatically (e.g. use hk fix if hk.pkl is present) to cover the output generated by it.

What do you think @movabo ?