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

Make hook installation optional #261

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Contributor

@roberth roberth commented Mar 17, 2023

A highly opinionated workflow doesn't work for all projects, especially ones that already have a long history, notably NixOS/nix. Some people hate pre-commit with a passion.

These things should not be showstoppers.

To quote the docs:

Opting out, opting in

Users can enforce their own preference by setting NIX_PRE_COMMIT_HOOKS_INSTALL to 0 or 1 in their environment, e.g. in .profile or in home.sessionVariables for home-manager users.

Furthermore, project maintainers can set a default preference via the installByDefault option.

Finally, regardless of these settings, you may install the hooks manually by running nix-pre-commit-hooks-install in your shell. This does what it says, and the shell hook will detect that the hooks exist and keep them up to date.

@domenkozar
Copy link
Member

I'm confused why this is needed, can't you make the call to pre-commit run optional based on a boolean?

@roberth
Copy link
Contributor Author

roberth commented Apr 11, 2023

I don't think run is responsible for installation.

The goal is to let a project choose not to install any hooks into .git by default, unless the devShell user has opted in.
I think something might done at the project level, but it won't be quite as user friendly, as it doesn't standardize this installation logic.

@roberth
Copy link
Contributor Author

roberth commented Apr 14, 2023

I've re-done it. This time it just (cleanly) refactors the existing logic, so that it can be called without installing the hooks into .git.
Solved some tech debt along with that. We now have shellcheck on the script.

Comment on lines +25 to +26
# `types_or = [ ... something ... ]` doesn't pick up our .sh file
shellcheck.types_or = nixpkgs.lib.mkForce [ ];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't have been necessary, but that's a different problem; not for this PR.

@phip1611
Copy link
Contributor

@roberth I'd love to have this. Are you willing to refresh your PR?

@domenkozar
Copy link
Member

Sorry for the mess, we revamed module structure in #397, could you port these changes over?

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