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 default pre-commit hooks #401

Merged
merged 10 commits into from
Apr 1, 2024

Conversation

totoroot
Copy link
Collaborator

@totoroot totoroot commented Feb 22, 2024

This PR adds almost all non-deprecated hooks from the original pre-commit-hooks repository.

Two of these hooks were asked for in #31 by @PierreR and workarounds have been proposed, like writing custom hooks, or using the rawConfig option.

Thanks to @kalbasit, pre-commit-hooks got packaged for nixpkgs, so why not add all these default pre-commit hooks that are listed here?

I've tested them all, set some sane defaults and occasionally renamed them slightly or changed the description, so it is clearer, what the individual hooks are used for. The rest of the settings were taken from the hooks configuration in the original repository.

All the hooks seem to pass the initial checks and can be used just fine. I haven't added all the arguments for all hooks, but tried to add all that are necessary for sensible usage as a pre-commit hook.

In case someone tries to use one of these hooks and cannot find a needed option that is available in the original hook, please open an issue and feel free to assign me to it. I'll try to add it then.

I have yet to add all hooks to the README. Keeping this as a draft until I get around to it, which should be at the beginning of next week. Feel free to review the proposed changes in the meantime.

@totoroot totoroot force-pushed the add-default-pre-commit-hooks branch 2 times, most recently from 9edc7f6 to 7bb2c65 Compare February 22, 2024 15:09
@totoroot totoroot self-assigned this Feb 22, 2024
@totoroot
Copy link
Collaborator Author

Closure size of pre-commit-hooks is rather small:

λ du -sh $(nix-build -A python3Packages.pre-commit-hooks)
860K	/nix/store/g07vbn53mvqy5gaqa3dmw4w0lcr2i89a-python3.11-pre-commit-hooks-4.5.0

@totoroot totoroot force-pushed the add-default-pre-commit-hooks branch 4 times, most recently from 2523017 to 3e3c144 Compare February 27, 2024 10:34
@totoroot
Copy link
Collaborator Author

I have now listed all added default pre-commit hooks to the corresponding sections of the README.

Since I noticed a couple of formatting issues in the README, I fixed them in 9593b40, so that it is now standard Markdown as checked by mdl and markdownlint. I also sorted all hooks alphabetically in the subsections.

Since the various hooks includes hooks other than formatters, I renamed the section's headline from Other formatters to Various other hooks.

@domenkozar Should we add mdl to the pre-commit hooks in this repository so that the README gets linted on new contributions?

@totoroot totoroot marked this pull request as ready for review February 27, 2024 10:54
@totoroot totoroot requested a review from domenkozar February 27, 2024 10:54
@domenkozar
Copy link
Member

Oh thanks for all this work! We'll have to base it on top of #397 otherwise we'll get into conflicts.

@totoroot totoroot changed the title Add pre-commit default hooks Add all default pre-commit hooks Feb 27, 2024
@totoroot totoroot changed the title Add all default pre-commit hooks Add default pre-commit hooks Feb 27, 2024
@totoroot
Copy link
Collaborator Author

Oh thanks for all this work! We'll have to base it on top of #397 otherwise we'll get into conflicts.

Sure, I'll try to rebase onto that.

@totoroot totoroot force-pushed the add-default-pre-commit-hooks branch from 9593b40 to b9fadf8 Compare March 6, 2024 14:49
@totoroot
Copy link
Collaborator Author

totoroot commented Mar 6, 2024

@domenkozar Alright, my changes are now on top of #397.

@domenkozar domenkozar changed the base branch from master to new-modules March 6, 2024 15:02
@domenkozar
Copy link
Member

cc @sandydoo

@totoroot totoroot force-pushed the add-default-pre-commit-hooks branch from b9fadf8 to e9b5ead Compare March 7, 2024 12:40
@totoroot
Copy link
Collaborator Author

totoroot commented Mar 7, 2024

Some braces were missing, but now it should work fine.
@sandydoo Please let me know if you need any help finishing up #196.

@sandydoo
Copy link
Member

sandydoo commented Mar 7, 2024

Great work, @totoroot!

RE #196, there are two main blockers I'd like to solve before merging:

  1. We'd like to offer an overridable package for every hook. The problem is that a few hooks have more complex requirements: some hooks depend on several packages or have wrappers of some kind.
    How much do we expose here? Should we expose the wrappers to allow users to override them? Or is this an implementation detail?
    Should package be a single package type or some attribute set of packages? If it's a single package (or a symlink join), can we reliably access all of the packages used by the enabled hooks to create a dev shell?
  2. The deprecation notices for the renamed options don't currently show up. Maybe there's some trick to get these to propagate through the submodules.

If you have any ideas/thoughts on these, I'd love to hear them!

@domenkozar
Copy link
Member

domenkozar commented Mar 8, 2024

  1. we discussed about this one quite a bit, most likely it's best to have cfg.package and allow it to be a list of packages that is then used with pkgs.symlinkJoin
  2. should be doable like so: https://github.com/NixOS/nixpkgs/pull/287506/files#diff-346cfce0bc66f4470c59870b415a1582bd5422b09d67309feb20e9e8597d2f7bR899

@domenkozar
Copy link
Member

Hey! We merged it, could you rebase on top of master now?

@totoroot totoroot force-pushed the add-default-pre-commit-hooks branch from e9b5ead to 87719d2 Compare March 27, 2024 11:18
@totoroot totoroot changed the base branch from new-modules to master March 27, 2024 11:18
@totoroot totoroot force-pushed the add-default-pre-commit-hooks branch from 87719d2 to b9fadf8 Compare March 27, 2024 11:31
@totoroot
Copy link
Collaborator Author

Yesterday I messed up the large rebase a little, so many thanks to @9999years who fixed up the branch 💐

@domenkozar Ready for review/merge now!

@9999years
Copy link
Contributor

git-annex seems to not build on macOS, maybe we should disable it...?

@totoroot
Copy link
Collaborator Author

git-annex seems to not build on macOS, maybe we should disable it...?

@9999years Surely a good idea, but that should be done in a separate PR, as this one did not change the git-annex hook.

@9999years
Copy link
Contributor

Wait, the CI failures are non-blocking -- CI hasn't passed on the master branch in 2 months: 0b6fdb0

@9999years
Copy link
Contributor

Opened #421 to fix the CI failures on macOS.

@domenkozar
Copy link
Member

Could you rebase on master?

@totoroot
Copy link
Collaborator Author

Could you rebase on master?

I merged master into this branch.

@domenkozar domenkozar merged commit ffbf469 into cachix:master Apr 1, 2024
4 checks passed
@sandydoo sandydoo mentioned this pull request Apr 4, 2024
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.

4 participants