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

Use reformatter.el for zig fmt #39

Closed
joachimschmidt557 opened this issue Oct 6, 2020 · 8 comments · Fixed by #51
Closed

Use reformatter.el for zig fmt #39

joachimschmidt557 opened this issue Oct 6, 2020 · 8 comments · Fixed by #51

Comments

@joachimschmidt557
Copy link
Member

https://github.com/purcell/reformatter.el provides an easy interface to provide idiomatic formatting commands. Using this library would make it would be possible to move a lot of the code which deals with zig fmt from this package to reformatter.el. Of course, this means that this mode has one more Elisp package as a dependency.

@joachimschmidt557
Copy link
Member Author

joachimschmidt557 commented Dec 8, 2020

Issues with current buffer reformatting which would be solved by using reformatter.el:

  • Files on remote servers (tramp) cannot be formatted and zig-mode will annoyingly tell this to the user every time a file is saved
  • Files which are not saved on disk (e.g.*scratch*) cannot be formatted
  • revert-buffer is run after every format (and thus after every save), which leads to unexpected behavior such as font size going back to the default, LSP mode is turned off (lsp-mode is turned off for buffer on buffer save #49), etc. reformatter.el solves this behaviour by running the code through the formatter (via stdin, which is possible in zig fmt) before saving so no revert is necessary

Due to this, I would suggest that we consider this proposal and weigh the advantages with the disadvantage of having one additional dependency.

@GarbageHamburger
Copy link

Most of the programming major modes in Emacs do not seem to have explicit code formatting functionality, but rather, packages such as https://github.com/lassik/emacs-format-all-the-code are used. Coupled with LSP support for formatting via zls, is there a need to have explicit formatting support in zig-mode?

@joachimschmidt557
Copy link
Member Author

If the zig fmt is not used often by users of zig-mode, we can also consider removing that. But I personally use it all the time and the format on save functionality helps me a lot. zig-mode already supports compiling, running and testing zig source files, so formatting is not very far-fetched.

Seeing zig.vim also having formatting and format on save builtin, I support keeping formatting functionality and moving it to reformatter.el.

@GarbageHamburger
Copy link

I see. I will look into adding this myself, I'm also looking into #13 as that significantly improves interop, but I am in no way an expert elisp hacker ^^.

@Inc0n
Copy link

Inc0n commented Jul 3, 2022

I want to add my zig-mode format experience here, given that it's very useful to have hints about my zig code, it doesn't play well with undo-hist, and cause it to prompt buffer-undo-list is not empty. Do you want to recover now? (y or n) every time zig formats on before save hook.

@jiacai2050
Copy link
Contributor

jiacai2050 commented Dec 11, 2022

Hi @joachimschmidt557 I try your forked version, https://github.com/ziglang/zig-mode/pull/51/files

it works amazing, since zls is not very stable now, so I suggest we merge it in master.

PS: I guess no one care one more package, besides its author is purcell, master of elisp.

@jiacai2050
Copy link
Contributor

jiacai2050 commented Dec 11, 2022

I'm a long-time rust-format-buffer user, and there is no such logic in there

Maybe we can follow what rust-mode does, pass stdin to zig fmt, and reinsert via replace-buffer-contents when succeed

@joachimschmidt557
Copy link
Member Author

See #75: I will merge this PR on the 1st of January 2023.

@joachimschmidt557 joachimschmidt557 changed the title Proposal: use reformatter.el for zig fmt Use reformatter.el for zig fmt Dec 16, 2022
@joachimschmidt557 joachimschmidt557 pinned this issue Dec 20, 2022
@joachimschmidt557 joachimschmidt557 unpinned this issue Jan 1, 2023
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 a pull request may close this issue.

4 participants