Add extraPackages to each hook and propagate them to enabledPackages

[9999years]

enabledPackages can be used to include all the relevant packages for enabled hooks into a developer environment. Unfortunately, it doesn't automatically pick up on packages other than a hook's default package attribute. By adding an extraPackages option to each hook, these other packages can be seamlessly added to developer environments.

I've included defaults for extraPackages for the following hooks:

• treefmt: the enabled settings.formatters
• clippy, rustfmt: the packageOverrides
• dune-fmt: the enabled settings.extraRuntimeInputs

[domenkozar]

Shouldn't we just propagate packageOverrides?

[9999years]

packageOverrides doesn't seem to have the expectation that every defined packageOverrides key will be propagated to the developer environment. (E.g. couldn't someone create one set of packageOverrides and use it for all their hooks?)

I'm also not quite sure how that would work with e.g. treefmt -- I suppose treefmt could just implicitly use all the packageOverrides values as formatters, but it seems a little clumsy. What do you think?

[sandydoo]

E.g. couldn't someone create one set of packageOverrides and use it for all their hooks

We should remove packageOverrides from the base hook module. It was a mistake on my part to add it there. It doesn't do anything by default and the only 3 hooks that use it overload the option anyway.

It's sole purpose is to provide an access point to override specific dependencies for hooks that have them. And the reason it's an attrset and not a list of packages (like extraPackages) is so that the package names show up in the docs. I initially wanted to expose the wrapper packages for these hooks and let people use overrides, but I wasn't sure how to document this. So we compromised.

I'm also not quite sure how that would work with e.g. treefmt -- I suppose treefmt could just implicitly use all the packageOverrides values as formatters, but it seems a little clumsy. What do you think?

Yeah, treefmt is not like the other hooks. That's why treefmt.settings.formatters exists.

@domenkozar, I quite like this actually. packageOverrides doesn't fit the bill because we have hooks like treefmt and dune-fmt.

[sandydoo]

LGTM!

The only question I have is whether we might want to mark this as internal? But then it won't show up in the docs at all, which is annoying if you're writing a custom hook. 🤷 Probably good as it is.

[roberth]

internal?

I've argued for a category of "advanced" options, a middle ground, but that's controversial, in the context of NixOS at least.

• Advanced options NixOS/nixpkgs#257300

[9999years]

We could also replace packageOverrides with individual package options.

[sandydoo]

We could also replace packageOverrides with individual package options.

That's what we're doing now, no? Or do you have something else in mind?

git-hooks.nix/modules/hooks.nix

Lines 176 to 185 in 40e6053

	options.packageOverrides = {
	cargo = mkOption {
	type = types.package;
	description = lib.mdDoc "The cargo package to use";
	};
	clippy = mkOption {
	type = types.package;
	description = lib.mdDoc "The clippy package to use";
	};
	};

[9999years]

That's what we're doing now, no? Or do you have something else in mind?

Oh, I see. I thought it was an arbitrary attrset of packages for some reason.

Maybe we should remove the packageOverrides level then, so that overriding packages can be done with package for the main package and (e.g.) cargo and clippy for the others (rather than packageOverrides.cargo`).

[sandydoo]

Maybe we should remove the packageOverrides level then, so that overriding packages can be done with package for the main package and (e.g.) cargo and clippy for the others (rather than packageOverrides.cargo`).

Maybe 🤷 The extra level excludes the (admittedly unlikely) possibility of naming conflicts with other pre-commit options.