Skip to content

Make hook installation optional #261

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

Make hook installation optional #261

wants to merge 14 commits into 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.

@roberth roberth mentioned this pull request Mar 17, 2023
Merged
7 tasks
@roberth roberth force-pushed the optional-install branch from 212f93c to 6c08a5c Compare April 7, 2023 15:05
@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 added 14 commits April 14, 2023 15:40
@roberth
installationScript: Refactor interpolations into shell variables
6026791
@roberth
installationScript: Move the logic to a separate file
ec00047
@roberth
pre-commit-install.sh: Factor out git dir retrieval
671ca3e
@roberth
pre-commit-install.sh: Extract _pre_commit_hooks_nix_install_stages()
45c6ee4
@roberth
pre-commit-install.sh: Extract _pre_commit_hooks_nix_is_config_up_to_…
a67b8d5 
…date()
@roberth
pre-commit-install.sh: Extract _pre_commit_hooks_nix_ensure_config_fi…
3e301e1 
…le_up_to_date()
@roberth
pre-commit-install.sh: Use $_pre_commit_hooks_nix_local_config_file i…
e2cffc6 
…nstead of $GIT_WC
@roberth
pre-commit-install.sh: Adding missing quotes
c88274d
@roberth
pre-commit-install.sh: Add _pre_commit_hooks_nix_install_config_file_…
bd99ccd 
…only
@roberth
pre-commit-install.sh: Report the right message when updating the con…
c228640 
…fig file
@roberth
nix/default.nix: Use ShellCheck
e9aaca9 
pre-commit did not pick up on the .sh extension because of types_or.
This should be fixed properly somehow, but that's out of scope for
this commit and branch.
@roberth
pre-commit-install.sh: Document interface
ab17ee8
@roberth
pre-commit-install.sh: Fix and improve global variable error
152c6e5
@roberth
Add environmentSetupScript
af4ad11
@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.

roberth
roberth commented Apr 14, 2023
View reviewed changes
nix/default.nix
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.

@roberth roberth mentioned this pull request Jun 8, 2023
Open
12 tasks
@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?

@domenkozar
Copy link
Member

Closing as outdated

@domenkozar domenkozar closed this Jul 11, 2024
@phip1611
Copy link
Contributor

Is the functionality request outdated or just the state of the PR?

Is there a replacement?

@domenkozar
Copy link
Member

I think having enable option would make this a lot simpler, I'm happy to merge it if someone makes a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@roberth @domenkozar @phip1611