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

Remove recursive mode #151

Closed
x10an14 opened this issue Feb 25, 2024 · 10 comments · Fixed by #240
Closed

Remove recursive mode #151

x10an14 opened this issue Feb 25, 2024 · 10 comments · Fixed by #240

Comments

@x10an14
Copy link

x10an14 commented Feb 25, 2024

Context

Currently testing out the new pkgs.nixfmt-rfc-style, and it's taking forever + erroring out since it attempts to format my .gitignored .direnv/ folder:

-> $ nix fmt
nixfmt: ./.direnv/flake-inputs/5gc4ncnkzcgma8fl1qabm8v9kj7w68gl-source/doc/doc-support: openTempFileWithDefaultPermissions: permission denied (Read-only file system)

Expected behaviour

  • Don't format .gitignored/otherwise explicitly ignored files/files in ignored subfolders of repository.
  • Maybe (additionally, which would solve my issue w/the error, but not the runtime) ignore read-only files.

Things to consider:

  • only support git ignores?
  • support others?
  • add a default list of elements to ignore (like .direnv/, result/)?
@x10an14
Copy link
Author

x10an14 commented Feb 25, 2024

"Taking forever" ~> 20 seconds, just to not mislead what I meant by that technical hyperbole (but then again, how many expect a linter/formatter to take more than sub two seconds).

@piegamesde
Copy link
Member

While nixfmt as of recently has the capability of recursing into subdirectories, usually this should be the job of the program invoking it. Therefore, you should probably take your feature request to the nix fmt developers in your case. For reference, treefmt has a configuration file where you can specify ignores.

Handling gitignore semantics is a highly non-trivial task, and should not be re-implemented for every language formatter.

@piegamesde piegamesde closed this as not planned Won't fix, can't repro, duplicate, stale Feb 25, 2024
@infinisil
Copy link
Member

@piegamesde I don't agree with this. It's really not that hard to support this, because we can use a library like vcs-ignore, which will be good enough for most cases.

@infinisil
Copy link
Member

A especially good argument for needing this is that it doesn't make sense for a formatter to accept directory arguments when it doesn't make any effort to filter those directories, because at that point you need to use another tool for filtering anyways and you'd pass files to the formatter instead.

And at that point, nixfmt is essentially not an end-user CLI anymore, which might be fine, but then we should also deprecate passing directories. This needs to be discussed further.

Low priority, but let's keep this open for now.

@infinisil infinisil reopened this Mar 6, 2024
@piegamesde
Copy link
Member

Yeah, adding directories has just been added in the last release, and honestly I'm uncertain about that either. There's already issues with symlink handling IIRC.

@acid-bong
Copy link

Btw, vanilla nixfmt behaves the same

@infinisil
Copy link
Member

(discussed in the team meeting today)

  • With nix fmt this becomes less important
  • There's also treefmt
  • Generally no need to manually call nixfmt
  • Let treefmt or nix take care of this instead.
  • Decision: nixfmt should not be in charge of recursing into directories. That functionality should be removed before the first official release.
  • Short-term can be kept around

So let's not worry about implementing this and instead focus on nix fmt/treefmt integration. We can keep the open issue for now though

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-03-19/41845/1

kachick added a commit to kachick/kachick.github.io that referenced this issue Apr 1, 2024
Because of just calling nix fmt does not ignore .direnv

NixOS/nixfmt#151
@infinisil infinisil changed the title Ignore .gitignored files? Remove recursive mode Aug 6, 2024
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-08-06/50222/1

infinisil added a commit that referenced this issue Aug 26, 2024
The recursive mode has caused problems because it doesn't do any
filtering, which can mess with files in `.git` directories and
elsewhere. While we could support sane implicit filters and an interface
to filter explicitly, that adds complexity and maintenance burden.

Instead, we can promote the use of `treefmt` instead, a "formatting
multiplexer", which supports file filtering by default. So `nixfmt` will
only be the "backend" formatter, while `treefmt` is the frontend.

Previously discussed in a team meeting here:
#151 (comment)
@infinisil infinisil linked a pull request Aug 26, 2024 that will close this issue
@infinisil
Copy link
Member

PR to weakly deprecate recursive mode: #240

infinisil added a commit that referenced this issue Aug 26, 2024
The recursive mode has caused problems because it doesn't do any
filtering, which can mess with files in `.git` directories and
elsewhere. While we could support sane implicit filters and an interface
to filter explicitly, that adds complexity and maintenance burden.

Instead, we can promote the use of `treefmt` instead, a "formatting
multiplexer", which supports file filtering by default. So `nixfmt` will
only be the "backend" formatter, while `treefmt` is the frontend.

Previously discussed in a team meeting here:
#151 (comment)
infinisil added a commit that referenced this issue Aug 26, 2024
The recursive mode has caused problems because it doesn't do any
filtering, which can mess with files in `.git` directories and
elsewhere. While we could support sane implicit filters and an interface
to filter explicitly, that adds complexity and maintenance burden.

Instead, we can promote the use of `treefmt` instead, a "formatting
multiplexer", which supports file filtering by default. So `nixfmt` will
only be the "backend" formatter, while `treefmt` is the frontend.

Previously discussed in a team meeting here:
#151 (comment)
infinisil added a commit that referenced this issue Aug 26, 2024
The recursive mode has caused problems because it doesn't do any
filtering, which can mess with files in `.git` directories and
elsewhere. While we could support sane implicit filters and an interface
to filter explicitly, that adds complexity and maintenance burden.

Instead, we can promote the use of `treefmt` instead, a "formatting
multiplexer", which supports file filtering by default. So `nixfmt` will
only be the "backend" formatter, while `treefmt` is the frontend.

Previously discussed in a team meeting here:
#151 (comment)
infinisil added a commit that referenced this issue Aug 27, 2024
The recursive mode has caused problems because it doesn't do any
filtering, which can mess with files in `.git` directories and
elsewhere. While we could support sane implicit filters and an interface
to filter explicitly, that adds complexity and maintenance burden.

Instead, we can promote the use of `treefmt` instead, a "formatting
multiplexer", which supports file filtering by default. So `nixfmt` will
only be the "backend" formatter, while `treefmt` is the frontend.

Previously discussed in a team meeting here:
#151 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants