Move to new module structure #397

sandydoo · 2024-02-10T23:54:32Z

Tackles #196.

Tasks

Move hook settings from options.settings.<name> to options.hooks.<name>.settings
Add package to the hook module
Add default packages to each hook (see issues)
Migrate non-hook settings, i.e. settings.rust.
Add a description to every hook setting
Use lib.mkDefault on hook definitions to make overrides easier
Migrate any hooks.<name>.settings.package to hooks.<name>.package.
Verify generated docs. nix build .#checks.<system>.doc-check.

Out of scope

Remove tools? Breaking change 🤔
Expose helpers to get all active hooks and their packages
Add all hooks to options.hooks so that they appear in the docs.

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 "$@"
    ''';
}

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.
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: moved to options.settings.rust.
Some packages already use package as a setting. Need to migrate these.
Solved: renamed and deprecated.
mkRenamedOptionModule isn't showing any warnings. Is it because of the submodules?
Solved: added assertions/warnings handling

Misc fixes

credo: pass the strict flag when credo.settings.strict is enabled.

sandydoo · 2024-02-11T00:12:29Z

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.

domenkozar · 2024-02-11T11:41:21Z

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.

domenkozar · 2024-02-11T11:44:27Z

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.

sandydoo · 2024-02-11T13:07:47Z

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.";

domenkozar · 2024-02-12T08:18:51Z

🥳 🥳 🥳

domenkozar · 2024-02-15T17:38:00Z

NixOS/nixpkgs#96006

Based on #196.

domenkozar · 2024-03-12T07:09:18Z

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)?

sandydoo · 2024-03-15T00:44:53Z

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 fee04a7?
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 ]?

domenkozar · 2024-03-15T04:06:56Z

Nice :) let's ship this.

nifoc · 2024-03-19T13:43:26Z

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.

sandydoo · 2024-03-19T16:17:46Z

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

@nifoc, thanks, will fix

roberth · 2024-03-27T11:53:05Z

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

domenkozar · 2024-04-01T06:43:00Z

@sandydoo do you know?

sandydoo · 2024-04-01T07:04:57Z

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

roberth · 2024-04-01T07:06:30Z

@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: #422

- Remove `before` and `after` from the `raw` config. These are not valid pre-commit configuration options and should not be here. - Remove code to filter out `before` and `after` from the `raw` config. - Expose the `id` inside the hook to make it easier to reference and sort. - Set `id` to the attr name of the hook, instead of the `name`. This corrects a mistake make in the module transition (#397). Technically, a breaking change, but we haven't seen feedback on the initial mistake.

- Remove `before` and `after` from the `raw` config. These are not valid pre-commit configuration options and should not be here. - Remove code to filter out `before` and `after` from the `raw` config. This is not needed if they're not in the config in the first place. - Expose the `id` inside the hook to make it easier to reference and sort. - Set `id` to the attr name of the hook, instead of the `name`. This corrects a mistake made in the module transition (#397). Technically, a breaking change, but we haven't had feedback from the initial mistake.

Consolidates previous work from #533 and #536. - Removes `before` and `after` from the `raw` config. These are not valid pre-commit configuration options and should not be here. - Removes code to filter out `before` and `after` from the `raw` config. There's nothing to filter out if they're not in the config in the first place. - Set `id` to the attr name of the hook, instead of the `name`. This corrects a mistake made in the module transition (#397). Technically, a breaking change, but we haven't had feedback from the initial mistake. - Expose `id` in options.

sandydoo added the enhancement New feature or request label Feb 10, 2024

sandydoo added 19 commits February 18, 2024 16:43

Implement new module design

15330be

Based on #196.

Annotate and fix errors

4f427b4

Add placeholder descriptions

94283df

Set raw

2545f40

Fix

ce684eb

Pass down default_states to each hook

3d416f0

Try passing default_stages again

3833dfd

Rename options

22d19cf

Make run command backwards-compatible

bc05acf

Move hook-specific settings under hooks.<name>.settings

4d53d13

Add package to raw config

de16d2f

Add default packages

27f30f0

Remove unused arg

72bc75c

Fix package typos

f069ad8

Apply mkDefault to built-in hooks

d84404d

Migrate settings.<name>.package to hooks.<name>.package

db697eb

Remove package from the raw options

22683d3

Fix option remap

87a3853

Add a description to each hook

8af5964

sandydoo force-pushed the new-modules branch from e750d99 to 8af5964 Compare February 18, 2024 16:50

domenkozar mentioned this pull request Feb 27, 2024

Add default pre-commit hooks #401

Merged

sandydoo added 2 commits March 9, 2024 23:43

Add assertions and warnings to the pre-commit module

6b2bed0

Migrate binPath to use the package option by default

bf780cc

Add a todo for treefmt

6b40525

sandydoo added 3 commits March 18, 2024 18:22

Add packageInputs to allow overriding complex wrapper hooks

0b32cd7

Implement packageOverrides and add formatters to treefmt

df4a211

Update hook descriptions

b9fb64c

sandydoo marked this pull request as ready for review March 18, 2024 22:51

README: refresh

fc79a53

domenkozar merged commit 844ae66 into master Mar 19, 2024
6 of 8 checks passed

sandydoo mentioned this pull request Mar 19, 2024

The treefmt flake-parts integration has conflicting config definitions #411

Closed

ysndr mentioned this pull request Apr 25, 2024

build: use latest Rust toolchain using fenix flox/flox#1371

Merged

sandydoo mentioned this pull request Dec 15, 2024

Improve before and after internal logic #537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to new module structure #397

Move to new module structure #397

sandydoo commented Feb 10, 2024 •

edited by domenkozar

Loading

sandydoo commented Feb 11, 2024 •

edited

Loading

domenkozar commented Feb 11, 2024

domenkozar commented Feb 11, 2024

sandydoo commented Feb 11, 2024

domenkozar commented Feb 12, 2024

domenkozar commented Feb 15, 2024

domenkozar commented Mar 12, 2024

sandydoo commented Mar 15, 2024

domenkozar commented Mar 15, 2024

nifoc commented Mar 19, 2024

sandydoo commented Mar 19, 2024

roberth commented Mar 27, 2024

domenkozar commented Apr 1, 2024

sandydoo commented Apr 1, 2024

roberth commented Apr 1, 2024

Move to new module structure #397

Move to new module structure #397

Conversation

sandydoo commented Feb 10, 2024 • edited by domenkozar Loading

Tasks

Out of scope

Issues to tackle

Misc fixes

sandydoo commented Feb 11, 2024 • edited Loading

domenkozar commented Feb 11, 2024

domenkozar commented Feb 11, 2024

sandydoo commented Feb 11, 2024

domenkozar commented Feb 12, 2024

domenkozar commented Feb 15, 2024

domenkozar commented Mar 12, 2024

sandydoo commented Mar 15, 2024

domenkozar commented Mar 15, 2024

nifoc commented Mar 19, 2024

sandydoo commented Mar 19, 2024

roberth commented Mar 27, 2024

domenkozar commented Apr 1, 2024

sandydoo commented Apr 1, 2024

roberth commented Apr 1, 2024

sandydoo commented Feb 10, 2024 •

edited by domenkozar

Loading

sandydoo commented Feb 11, 2024 •

edited

Loading