git-hooks.nix

git-hooks.nix copied to clipboard

Tackles #196.

Tasks

• [x] Move hook settings from options.settings.<name> to options.hooks.<name>.settings
• [x] Add package to the hook module
• [x] Add default packages to each hook (see issues)
• [x] Migrate non-hook settings, i.e. settings.rust.
• [x] Add a description to every hook setting
• [x] Use lib.mkDefault on hook definitions to make overrides easier
• [ ] Remove tools? Breaking change 🤔
• [ ] Expose helpers to get all active hooks and their packages
• [x] Migrate any hooks.<name>.settings.package to hooks.<name>.package.
• [ ] Add all hooks to options.hooks so that they appear in the docs.
• [x] Verify generated docs. nix build .#checks.<system>.doc-check.

Issues to tackle

• [ ] Some hooks depend on a custom, more complex package. These are difficult to override.

  • rustftm
  • clippy
  • treefmt

    The `treefmt` package to use.
    
    Should include all the formatters configured by treefmt.
    
    For example:
    ```nix
    pkgs.writeShellApplication {
      name = "treefmt";
      runtimeInputs = [
        pkgs.treefmt
        pkgs.nixpkgs-fmt
        pkgs.black
      ];
      text =
        '''
          exec treefmt "$@"
        ''';
    }
    

• [x] Many hooks have adopted binPath in lieu of package. Should these be deprecated and migrated to package? Here's an interesting case: you can provide a path to the local node_modules folder instead.

  "eslint binary path. E.g. if you want to use the eslint in node_modules, use ./node_modules/.bin/eslint.";

  Solved: I've kept binPath around, but set it to null by default. Hooks will use binPath if it's set, but otherwise default to package.

• [x] There is a special options.settings.rust option that is shared amongst some hooks. It's not a hook itself. We need to define the module type for options.settings. Solved: move to options.settings.rust.

• [x] Some packages already use package as a setting. Need to migrate these. Solved: renamed and deprecated.

• [x] mkRenamedOptionModule isn't showing any warnings. Is it because of the submodules? Solved: added assertions/warnings handling

Misc fixes

• [x] credo: pass the strict flag when credo.settings.strict is enabled.

cc @domenkozar @roberth

Here's where we're currently at:

{
  hooks.ormolu.enable = true;
  hooks.ormolu.package = pkgs.ormolu.override { ... }; # still need to add lib.mkDefault everywhere
  hooks.ormolu.settings.defaultExtensions = [ "lhs" "hs" ];
}

What else do we want to include here? I've outlined what needs to get done above, but there are some bits I don't fully understand, like the non-pre-commit hook stuff.

Many hooks have adopted binPath in lieu of package. Should these be deprecated and migrated to package?

There's lib.getExe that should be used instead.

There is a special options.settings.rust option that is shared amongst some hooks. It's not a hook itself. We need to define the module type for options.settings.

These should be reused by all the hooks that use rust bits.

Many hooks have adopted binPath in lieu of package. Should these be deprecated and migrated to package?

There's lib.getExe that should be used instead.

Nice! Does that always work?

What should we do with hooks like this? It's suggesting to set a local path. Should all hooks be able to override the path to the binary? Is package perhaps not a good enough abstraction for some hooks?

"eslint binary path. E.g. if you want to use the eslint in node_modules, use ./node_modules/.bin/eslint.";

:partying_face: :partying_face: :partying_face:

https://github.com/NixOS/nixpkgs/issues/96006

Looks good to me. Do we need some kind of migration guide or will the warnings system take care of it all (minus the bugs)?

Do we need some kind of migration guide or will the warnings system take care of it all (minus the bugs)?

A good idea either way!

@domenkozar, what do you think of this https://github.com/cachix/pre-commit-hooks.nix/commit/fee04a760a680b573b543bbcabc9dcccbeb4a502? The clippy and rustfmt hooks fit this pattern, but the treefmt one feels a little weird. Maybe we should define hooks.treefmt.settings.formatters = [ pkgs.nixfmt ]?

Nice :) let's ship this.

I think this PR is causing the following issue (specifically with the flake.parts module?):

trace: warning: The option `settings.treefmt.package' defined in `/nix/store/wgw700x1vzpjxak25kkd3s2fk3k6x5m7-source/flake-module.nix' has been renamed to `hooks.treefmt.package'.
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'dotfiles'
         whose name attribute is located at /nix/store/89f8hv2f5yw79zjnbrvibsdmqsnndl97-source/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute '__impureHostDeps' of derivation 'dotfiles'

         at /nix/store/89f8hv2f5yw79zjnbrvibsdmqsnndl97-source/pkgs/stdenv/generic/make-derivation.nix:443:7:

          442|       __propagatedSandboxProfile = unique (computedPropagatedSandboxProfile ++ [ propagatedSandboxProfile ]);
          443|       __impureHostDeps = computedImpureHostDeps ++ computedPropagatedImpureHostDeps ++ __propagatedImpureHostDeps ++ __impureHostDeps ++ stdenv.__extraImpureHostDeps ++ [
             |       ^
          444|         "/dev/zero"

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: The option `perSystem.aarch64-darwin.pre-commit.settings.hooks.treefmt.package' is defined multiple times while it's expected to be unique.

       Definition values:
       - In `/nix/store/wgw700x1vzpjxak25kkd3s2fk3k6x5m7-source/modules/hooks.nix': <derivation treefmt>
       - In `/nix/store/wgw700x1vzpjxak25kkd3s2fk3k6x5m7-source/modules/hooks.nix': <derivation treefmt>
       Use `lib.mkForce value` or `lib.mkDefault value` to change the priority on any of these definitions.

I think this PR is causing the following issue (specifically with the flake.parts module?):

@nifoc, thanks, will fix

Damn, I didn't see the ping.

What happened to enable?

I'm getting error: The option `enable' does not exist. on https://hercules-ci.com/github/hercules-ci/hercules-ci-agent/jobs/2871

@sandydoo do you know?

Not sure I understand where the error is coming from. Like the per-hook enable? It should be there 🤷

@sandydoo I figured it out yesterday: I forgot that I was merging some code of my own into the hook module. With this I can make it work again: https://github.com/cachix/git-hooks.nix/pull/422