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

Add format-all recipe #5390

Merged
merged 1 commit into from
Apr 20, 2018
Merged

Add format-all recipe #5390

merged 1 commit into from
Apr 20, 2018

Conversation

lassik
Copy link
Contributor

@lassik lassik commented Mar 27, 2018

Brief summary of what the package does

Lets you auto-format source code in any one of several languages using the same command for all languages, instead of learning a different elisp package and formatting command for each language.

Currently supported:

  • C/C++ (clang-format)
  • D (dfmt)
  • Elm (elm-format)
  • Emacs Lisp (native)
  • Go (gofmt)
  • Haskell (hindent)
  • JavaScript (standard)
  • OCaml (ocp-indent)
  • Perl (perltidy)
  • Python (autopep8)
  • Ruby (rufo)
  • Rust (rustfmt)
  • Shell script (shfmt)
  • Swift (swiftformat)

New formatters can be added fairly easily if they are external programs that can read code from stdin and format it to stdout.

Direct link to the package repository

https://github.com/lassik/emacs-format-all-the-code

Your association with the package

Just wrote it :p

Checklist

Please confirm with x:

@purcell
Copy link
Member

purcell commented Apr 8, 2018

Thanks, and sorry about the slow response.

I'm pretty sure there's at least one package like this in MELPA already, but I can't find it right now, and I think it's a good idea which I would use myself! So, feedback re. the code:

  • I suggest you drop the :install commands. It's tangential to the core behaviour of the package, and the commands will be highly system-depend. Just say "install <executable>" instead.
  • format-all-fix-trailing-whitespace is just an ad-hoc reimplementation of whitespace-cleanup. I suggest you remove it in any case, since if users care about this, they can add whitespace-cleanup to their before-save hook.
  • Consider using alists instead of plists: let-alist is then very convenient.
  • Provide a local minor mode which alters the local before-save-hook in order to call format-all-the-buffer (which you should probably rename to something a bit clearer like format-all-reformat-buffer, and provide an ;;;###autoload for).
  • The :executable vs. :function thing isn't quite right yet, IMO. It's weird to have executable names specified separately from their arguments, and to have to write functions for each reformatter, when most of them do something quite simple. Perhaps support either :function or :command '(...), where the function is passed the plist, and the default :function would execute the :command it finds there.

Hope that helps!

@lassik
Copy link
Contributor Author

lassik commented Apr 8, 2018

Thanks, and sorry about the slow response.

No problem, we are all volunteers here :) I appreciate the detailed feedback.

I'm pretty sure there's at least one package like this in MELPA already, but I can't find it right now, and I think it's a good idea which I would use myself!

I also tried hard to find one but couldn't. I tried again to go through all MELPA search results for the words multiple, several, many, languages, format, indent and did similar Google searches and couldn't find anything either.

I suggest you drop the :install commands. It's tangential to the core behaviour of the package, and the commands will be highly system-depend. Just say "install " instead.

I personally really like this feature and would like to keep it. It is obviously destined to be imperfect but I often wish programs would go the extra mile to give me useful hints like this ("most people in your situation will want to try X"). Given that universal installation commands don't exist, I think suggesting a system-dependent command is more helpful than suggesting nothing at all. If you go through the current list of install commands there is a lot of variation among them which each user would otherwise have to figure out manually via web search. It's also really easy to implement this, just a few extra lines of code.

I should definitely change it to say Try installing it with command X instead of Install it with command X.

format-all-fix-trailing-whitespace is just an ad-hoc reimplementation of whitespace-cleanup. I suggest you remove it in any case, since if users care about this, they can add whitespace-cleanup to their before-save hook.

The thing is that many (probably most) of the external formatters already do their own whitespace cleanup as part of their normal operation. It usually can't even be turned off. I think they are doing the right thing, since accidental whitespace changes are a common annoyance in version control diffs. I can't imagine anyone wanting to auto-format their code to a precise standard, yet not remove trailing whitespace.

format-all-fix-trailing-whitespace exists because a few formatters don't fix whitespace for some reason. The point of the format-all package is to provide a consistent interface to formatters, so that users can forget about their differences (I want to give people "the luxury of ignorance"). If most formatters clean up whitespace but some of them randomly don't, I think that violates this expectation and we should patch it up. The ideal would be to convince the people making those external formatters to add whitespace cleanup, but there are so many of them that it would take years.

I think Emacs' own whitespace-cleanup does too much for our purposes. It relies on indent-tabs-mode and tab-width to fix tabs vs spaces. But the Emacs values of those variables may not agree with the formatter's idea of tabs vs spaces and it would be very complex to match them up reliably for all formatters (and in my opinion fruitless to expect users to always have them matched up). Indeed, I have personally fallen into a pattern where I often don't even bother to configure Emacs' indentation and whitespace rules anymore - I just keep doing format-all constantly and the formatter makes everything right with no configuration.

To clarify, I think most if not all external formatters fix tabs vs spaces (to their idea of what it should be) but some of them don't fix trailing whitespace. We can easily lop off trailing whitespace in with an Emacs function without caring about tabs vs spaces. Hence the custom function.

Consider using alists instead of plists: let-alist is then very convenient.

Sweet :) I'll keep that in mind. In the current code most properties are system-dependent (or can be) so in practice they are accessed via format-all-property.

Provide a local minor mode which alters the local before-save-hook in order to call format-all-the-buffer (which you should probably rename to something a bit clearer like format-all-reformat-buffer, and provide an ;;;###autoload for).

Will do. We should obviously provide a save hook but I didn't know how to make one and kept putting it off.

The :executable vs. :function thing isn't quite right yet, IMO. It's weird to have executable names specified separately from their arguments, and to have to write functions for each reformatter, when most of them do something quite simple. Perhaps support either :function or :command '(...), where the function is passed the plist, and the default :function would execute the :command it finds there.

I know, I expect that part of the code to keep evolving. I think we should collect many more formatters (2-3x the current amount) before worrying about making it pretty.

There is indeed boilerplate with all the similar functions but I haven't refactored it yet because it isn't clear how to best do it. It's easy to come up with a prettier data structure but the trick is to cover all the special cases that are bound to come up. (For example, the executable argument is weird but I haven't factored it out because I anticipate some formatters need to be called in weird ways. Ocaml's ocp-indent is already non-ideal under the current representation and needs to be thought about, this kind of thing is what I mean. We should collect 3-5 non-ideal formatters before we can make a proper decision about what kind of data structure to use.)

What should we do about release - should we hold off on putting this into MELPA in case you find the other similar package you recall coming across? What do you think about the points we discussed and do you think the package should be polished more before adding it in? I think there is necessarily some experimentation involved in a package dealing with so many unknowns (look at how many formatters atom-beautify supports and imagine all the corner cases they had to cover) so I didn't worry about making everything perfect. My main criterion was to beat at it until it supports many languages and is a pleasure for me to use (which it wasn't at first but now is) and figure out the rest later.

I think the main point for users is to have the following stable interface:

  • command to format the current buffer
  • command to put in their before-save-hook
  • (later) custom variables for selecting and configuring formatters

and the internals can then evolve as needed from release to release.

@lassik
Copy link
Contributor Author

lassik commented Apr 8, 2018

I thought some more about the minor mode and before-save-hook and the customize variables and I'd like to hold off on those for now because It's far from obvious how to do them right.

The thing is that everyone working on a particular codebase should really be using the same external formatter with the same settings (whether they use Emacs, Vim or any other editor). Otherwise version control diffs will eventually turn into a mess and much of the point of auto-formatting in the first place is moot. This implies that formatter settings should really be stored in an editor-independent file that is checked into version control. (Ideally some existing settings file such as .editorconfig would be extended for this purpose instead of making Yet Another Dotfile with its own syntax and filename globbing rules :))

So basically, I think any effort that we spent putting code formatting settings in Emacs would only serve to detract from the much more important goal of specifying a unified cross-editor settings file. We should work together with people from the Vim, Atom and other communities to make it a reality sooner. Once a de facto cross-editor standard is agreed upon, we can then add a global minor mode for Emacs and people can leave it always enabled and not worry about configuring it specifically for Emacs. (EDIT: Maybe we should just wait for Unibeautify to finish this, they already have this: https://unibeautify.com/docs/config-file.html)

@purcell
Copy link
Member

purcell commented Apr 20, 2018

Great, thanks for your patience.

I get your point about standardising configuration into .editorconfig etc., and supporting such a thing is a worthy goal that isn't in conflict with providing good Emacs-only configuration for the package: if format-all had a very Emacs-oriented configuration system, then it would be easy to translate settings to it from any editorconfig standard that might arise.

In any case, I'll go ahead and merge this now.

@lassik
Copy link
Contributor Author

lassik commented Apr 21, 2018

Thanks :) I quoted the relevant parts of the above discussion in the project's GitHub issues.

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.

2 participants