jwiegley / use-package

A use-package declaration for simplifying your .emacs
https://jwiegley.github.io/use-package
GNU General Public License v3.0
4.42k stars 261 forks source link

Revert "Auto detect mode suffix in hook keyword" #1047

Closed jwiegley closed 1 year ago

jwiegley commented 1 year ago

This reverts commit 8bad61fe2ca77264b061ef9c8fe0247e2e6f5b4e.

This commit silently added -mode when determining the function to add to a hook. So for example:

(use-package foo
  :hook text-mode)

This would add foo-mode to text-mode-hook. However, this magical behavior is inconsistent. It doesn't happen for :mode, for example:

(use-package foo
  :mode "\\.foo\\'")

This adds just foo to the auto-mode-alist.

I think we should be consistent between these two use cases, and the less magic the better.

Further, this commit actually silently broke my config, since I had been using :hook buffer-save to call backup-each-save, but once it started suffixing -mode, my hook began attempting to call a non-existent function, because backup-each-save isn't a mode.

jwiegley commented 1 year ago

Ping @CeleritasCelery

CeleritasCelery commented 1 year ago

Further, this commit actually silently broke my config, since I had been using :hook buffer-save to call backup-each-save, but once it started suffixing -mode, my hook began attempting to call a non-existent function, because backup-each-save isn't a mode.

This is intentional. This change was not intended to be perfectly backwards compatible, but rather provide a sane default. As you discovered, most packages that you would add to a hook are modes. You found one in your config that was not, and I had one in my config that was not, but the vast majority are. The purpose of this is that you can use the short form (:hook text-mode) in the 95% of cases instead of having to use the long form (:hook (text-mode . foo-mode)). Before this change, the short form was not really usable in practice because most thing you would add to a hook were implicit modes. With backup-each-save you would just change it to (use-package backup-each-save :hook (buffer-save . backup-each-save)).

This would add foo-mode to text-mode-hook. However, this magical behavior is inconsistent. It doesn't happen for :mode

That seems like a reason to fix the behavior of :mode, not throw out the original change.

743

jwiegley commented 1 year ago

I really don't like having too much magic. I'm even torn on whether -hook should be auto-appended to the symbol on the left-hand side, but that one is easier for me to accept.

If there are no major objections other than the above, I will merge this breaking change toward the next release.

ywwry66 commented 1 year ago

README.md needs to be updated with this change. See https://github.com/jwiegley/use-package/commit/be3c570ced486db2c5bf6c0238b901f5ee4f64de.

jwiegley commented 1 year ago

That's a very good point, @ywwry66, thank you.