Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation: mention nixf-tidy in contributing guide #332695

Open
3 tasks done
flokli opened this issue Aug 6, 2024 · 18 comments
Open
3 tasks done

Documentation: mention nixf-tidy in contributing guide #332695

flokli opened this issue Aug 6, 2024 · 18 comments

Comments

@flokli
Copy link
Contributor

flokli commented Aug 6, 2024

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.

nix-community/nixd#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


Add a 👍 reaction to issues you find important.

@inclyc
Copy link
Member

inclyc commented Aug 6, 2024

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
Copy link
Member

I'll write a wrapper around nixf-tidy

@eclairevoyant eclairevoyant changed the title Documentation: nixf-tidy documentation is basically non-existent Documentation: mention nixf-tidy in contributing guide Aug 7, 2024
@flokli
Copy link
Contributor Author

flokli commented Aug 7, 2024

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
Copy link
Member

inclyc commented Aug 7, 2024

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
Copy link
Contributor Author

flokli commented Aug 10, 2024

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
Copy link
Member

inclyc commented Aug 10, 2024

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
Copy link
Contributor Author

flokli commented Aug 10, 2024

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
Copy link
Contributor Author

flokli commented Aug 10, 2024

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
Copy link
Member

inclyc commented Aug 10, 2024

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
Copy link
Member

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: 2024-08-10_18-26

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
Copy link
Contributor Author

flokli commented Aug 10, 2024

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
Copy link
Member

infinisil commented Aug 10, 2024

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
Copy link
Contributor Author

flokli commented Aug 10, 2024

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
Copy link
Member

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 👍

@Aleksanaa
Copy link
Member

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
Copy link
Member

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 :)

@inclyc
Copy link
Member

inclyc commented Jan 1, 2025

I just made a wrapper here! 👀 Take a look?

@infinisil
Copy link
Member

Looking good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants