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

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

Merged
merged 1 commit into from May 11, 2024

Conversation

dankessler
Copy link
Contributor

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 ❤️ 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!

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.
@dankessler dankessler mentioned this pull request Apr 10, 2024
@gdkrmr
Copy link
Contributor

gdkrmr commented Apr 12, 2024

I have tested the fix and it works as intended.

@smile13241324 smile13241324 merged commit 2fb41e7 into syl20bnr:develop May 11, 2024
8 checks passed
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.

None yet

3 participants