none-ls.nvim icon indicating copy to clipboard operation
none-ls.nvim copied to clipboard

Add "nix flake fmt" builtin formatter

Open jfly opened this issue 1 year ago • 1 comments

This implements a builtin formatter that (essentially) invokes nix fmt on the given buffer. I called it "nix flake fmt", even though the underlying command is nix fmt, because I didn't want people to confuse it with none-ls's pre-existing nixfmt builtin formatter. Naming is hard.

This was pretty tricky to build. Issues I ran into:

  • nix fmt is slow enough that it's unbearable to run every time you save a buffer. The slowness is the time spent evaluating all the relevant nix code each time, so I opted to cache the result.
    • I wanted to cache this per project rather than per buffer, so I added a new by_bufroot cache helper (inspired by the existing by_bufnr cache helper). That's the first commit in this PR, and could be merged up independently, but it might be silly to do so without any builtins using it.
    • The underlying nix eval, nix build, etc implementation makes me cry :sob:. I'm hoping a Nix expert swoops in and shows me a better way. Or, perhaps we can use this as motivation to add a feature to the nix cli to make this easier. I have a few ideas about things that could make this simpler.
  • The current implementation assumes that the nix fmt entrypoint takes a --walk=filesystem parameter. That's a bogus assumption. I've left a comment in the code explaining how I think we should fix this (it'll require a change to treefmt itself). I'd like to address this before we merge this PR up, so I've marked this as a draft.

jfly avatar Oct 11 '24 01:10 jfly

I actually just learned that neovim seems to have a tempdir with automatic cleanup already builtin. So all you have to do is doing this once and save it in temporary variable:

tmp_dir = vim.fn.tempname()

And pass this as an argument for the gcroot. When vim terminates this directory is removed again.

This logic seems much easier than having to check store paths with nix-store and is also more sound.

Mic92 avatar Oct 12 '24 07:10 Mic92

I've rebased this on top of https://github.com/nvimtools/none-ls.nvim/pull/197 now that it's merged. Also, treefmt v2.1.0 (with https://github.com/numtide/treefmt/commit/659aa0fdc20d2deaeaf76d325449ce86d2869036) has been released for a while, so I feel comfortable removing the --walk=filesystem option.

jfly avatar Dec 17 '24 22:12 jfly

If someone with a different setup could test this I'd be glad to merge, since I have zero Nix experience.

mochaaP avatar Dec 25 '24 04:12 mochaaP

cc @Lassulus for testing ^

Mic92 avatar Dec 25 '24 13:12 Mic92

This seems to work for me.

mightyiam avatar Dec 27 '24 17:12 mightyiam

Should this be in none-ls vs none-ls-extras? Are there any guidelines as to which sources go to which? I find it interesting that "eslint", for example, is in extras, and "nix fmt" would be in main.

barrett-ruth avatar Dec 28 '24 13:12 barrett-ruth

eslint is in extras since a better alternative is available (eslint-lsp).

mochaaP avatar Dec 28 '24 20:12 mochaaP

eslint is in extras since a better alternative is available (eslint-lsp).

of course. ty!

barrett-ruth avatar Dec 29 '24 00:12 barrett-ruth

Thanks for the merge, @mochaaP!

jfly avatar Dec 29 '24 02:12 jfly