Add "nix flake fmt" builtin formatter #192
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jfly
commented
Oct 11, 2024
Comment on lines
124
to
142
| -- `--walk` is specific to treefmt, and this formatter is supposed | ||
| -- to be generic. | ||
| -- Note: this could get converted to the new `TREEFMT_WALK` | ||
| -- environment variable once | ||
| -- <https://github.com/numtide/treefmt/commit/bac4a0d102e1142406d3a7d15106e5ba108bfcf8> | ||
| -- is fixed, which would at least play nicely with other types of | ||
| -- `nix fmt` entrypoints. | ||
| -- | ||
| -- However, IMO, the real fix is to change treefmt itself to be | ||
| -- willing to format files passed explicitly, even if they're | ||
| -- gitignored: | ||
| -- https://github.com/numtide/treefmt/issues/435 | ||
| "--walk=filesystem", |
There was a problem hiding this comment.
For the record, this is the thing I'd like to figure out before we merge this PR.
There was a problem hiding this comment.
I just realized I said "gitignored" in this comment. What I really meant was "even if they're not tracked by git".
1 task
Mic92
reviewed
Oct 11, 2024
Mic92
reviewed
Oct 11, 2024
Mic92
reviewed
Oct 11, 2024
Mic92
reviewed
Oct 11, 2024
jfly
force-pushed
the
add-nix-fmt
branch
2 times, most recently
from
3ba73f7
to
ec16dd4
Compare
|
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: And pass this as an argument for the gcroot. This logic seems much easier than having to check store paths with |
Mic92
reviewed
Oct 12, 2024
1 task
I'm building a [`nix fmt`](https://nix.dev/manual/nix/latest/command-ref/new-cli/nix3-fmt) formatter. The tricky thing about `nix fmt` is that for nontrivial projects, the entrypoint (`nix fmt`) can be slow enough to be a bad experience to run on every save, and can even trigger [neovim's lsp timeout](https://github.com/nvimtools/none-ls.nvim?tab=readme-ov-file#i-am-seeing-a-formatting-timeout-error-message). It's not the that underlying formatter it invokes are slow, it's that it can take 1-2 seconds for nix to evaluate the `flake.nix` and determine what command `nix fmt` should run under the hood. Since the underlying command shouldn't change very often for a given project, I feel like it would be reasonable to cache this result per project, hence this new `by_bufroot` callback.
This uses `nix fmt` under the hood. I introduced some caching make it performant, as `nix fmt` can take a little while to evaluate all your nix code to figure out the formatter entrypoint.
This protects us against garbage collection so long as vim is running.
Lassulus
reviewed
Oct 14, 2024
Comment on lines
+77
to
+93
| -- Dirty hack to check if the flake actually defines a formatter. | ||
| -- | ||
| -- I cannot for the *life* of me figure out a less hacky way of | ||
| -- checking if a flake defines a formatter. Things I've tried: | ||
| -- | ||
| -- - `nix eval . --apply '...'`: This doesn't not give me the flake | ||
| -- itself, it gives me the default package. | ||
| -- - `builtins.getFlake`: Every incantation I've tried requires | ||
| -- `--impure`, which has the performance downside described above. | ||
| -- - `nix flake show --json .`: This works, but it can be quite slow: | ||
| -- we end up evaluating all outputs, which can take a while for | ||
| -- `nixosConfigurations`. | ||
| if cp.stderr:find("error: flake .+ does not provide attribute .+ or 'formatter'") then | ||
| log:warn("this flake does not define a `nix fmt` entrypoint") | ||
| else | ||
| log:error(string.format("unable discover 'nix fmt' command. stderr: %s", cp.stderr)) | ||
| end |
There was a problem hiding this comment.
I don't know of any better way to check if a flake exposes an attr. but can't we just forward the error as warning in any case this fails and hope that nix provides a good enough warning so we don't need to handle it explicitly?
There was a problem hiding this comment.
I could, but how would I choose the appropriate log level (warn vs error)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements a builtin formatter that (essentially) invokes
nix fmton the given buffer. I called it"nix flake fmt", even though the underlying command isnix fmt, because I didn't want people to confuse it with none-ls's pre-existingnixfmtbuiltin formatter. Naming is hard.This was pretty tricky to build. Issues I ran into:
nix fmtis 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.by_bufrootcache helper (inspired by the existingby_bufnrcache 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.nix eval,nix build, etc implementation makes me cry 😭. 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 thenixcli to make this easier. I have a few ideas about things that could make this simpler.nix fmtentrypoint takes a--walk=filesystemparameter. 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 totreefmtitself). I'd like to address this before we merge this PR up, so I've marked this as a draft.