Skip to content

flake: add formatter#3620

Merged
ncfavier merged 3 commits intonix-community:masterfrom
ncfavier:nix-fmt
Jun 27, 2023
Merged

flake: add formatter#3620
ncfavier merged 3 commits intonix-community:masterfrom
ncfavier:nix-fmt

Conversation

@ncfavier
Copy link
Copy Markdown
Member

Closes #3615

@ncfavier ncfavier requested a review from rycee as a code owner January 25, 2023 12:01
Copy link
Copy Markdown
Member

@cyntheticfox cyntheticfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to work this way from my testing, though looking at the changes to the "format" script, it might be good in the future to rewrite to allow for running on distinct files, or to use something like git rev-parse HEAD to resolve the repository root.

@ncfavier
Copy link
Copy Markdown
Member Author

I hadn't thought of using git. We can list the files relative to the repo root with git ls-files, filter them, and reconstruct absolute paths by prepending git rev-parse --show-toplevel.

Comment thread format Outdated
Comment on lines +47 to +51
git ls-files --full-name -- "${files[@]}" |
grep '\.nix$' |
grep -v "${exclude_args[@]}" |
sed "s|^|$git_root/|" |
xargs -d '\n' nixfmt "${nixfmt_args[@]}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to check that sed command. I don't think that's the right syntax.

Either way, I think find was the right approach anyways, as git ls-files will only work on tracked files. Now, that may actually be desirable behavior, but I doubt it. If it is, there are options to git ls-files for exclusions, null-termination.

It'd make much more sense afaict to keep the default behavior of formatting all files in the repository if no file names are provided, but to run the formatter absolutely if given any file names.

That being said, the find function could well leverage -execdir instead of exec, and ensuring -type f. It might actually be faster though to leverage the ability of nixfmt to work on multiple files with find $GITROOT -mindepth 1 -name '.git' -prune -o -type f $FINDEXPR[@] -print0 | xargs -0 nixfmt ${nixfmt_args[@]}. Where $FINDEXPR is a statement like ! -path $FILEPATH.

GNU find is quite a beast...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to check that sed command. I don't think that's the right syntax.

✨ Always read the manual! You never know what you might learn! ✨

as git ls-files will only work on tracked files

We can add -co, that would list untracked files as well.

Sadly git ls-files --exclude only filters untracked files, so we have to do the filtering ourselves. (We can't use pathspecs for this either: they're understood as a disjunction, so we can't say "list the files under those directories AND that match *.nix").

null-termination

I'm normally in favour of that but that's obviously never going to be an issue with home-manager and I can't be bothered.

find ... -print0 | xargs -0 nixfmt ...

That's equivalent to -exec nixfmt {} +, which is what we had previously.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't be bothered.

Did it anyway.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to check that sed command. I don't think that's the right syntax.

sparkles Always read the manual! You never know what you might learn! sparkles

Guess that's what I get for not reading every possible manpage out there and expecting GNU's sed manpage to actually detail the command, not just implementation-specifics...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah GNU man pages suck. This is documented in the POSIX man page sed(1p) and in the actual GNU sed manual.

@stale

This comment was marked as outdated.

@stale stale bot added the status: stale label Apr 28, 2023
Copy link
Copy Markdown
Member

@cyntheticfox cyntheticfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having re-looked at it, looks good to me.

@stale stale bot removed the status: stale label May 5, 2023
@somasis
Copy link
Copy Markdown
Member

somasis commented Jun 27, 2023

Anything holding this back from being merged? It'd be nice to have for entirely selfish reasons (like being able to reliably use nix fmt as my editor's default Nix file formatting command).

ncfavier added 3 commits June 27, 2023 23:10
For now, fail if the user tries to format a specific file/directory,
or runs the formatter from within a subdirectory.

Handling these situations is slightly tricky because `find -path` is
not very flexible.
This allows running the formatter with `nix fmt`, added in Nix 2.8.
This is cleaner than `find` and allows us to restrict formatting to
particular files or subdirectories.
@ncfavier ncfavier merged commit 9dd107a into nix-community:master Jun 27, 2023
@ncfavier ncfavier deleted the nix-fmt branch June 27, 2023 21:32
antholeole pushed a commit to antholeole/home-manager that referenced this pull request Jul 24, 2023
* format: improve argument handling

For now, fail if the user tries to format a specific file/directory,
or runs the formatter from within a subdirectory.

Handling these situations is slightly tricky because `find -path` is
not very flexible.

* flake: add formatter

This allows running the formatter with `nix fmt`, added in Nix 2.8.

* format: use git ls-files

This is cleaner than `find` and allows us to restrict formatting to
particular files or subdirectories.
SimSaladin added a commit to SimSaladin/home-manager that referenced this pull request Aug 26, 2023
* upstream/master: (352 commits)
  flake.lock: Update
  qcal: add module
  bat: generate cache file in XDG cache home
  nixos: only refer to `users.users` if needed
  home-cursor: remove IFD when linking icon directories
  flake.lock: Update
  home-cursor: improve icon compatibility
  fix(qt): allow theming for apps started by systemd (nix-community#4349)
  flake.lock: Update
  fcitx5: add `fcitx5-config-qt` to test stub
  pqiv: only run tests on Linux platforms
  pqiv: add module
  Translate using Weblate (German)
  xplr: add module
  browserpass: support librewolf
  Translate using Weblate (Romanian)
  Translate using Weblate (Portuguese (Brazil))
  hyprland: remove xwayland.hidpi (nix-community#4302)
  home-manager: handle profile list in Nix >2.17
  lf: simplify option validation (nix-community#4334)
  programs.khal: uncomment locale config (nix-community#4319)
  kanshi: support adaptive sync (nix-community#4328)
  mu: add package option (nix-community#4325)
  modules: types.string throws error now (nix-community#4324)
  taffybar: Avoid restarting too quickly (nix-community#4316)
  aerc: do not use smtp-starttls (nix-community#4272)
  zsh: Add `zsh.history.ignoreAllDups` config option (nix-community#4248)
  jujutsu: fix zsh completion (nix-community#4305)
  Translate using Weblate (Korean)
  Translate using Weblate (Romanian)
  Translate using Weblate (German)
  flake.lock: Update
  network-manager-applet: remove `--sm-disable` flag
  himalaya: update a link to ticket
  nushell: deprecation of let-env (nix-community#4292)
  man: fix caches generation in cross-compiled system (nix-community#4294)
  fish: fix session vars build in cross-compiled system (nix-community#4293)
  Update translation files
  Translate using Weblate (Chinese (Simplified))
  gh: option to enable helper for additional hosts (nix-community#4288)
  home-manager: rework news command
  home-manager: skip manual in uninstall configuration
  gnome-terminal: add assertion on profile names
  hyprland: use `toKeyValue`'s `indent` argument (nix-community#4274)
  gpg: fix typo (nix-community#4277)
  firefox: make package nullable (nix-community#4113)
  git-sync: add news entry for darwin
  tmate: don't generate empty config file (nix-community#4271)
  gh-dash: add module
  git-sync: add darwin support
  flake.lock: Update
  hyprland: prioritize variables and beziers (nix-community#4263)
  hyprland: add module
  script-directory: fix documentation link (nix-community#4258)
  Translate using Weblate (Indonesian)
  Translate using Weblate (Ukrainian)
  Translate using Weblate (Spanish)
  Translate using Weblate (Swedish)
  docs: hide `_module.*` from NixOS/nix-darwin docs
  jujutsu: update for Jujutsu 0.8.0 (nix-community#4250)
  ripgrep: don't set env. variable if no config (nix-community#4254)
  docs: update for Markdown migration
  treewide: remove now-redundant `lib.mdDoc` calls
  docs: clean up after Markdown migration
  flake: remove temporary nixpkgs pin
  treewide: convert all option docs to Markdown
  treewide: adjust some DocBook for conversion
  treewide: `mkAliasOption` -> `mkAliasOptionMD`
  treewide: `mkPackageOption` -> `mkPackageOptionMD`
  treewide: convert options with tables to Markdown
  treewide: convert options with lists to Markdown
  treewide: convert custom `enable` docs to Markdown
  treewide: convert parameterized docs to Markdown
  treewide: manually convert some docs to Markdown
  treewide: fix `mkEnableOption` arguments
  treewide: add missing option descriptions
  docs: use `nixosOptionsDoc`
  flake: temporarily pin nixpkgs version
  version: add `isReleaseBranch`
  manual: add test
  Update translation files
  flake.lock: Update
  home-manager: remove Home Manager default paths
  imapnotify: Use JSON type for extraConfig (nix-community#4238)
  Translate using Weblate (French)
  zsh: fix custom syntax highlighting styles (nix-community#4236)
  i3-sway: allow arbitrary floating modifier (nix-community#4229)
  i3-sway: multiple outputs (nix-community#4223)
  home-cursor.nix: enable gtk module when enabling gtk config generation (nix-community#4144)
  aerc: add assertion to limit per-account extraConfig to UI config (nix-community#4196)
  flake.lock: Update
  ssh-agent: add assertion and fix news entry (nix-community#4210)
  imapnotify: move test
  imapnotify: use direct nix store path for config
  flake.lock: Update
  nushell: add login.nu configuration option
  himalaya: fix notmuch backend
  swayosd: add module
  pyenv: add module
  darcs: add module
  tests: some minor cleanups
  goimapnotify: remove test dependency on notmuch
  unison: Allow using same option multiple times (nix-community#4208)
  imapnotify: Add launchd agent (nix-community#3291)
  i3: remove deprecated example in i3.config.startup (nix-community#4201)
  antidote: static file move to /tmp
  Translate using Weblate (Indonesian)
  PULL_REQUEST_TEMPLATE.md: add maintainer cc section (nix-community#4193)
  programs.khal
  vdirsyncer: synchronize metadata as well (nix-community#4167)
  i18n: Use glibcLocales from NixOS if possible (nix-community#2333) (nix-community#4177)
  xfconf: remove properties with null values (nix-community#4192)
  home-manager: Use `path:` URI type for flake default (nix-community#3646)
  ci: autolabel calendars PRs (nix-community#4165)
  flake.lock: Update (nix-community#4161)
  news: fix typo in programs.zsh.antidote entry
  ssh-agent: init module (nix-community#4178)
  chromium: fix `commandLineArgs` to use the user specified package (nix-community#4175)
  sxhkd: allow usage of derivations as keybind commands (nix-community#4169)
  README: Remove the pills (nix-community#4181)
  lsd: use -A instead of -a in aliases (nix-community#4173)
  kakoune: add defaultEditor option
  docs: update link to allowed users setting (nix-community#4176)
  zsh: allow setting custom syntax highlighting styles (nix-community#4122)
  flake: add formatter (nix-community#3620)
  broot: fix test (nix-community#4170)
  antidote: fix .dot path
  ci: build manual and push to home-manager.dev
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add formatter flake output

3 participants