-
Notifications
You must be signed in to change notification settings - Fork 182
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
Fix rust-mode lazy loading #530
Conversation
Thanks, @jroimartin can you see why the tests are failing ? |
Sure, I'm working on it. |
74f7728
to
b9baa86
Compare
@psibi PTAL. I think this is the smallest required change. On the flip side, it is a bit tricky. But having two major modes with the same name and loading them conditionally seems problematic. So, I opted for allowing the stub function (called |
Thanks, LGTM. @condy0919 Can you see if it works in your setup too ? I will test drive this PR too. |
Sorry, I'm in a touring🥺 can't help that |
No rush. There is a valid workaround which is just adding |
No worries, I'm also going to be on travel for the next two days. I will test this out more properly after that. |
rust-mode.el
Outdated
(require 'rust-mode-treesitter) | ||
(require 'rust-prog-mode)) | ||
;;;###autoload | ||
(defun rust-mode () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this function conflict with the rust-mode
function that is exposed by the rust-prog-mode / rust-treesitter-mode ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Infact, give that you are calling rust-mode
in the body of this function - it makes me more nervous. Do you think we can rename it to a different function instead, probably rust-mode-load
or something like that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psibi yeah, it is a bit hacky...
The thing is that we want to autoload rust-mode.el
when the user calls (rust-mode)
and then select the actual major mode. However, both versions of the major mode (rust-prog-mode
and rust-treesitter-mode
) are called rust-mode
and which one to choose depends on the environment (the emacs version and the value of rust-mode-treesitter-derive
). So, I don't know how to autoload one or the other conditionally.
Also, we cannot change the name of the major modes (and autoload both) because the hooks are named after them and other extensions expect these modes to be called rust-mode
.
So, this PR autoloads a stub called rust-mode
that is redefined by the specific call to define-derived-mode
. Thus, the following calls to rust-mode
execute the rust-mode
function corresponding to the actual major mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably
rust-mode-load
or something like that ?
Or rust-mode-maybe
, it's mentioned in the corresponding issue. But using another name may break users' config as many people manually do
(add-to-list 'auto-mode-alist '("\\.rs\\'" . rust-mode))
This PR fixes the following error that happends when opening a .rs file: File mode specification error: (void-function rust-mode) It conditionally autoloads the proper rust-mode version depending on the user environment. Fixes rust-lang#528
b9baa86
to
db7d086
Compare
@psibi @condy0919 what do you think of db7d086? This option does not require messing up with the |
This solution comes to mind at first, and the trick is used in the evil-exchange package. |
I did some testing and this seems to work fine. @jroimartin Thanks for the PR. @condy0919 Thanks for testing and review. |
This PR fixes the following error that happends when opening a .rs
file:
It conditionally autoloads the proper rust-mode version depending on
the user environment.
Fixes #528