Documentation: mention nixf-tidy in contributing guide

[flokli]

Problem

Since a while, GH actions runs nixf-tidy to detect useless rec and unused variables.

While I'm generally in favor of this, the output sometimes lacks detail, and it's desirable to run some of the checks locally, to prevent spamming PR reviewers (and CI).

However, nixf-tidy and its usage is very sparsely documented.

https://github.com/nix-community/nixd/pull/446 seems to have introduced it to nixd, yet there's neither a --help handler, nor documentation about the tool anywhere

Proposal

document nixf-tidy in both nixd repo, as well in the general nixpkgs contributing guideline and docs.

Checklist

• [x] checked latest Nixpkgs manual (source) and latest NixOS manual (source)
• [x] checked open documentation issues for possible duplicates
• [x] checked open documentation pull requests for possible solutions

---

Add a :+1: reaction to issues you find important.

[inclyc]

nor documentation about the tool anywhere

I don't know why you searched the documentation from PR introducing the tool. It is documented in libnixf README:

https://github.com/nix-community/nixd/tree/main/libnixf#standalone-tools-provided-along-libnixf

basically non-existent

No.

While I'm generally in favor of this, the output sometimes lacks detail, and it's desirable to run some of the checks locally

Currently this most fancy way of rendering the diagnostic is using the nixd language server. Other rendering techniques like TUI are not implemented.

[Aleksanaa]

I'll write a wrapper around nixf-tidy

[flokli]

I'll write a wrapper around nixf-tidy

Thanks! Such a wrapper could be implementing https://github.com/numtide/treefmt/blob/main/docs/formatter-spec.md, rather than reading contents from stdin - which should make it compatible with treefmt.

[inclyc]

I'll write a wrapper around nixf-tidy

Thanks! Such a wrapper could be implementing https://github.com/numtide/treefmt/blob/main/docs/formatter-spec.md, rather than reading contents from stdin - which should make it compatible with treefmt.

It is not a formatter but a linter, hmm, is it still benefitial to be compatible with treefmt in this case?

[flokli]

It is not a formatter but a linter, hmm, is it still benefitial to be compatible with treefmt in this case?

I was mostly refering to the part of having a CLI that allows you to specifiy a list of files to lint, rather than only accepting file contents from stdin. That's already make it much simpler to run locally IMHO.

[inclyc]

Hi @flokli,

That's already make it much simpler to run locally IMHO.

JSON output of nixf-tidy is a not human-readable format. When I wrote the first line of nixf-tidy, I just imaging it will be invoked by some automated scripts.

run locally

If the goal is to "reproduce the result" then the best approach is using the language server. It is the most straightforward way to reproduce fancy UI around the tooling and then people get instant feedback after fixing the issue.

Rendering texts / CLI / TUI on terminal mostly reinventing the wheel of editors (e.g. nvim).

[flokli]

I don't have the LSP setup, I got a complaint from the GH actions that a file isn't passing the linter.

It'd be very nice to reproduce it locally, similar to how the formatting check tells you to run a nixfmt $file on files failing the check.

[flokli]

I think making a tool a required CI check that you cannot easily run without deeply integrating it into your editor is a no-no. This either needs to get a better CLI frontend and more documentation in the nixpkgs manual, or should be removed and re-introduced once it has.

[inclyc]

Hi @flokli,

I think making a tool a required CI check that you cannot easily run without deeply integrating it into your editor is a no-no.

Generally integrating linters is not a bad idea I think, but I agree that we should leave the freedom to make choices.

This either needs to get a better CLI frontend

I think @Aleksanaa is working on this :). Maybe contact us if you are also interested in?

and more documentation in the nixpkgs manual, or should be removed and re-introduced once it has.

The linter currently generates a readable UI in GitHub's 'Changed Files' view, allowing most users to update the failed branch without needing to reproduce the tooling locally. Therefore, I kindly suggest we keep it as-is, as it hasn't caused significant issues so far(?)

[infinisil]

This either needs to get a better CLI frontend

I think @Aleksanaa is working on this :)

I agree with @flokli that we need a local CLI frontend (and some docs on how to run it). I'd rather not leave this in limbo, so unless we know this will be completed in the near future (like within a week or so, is this doable?), I'd be in favor of disabling the workflow until then. Note that it's very easy to disable/reenable workflows in the UI:

I merged the original addition of the workflow fairly quickly and as experimental, so we should also not hesitate to acknowledge some flaws, work them out first, and then try again :)

[flokli]

It's not that we'll gonna have a big risk getting a lot of garbage code. We still have human code review processes in place, we didn't have such a linter for a large part of nixpkgs lifetime, and it's always possible to lint (new) code manually. So yes, I'm in favor of disabling the check until it's more user-friendly (and don't see why we need to wait a week).

As written in the initial description, I do welcome the linting efforts, I just want to make sure it's not more toil for contributors than necessary, and having a documented CLI rather than requiring a full-blown integrated editor setup is one of these points.

Let's temporarily disable this check, and bring it back while documenting it and making it easily accessible for contributors.

[infinisil]

I like the link to the draft values! I'd say it's a bit hard to apply it directly here though. When a PR introduces a linter problem:

• Not having the check means that either:
  • A reviewer manually complains about it, the author needs to address it (lots of toil for author and reviewer)
  • It goes unnoticed (no toil)
• Having the check that only runs in CI:
  • Author needs to address it only after CI runs (toil only for the author)
• Having the check also running locally
  • Author needs to address it, but can do so with immediate feedback (no toil)

So the tradeoff with the experimental check is between the chance for lots of toil for the author and reviewer, and guaranteed some toil only for the author. Further complicated by how reviewer time is arguably more of a limited resource.

[flokli]

Well, we didn't have a check before. Humans could give feedback on unused variables etc, and both authors and reviewer were free to dismiss it. This changes with a required CI check, which currently must be fixed by a contributor, making this a bigger toil than something that can be ignored.

Anyways, I think this is going a bit offtopic, and probably also isn't a productive use of our time.

If you only want to disable the check in a week, and not before, that's your call ;-) The important part is it's easy to reintroduce it once the tooling and documentation issues for it have been addressed.

[infinisil]

Well, we didn't have a check before. Humans could give feedback on unused variables etc, and both authors and reviewer were free to dismiss it. This changes with a required CI check, which currently must be fixed by a contributor, making this a bigger toil than something that can be ignored.

That's a good point :+1:

[Aleksanaa]

I need to take IELTS and GRE before September (and I barely prepared for them), so I may not have the time and energy to finish this project before then. So it's okay to temporarily close it for now.

My idea is very straightforward: just a simple cli frontend using the famous https://crates.io/crates/ariadne crate and nixf as a library. If possible, add a crate that can handle diff so that we can determine the scope of the error based on the modification range. This cli can be placed in the nixd repo and maintained together with nixd. (I've already thought of a name, let’s call it ninter!)

But I haven't really written Rust code from scratch before, so I'm still in the painful learning process so it's not very fast. Of course, if someone else can do this, we'd also welcome it.

[infinisil]

Alright cool, I disabled the workflow manually for now, any committer can reenable it from https://github.com/NixOS/nixpkgs/actions/workflows/check-nixf-tidy.yml when ready :)