search

syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.56k stars 4.9k forks source link

improve ess loading and fix `ess-inferior-r-mode-map` bug #16366

Closed dankessler closed 1 month ago

dankessler commented 2 months ago

Note: this is closely related to #16364, but offers a more comprehensive fix without removing the functionality

This commit is motivated by a bug I encountered wherein ess would not load properly (e.g., deferred load triggered when editing foo.R) and I'd get an error about ess-inferior-r-mode-map being an undefined variable.

In the process of fixing the bug, I realized an opportunity to simplify the way ess gets autoloaded and configured.

Previously, use-package was defined for ess-site rather than ess; the autoloads for ess are such that one could go a long way without ever actually triggering ess-site, in which case nothing in the :config block would run. I've changed the use-package block to refer to ess, so that the autoloads will actually cause things in the :config block to fire.

Perhaps because of this, there were a whole bunch of with-eval-after-load blocks under :init that attempted to do just-in-time configuration of various ess features. However, this is kind of fragile, as sometimes ess gets refactored in such a way that it only makes since to do this configuration after multiple libraries are loaded. For example, the original bug RE ess-inferior-r-mode-map is one such instance. There was a block that looked like this:

(with-eval-after-load 'ess-inf
(spacemacs/ess-bind-keys-for-inferior))

and spacemacs/ess-bind-keys-for-inferior would try to manipulate two keymaps: inferior-ess-mode-map and inferior-ess-r-mode-map. The former is defined in ess-inf.el, but the latter is defined in ess-r-mode.el. Thus, this form would trigger as soon as ess-inf.el was loaded, but in many cases this was before ess-r-mode.el had been loaded, which caused the error.

I have streamlined all of this configuration by moving it out of the with-eval-after-load forms in :init and putting each directly into the :config block. In order to avoid the sundry dependencies of this code, the :config block now starts with (require 'ess-site), which should trigger loading of all of the libraries within ess. This may cause a bit of an overhead whenever ess is loaded, e.g., when the user first visits file foo.R, but the overhead shouldn't be experienced during startup and is probably worth it to have a more robust configuration.

Thank you :heart: for contributing to Spacemacs!

Before you submit this pull request, please ensure it is against the develop branch and that you have read the CONTRIBUTING.org file. It contains instructions on how to properly follow our conventions on commit messages, documentation, change log entries and other elements.

Don't forget to update CHANGELOG.develop if you want to be mentioned in the release file!

This message should be replaced with a description of your change.

Cheers!

gdkrmr commented 2 months ago

I have tested the fix and it works as intended.