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

Replace custom nixfmt wrapping with reformatter #176

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

lafrenierejm
Copy link
Contributor

@lafrenierejm lafrenierejm commented Dec 13, 2022

reformatter here refers to https://github.com/purcell/emacs-reformatter.

Note that as currently written this PR does remove existing user-facing commands. I would be glad to do any of the following:

  1. Leave as-is and accept this as a breaking change.
  2. Do not remove the old code and instead add the new functions in addition to the old one.
  3. Create aliases from the new commands to the old ones. This would keep the code clean and avoid outright breaking the existing interface, but has a potential risk of the new functions behaving slightly differently than the existing one.

For some related previous discussions, see

@lafrenierejm
Copy link
Contributor Author

@matthewbauer Any opinions on this change?

@matthewbauer matthewbauer merged commit 91a317e into NixOS:master Jan 12, 2023
@matthewbauer
Copy link
Member

matthewbauer commented Jan 12, 2023

Sorry, looks good! Using an external package for this seems like a good change.

chvp added a commit to chvp/nix-mode that referenced this pull request Jan 14, 2023
Since NixOS#176 nix-mode is broken for me. Melpa only picks up the dependencies in
the main nix-mode.el file, so this should fix it again.
@matthewbauer
Copy link
Member

Hi! So after discussion from Emacs ELPA, it sounds like we have an issue with using reformatter. The problem is reformatter is not included in ELPA, and since nix-mode is, including an outside hard dependency makes nix-mode not usable. So while we could require it as an optional dependency, it cannot be a hard dependency. It sounds like there is some interest in getting reformatter included in ELPA, but this could take some time and we can revisit this then.

I've decided to revert it for the time being: c18a24e

Thanks for the contribution though! I wish we could use reformatter since it simplifies things so much.

@peterhoeg
Copy link
Member

There seems to be a fair amount of momentum behind apheleia as the formatter framework to replace both format-all and reformatter (authors of all 3 frameworks are discussing it). At least with doom emacs, adding a new formatter is a one-liner:

(set-formatter! 'nixpkgs-fmt '("nixpkgs-fmt") :modes '(nix-mode))

So instead of committing to a particular framework, maybe this can be handled via documentation instead?

Full disclosure - with doom the line above is supposed to work with format-all as well, however it doesn't.

@akirak
Copy link
Contributor

akirak commented Apr 17, 2023

There seems to be a fair amount of momentum behind apheleia as the formatter framework to replace both format-all and reformatter (authors of all 3 frameworks are discussing it)

@peterhoeg Could you provide a link to the discussion?

@peterhoeg
Copy link
Member

@akirak
Copy link
Contributor

akirak commented Apr 17, 2023

@peterhoeg Thanks.

There seems to be a fair amount of momentum behind apheleia as the formatter framework to replace both format-all and reformatter (authors of all 3 frameworks are discussing it).

I have checked out the discussion at lassik/emacs-format-all-the-code#170, but apparently there is no consensus between the authors to switch to apheleia.

People should be free to choose whatever formatter frontend. I am using reformatter, and I haven't found a problem with it so far, but there may be people who prefer to use something else.

So instead of committing to a particular framework, maybe this can be handled via documentation instead?

I agree with this point. It is probably not a good idea to add an extra dependency just to save a few lines of configuration, as there are multiple options for running a formatter.

FYI, some people in the Nix community are using treefmt-nix. With treefmt, there is no need to add a formatter configuration specific to the Nix language.

@adamcstephens
Copy link

eglot/lsp-mode can also ask the language server to run the formatter. nil supports this today. I don't think it's necessary for this package to handle formatting at all.

@peterhoeg
Copy link
Member

peterhoeg commented Apr 18, 2023 via email

@purcell
Copy link
Member

purcell commented Aug 4, 2023

FWIW, reformatter should be clear to go into GNU ELPA soon (see purcell/emacs-reformatter#2), so you could restore this change in due course.

@purcell
Copy link
Member

purcell commented Sep 5, 2023

Update: reformatter is now in NonGNU ELPA, so you could presumably return to using it here if you like.

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.

6 participants