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

Add check_phase to pre-commit settings #415

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joedevivo
Copy link

The existing behavior of pre-commit-hooks.nix is to use the pre-commit phase of pre-commit hooks, unless your default_phases = [ "manual" ]. On further inspection I found that it has to be only manual. If default_phases = [ "manual" "push" ], it will use commit. In my particular case, I wanted default_phases = [ "push" ], which then ran commit hooks when I ran nix flake check.

This PR adds check_phase to the configuration. Setting check_phase = "push" runs the pre-push phase checks when I run nix flake check no matter what my default_phases are.

@sandydoo
Copy link
Member

sandydoo commented Apr 1, 2024

On further inspection I found that it has to be only manual. If default_phases = [ "manual" "push" ], it will use commit. In my particular case, I wanted default_phases = [ "push" ], which then ran commit hooks when I ran nix flake check.

@joedevivo, if I understand correctly, the issue you were having was that, after setting default_phases = ["manual" "push"], the pre-commit hooks still ran before each commit. Is that correct?
This sounds like a bug, which might be fixed #423.

As far as the feature itself goes, the manual stage/phase we had before feels like the appropriate choice for nix flake check. Even if we expose this option (and I'm not entirely sure what the use-case for that would be), the default should remain manual.

@m1-s
Copy link
Contributor

m1-s commented Apr 1, 2024

Added the mentioned test cases to the new test suite https://github.com/cachix/git-hooks.nix/pull/423/files#diff-6c8d597050bd88f356507ec90b216be2fcd69fce6e488c62060f12bae96deddeR37-R51

Indeed my PR appears to fix the mentioned issue already. Let me know if I misunderstood anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants