Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lefthook doesn't block commits when using a --fix or --write option. #440

Closed
ava-mc opened this issue Mar 1, 2023 · 3 comments
Closed
Labels
bug Something isn't working

Comments

@ava-mc
Copy link

ava-mc commented Mar 1, 2023

🔧 Summary

When adding linting checks for pre-commit (e.g. prettier, eslint, markdownlint) using the --fix option, the commit still goes through and fixes the file afterwards.

Lefthook version

Using lefthook version 1.3.2

Steps to reproduce

Commit file with fixable linting error, when having a linting check in pre-commit with the --fix option.

Expected results

If a file is automatically fixed by resolving the linting issues, I would want my commit to still be blocked, but fix the file.

Only way to circumvent this for the moment: do 2 separate checks: one purely for verifying, and one for fixing. However, this decreases the speed of the pre-commit checking a lot.

Actual results

Commit with linting mistakes still goes through, while file is fixed.

Possible Solution

Either on the side of the specific packages: add error code when fixes are executed
For lefthook: keep track when fix has happened and give the option to block commits in these types of cases.

(Specifically for prettier: there is a package pretty-quick that gives the option to --bail when a fix happens and still block the commit, but such a solution is not available for each linter.)

@ava-mc ava-mc added the bug Something isn't working label Mar 1, 2023
@mrexox
Copy link
Member

mrexox commented Mar 2, 2023

Hey @ava-mc! It's a good idea but I have concerns about the solution. The reason lefthook doesn't block commits is - the linter's return code is 0, so it finishes successfully. Many people want lefthook to behave smoothly and add changes for fixed files automatically.

I can recommend you the following configuration to get what you want:

pre-commit:
  commands:
    lint:
      # it looks like a hack but it makes sense that commands should fail (return non-0) to fail the hook
      run: npm run lint || (npm run lint --fix && exit 1)

Or if you want to automatically add fixed files to the commit, use the following:

pre-commit:
  commands:
    lint:
      run: npm run lint {staged_files} --fix && git add {staged_files}

I am going to add an auto-add changes files option, but still can't decide on naming. Anyway, please write if this works for you.

@pvds
Copy link

pvds commented Mar 12, 2023

@mrexox great that you're planning to add an "auto-add changed files" option.

I would suggest that as long as #64 has not been implemented/merged the documentation is updated with your recommendations.

@pvds
Copy link

pvds commented Aug 25, 2023

@mrexox I guess this issue can be closed since #64 is completed, allowing changed files to be added with https://github.com/evilmartians/lefthook/blob/master/docs/configuration.md#stage_fixed

@mrexox mrexox closed this as completed Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants