`typos` does not respect `pre-commit`s expected behaviour

[hmellor]

Description

We are currently using the typos pre-commit hook in https://github.com/vllm-project/vllm. Overall it's been a good experience. However, it does not behave how a pre-commit hook should.

If a pre-commit hook makes changes it is expected that it will:

• exit with a non-zero exit status so that pre-commit sees it as failed
• log the changes that it has made

The pre-commit hook provided by typos does neither of these things. This makes it dangerous to use because it will silently make changes without ever informing the user what changes it actually made.

For this reason we have had to remove --write-changes. If the pre-commit hook could behave properly, we would very much like to add it back.

[epage]

Do you have a link to documentation on these pre-commit hook expectations?

[hmellor]

Yes, https://pre-commit.com/#creating-new-hooks

The hook must exit nonzero on failure or modify files.

[epage]

What about

log the changes that it has made

[epage]

The hook must exit nonzero on failure or modify files.

So if I'm understanding this correctly, when making changes the expected behavior is for the operation to fail and for the user to try again? And at least in this scenario, output is important to explain why it failed.

For typos, this would be a breaking change to start erroring when something is fixed. To isolate this behavior to the pre-commit hook would require us to both provide the information in a programmatic way and to then access it in a programmatic way. Not thrilled with a flag to control this behavior.

[hmellor]

What about

log the changes that it has made

I suppose this part isn't mandated in the docs but all the hooks I use explain why they failed.

[hmellor]

The process is normally as follows:

• the hook makes changes, fails, the user sees the reason printed (pre-commit will show the printed output then the exit status is non-zero)
• the failure means that the changes remain unstaged and the user can clearly see what the hook fixed
• the user can then choose to accept the changes, commit them, and the hook will now pass

[Rolv-Apneseth]

Could a change/addition to typos --diff achieve the desired effect? It wouldn't actually make the changes, but using cargo fmt -- --check doesn't make changes either.

[epage]

@Rolv-Apneseth the default behavior of typos is the same as cargo fmt --check, do not change and error. --write-changes is an opt-in to modify. Our pre-commit hook defaults to --write-changes which you can override.

[Rolv-Apneseth]

@Rolv-Apneseth the default behavior of typos is the same as cargo fmt --check, do not change and error. --write-changes is an opt-in to modify. Our pre-commit hook defaults to --write-changes which you can override.

Ah, my bad. And you don't want to have an additional flag to modify the returned exit code, right?

[epage]

Yes, I called that option out in #1399 (comment)

[Rolv-Apneseth]

Yes, I called that option out in #1399 (comment)

Yes, sorry, just wasn't sure that's what you meant.

In that case, it may be a little messy but how would you feel about some kind of wrapper script for the pre-commit hook that runs typos twice (if there are changes to be made), for example:

output=$(typos)
retv="$?"

if [ "$output" != "" ]; then
    echo "$output"
fi

typos --write-changes && exit "$retv"

[hmellor]

@Rolv-Apneseth that's a pretty good suggestion. This should behave how pre-commit expects without needing to change the actual behaviour of typos