pre-commit hook is broken in nix

[kokobd]

Currently, nix develop complains:

nix-pre-commit-hooks: updating /home/kokobd/work/github.com/kokobd/haskell-language-server repo
nix-pre-commit-hooks: WARNING: Refusing to install because of pre-existing .pre-commit-config.yaml
    1. Translate .pre-commit-config.yaml contents to the new syntax in your Nix file
        see https://github.com/hercules-ci/nix-pre-commit-hooks#getting-started
    2. remove .pre-commit-config.yaml
    3. add .pre-commit-config.yaml to .gitignore

This problem was introduced by #2679, where .pre-commit-config.yaml was checked in git.
But according to our documentation, .pre-commit-config.yaml should be managed by Nix. People do not use Nix can just manually install pre-commit hook and paste the file content into .pre-commit-config.yaml

haskell-language-server/docs/contributing/contributing.md

Lines 171 to 216 in 09968a1

	### Formatter pre-commit hook
	We are using [pre-commit-hook.nix](https://github.com/cachix/pre-commit-hooks.nix) to configure git pre-commit hook for formatting. Although it is possible to run formatting manually, we recommend you to use it to set pre-commit hook as our CI checks pre-commit hook is applied or not.
	You can configure the pre-commit-hook by running
	``` bash
	nix-shell
	```
	If you don't want to use [nix](https://nixos.org/guides/install-nix.html), you can instead use [pre-commit](https://pre-commit.com) with the following config.
	```json
	{
	"repos": [
	{
	"hooks": [
	{
	"entry": "stylish-haskell --inplace",
	"exclude": "(^Setup.hs$|test/testdata/.*$|test/data/.*$|test/manual/lhs/.*$|^hie-compat/.*$|^plugins/hls-tactics-plugin/.*$|^ghcide/src/Development/IDE/GHC/Compat.hs$|^ghcide/src/Development/IDE/Plugin/CodeAction/ExactPrint.hs$|^ghcide/src/Development/IDE/GHC/Compat/Core.hs$|^ghcide/src/Development/IDE/Spans/Pragmas.hs$|^ghcide/src/Development/IDE/LSP/Outline.hs$|^plugins/hls-splice-plugin/src/Ide/Plugin/Splice.hs$|^ghcide/test/exe/Main.hs$|ghcide/src/Development/IDE/Core/Rules.hs|^hls-test-utils/src/Test/Hls/Util.hs$)",
	"files": "\\.l?hs$",
	"id": "stylish-haskell",
	"language": "system",
	"name": "stylish-haskell",
	"pass_filenames": true,
	"types": [
	"file"
	]
	}
	],
	"repo": "local"
	},
	{
	"repo": "https://github.com/pre-commit/pre-commit-hooks",
	"rev": "v4.1.0",
	"hooks": [
	{
	"id": "mixed-line-ending",
	"args": ["--fix", "lf"],
	"exclude": "test/testdata/.*CRLF*.hs$"
	}
	]
	}
	]
	}
	```

Now we have two options to compare:

1. Remove pre-commit config from Nix, and update the docs accordingly.
   • pros: Non-Nix users' life is made easier a little bit.
   • cons: Nix users will have to install pre-commit hooks manually.
2. Remove .pre-commit-config.yaml from Git, and let Nix manage it again.
   • pros: Nix users can benefit from the pre-commit hooks without any extra setup.
   • cons: Non-Nix users have to do one more manual step: paste the .pre-commit-config.yaml

I recommend the second option, as Non-Nix users will have some manual setups anyway, and we were on option 2 before #2679 was merged.

[drsooch]

To be fair this has been merged for 5 months. No one has raised any objections or issues since. It would seem like nix users are in the minority.

The original intention was to make it so users didn't have to copy the json from documentation.

Is there a third option that can satisfy both parties?

[drsooch]

I'm assuming the nix file of interest is flake.nix it looks like the pre-commit hook is out of date anyways. Not that it matters for nix users but we've added a \r\n to \n converter in the pre-commit hook as well. Seems odd nix can't handle a predefined pre commit hook configuration.

[kokobd]

I noticed the last change to flake.nix was made by @michaelpj last month, so I assume you are using Nix. Would you mind providing some advice on this?

[michaelpj]

I haven't really been using the flake-based shell so I'm not sure. Perhaps we could try using something like https://github.com/cachix/pre-commit-hooks.nix ? I think that generates pre-commit.yaml on the fly also?

[kokobd]

Perhaps we could try using something like https://github.com/cachix/pre-commit-hooks.nix ?

flake.nix is using it already, but it relies on the non-existence of .pre-commit-config.yaml.

[michaelpj]

Why did we commit .pre-commit-config.yaml then? I am confused.

[drsooch]

Why did we commit .pre-commit-config.yaml then? I am confused.

I had made an unrelated change to the config, and someone had suggested to commit it directly rather than have users copy the config directly.

[kokobd]

More context: .pre-commit-config.yaml was added back into .gitignore in #2892 last month. But it isn't removed from git. (an already checked-in file needs to be removed by git rm --cached, adding it to .gitignore alone is not sufficient)

[kokobd]

Considering not everyone is using Nix to develop HLS, ignoring .pre-commit-config.yaml will lead to an outdated config if it is changed. (who will remember to check docs for the latest .pre-commit-config.yaml regularly?).

So I'm planning to:

• Remove pre-commit config from flake.nix. Add pre-commit to shellHook directly (so that people using Nix still benefit from .pre-commit-config.yaml)
• Add pre-commit to Gitpod configuration
• Update docs about this. It's no longer required to copy .pre-commit-config.yaml from the docs.

[teto]

this breaks nix flake show. In the line checks = { pre-commit-check = pre-commit-check ghcDefault; }; pre-commit-check is unknown. I find the config of pre-commit hooks via nix a bit annoying, should I just ditch that check ?

[kokobd]

@teto Oh, nice catch! I only tested nix develop. I believe you can remove both checks = { pre-commit-check = pre-commit-check ghcDefault; }; and inherit (self.checks.${system}.pre-commit-check) shellHook;