search

lassik / emacs-format-all-the-code

Auto-format source code in many languages with one command
https://melpa.org/#/format-all
MIT License
613 stars 106 forks source link

Add erb formatter support #193

Closed nbelzer closed 2 years ago

nbelzer commented 2 years ago

Couldn't find any guidelines to contributing so my apologies if something is not handled properly.

I decided to use _Erb as the language-id as Github does not seem to have an official title for the language, instead identifying it as HTML+ERB.

lassik commented 2 years ago

Couldn't find any guidelines to contributing so my apologies if something is not handled properly.

Sorry about the lousy state of the documentation.

  • Added matcher to match files ending in .erb to the _Erb language (which was unsupported before).
  • Adds support for erb-formatter to format _Erb files.

I decided to use _Erb as the language-id as Github does not seem to have an official title for the language, instead identifying it as HTML+ERB.

I'm not familiar with ERB. The current version of languages.yml has these entries:

We could add all of these to language-id. It seems possible to tell them apart by filename extension. What Emacs modes do people use to edit ERB? I can't find an erb-mode. Does web-mode support it?

nbelzer commented 2 years ago

We could add all of these to language-id. It seems possible to tell them apart by filename extension. What Emacs modes do people use to edit ERB? I can't find an erb-mode. Does web-mode support it?

ERB is indeed supported by web-mode (which is also the major mode I am using when editing .html.erb files). As far as I am aware, erb-formatter only handles HTML files. Because of this I would suggest we only add support for HTML+ERB. I'll update the matcher to be more selective based on the filename (.html.erb instead of just .erb) and update the language-id.

Edit: I'm not so sure actually about the more selective filename matcher, since the formatter supports both .html.erb and .turbo_stream.erb files. I've left it as is now.

nbelzer commented 2 years ago

I've left two fixups (to make it clear what I've changed). If you are okay with these changes I can rebase the PR to clean them up before we merge.

lassik commented 2 years ago

Format-all uses a separate library called language-id.el to determine what language is used in the buffer to be formatted. Only languages for which there isn't an official GitHub Linguist ID are special-cased in format-all.el as a necessary hack.

Since both HTML+ERB and JavaScript+ERB are official LInguist IDs, I added them directly to language-id.el and we don't have to add any special-case code to detect them in format-all itself.

Hence, the PR looks fine as it is, but can you remove the special-case (and (string-match-p ".erb\\'" buffer-file-name) "HTML+ERB")?

nbelzer commented 2 years ago

Thanks for that. I've cleaned up the PR and removed the special-case matcher.

lassik commented 2 years ago

Hmm. We should probably refer to the formatter as erb-format since that's the name of the shell command used to run it. Seems clearer to me than erb-formatter which is the name of the Ruby module and gem. What do you think?

nbelzer commented 2 years ago

Agree, also seems more in line with the other formatters. I've made the changes

lassik commented 2 years ago

Perfect. Thanks a lot for sticking with it!