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

fix(lsp-mode.el): Make sure modules will be loaded #3779

Closed
wants to merge 1 commit into from

Conversation

jcs090218
Copy link
Member

By looking at the macro require's description:

If the optional third argument NOERROR is non-nil, then, if
the file is not found, the function returns nil instead of signaling
an error.  Normally the return value is FEATURE.

It seems like it only check file not found condition; I think it's better if we ignore all conditions so one lsp packages break won't break all of them.

@yyoncho
Copy link
Member

yyoncho commented Oct 24, 2022

But that way if the package that you want to use is broken you won't be able to understand why it doesn't load.

@jcs090218
Copy link
Member Author

jcs090218 commented Oct 24, 2022

But that way if the package that you want to use is broken you won't be able to understand why it doesn't load.

I have some thoughts on this issue,

  1. That wouldn't cause too many hassles since this will only ignore errors during the requiring time. And loading a bunch of modules at a time doesn't help you isolate the issue (unless you have debug-on-error on), and it will cause confusion when you are trying to figure out what is broken in the chain. I prefer ignoring it here, and debugging it by evaluating (require 'xxx) directly. I don't think one extra lsp package would hamper one another is a good idea.
  2. Add a debug flag, maybe a flag for developer/development mode?
;; just an example
(if lsp-debug
    (require package nil t)
  (ignore-errors (require package nil t)))
  1. The issue detector tasks should hand it over to CI. Something like following is a good start,
(let ((lsp-debug t)
      (lsp-auto-configure t))  ; this is enabled by default
  (lsp))  ; start the requiring test

All of them are design decisions. WDYT?

@yyoncho
Copy link
Member

yyoncho commented Oct 24, 2022

Have you observed packages that fail when you so (require 'lsp-xxx)? Because this is an indication of a bug. I see that some packages are doing executable-find on top level for example - we should remove these instances.

@jcs090218
Copy link
Member Author

lsp-metals is giving me several errors depends on your Emacs version,

Emacs 29+ snapshot (after emacs-mirror/emacs@80cf13a)

Eager macro-expansion failure: (wrong-number-of-arguments (1 . 1) 0)

Alexander-Miller/treemacs#982

Emacs 28.2 or earlier (before emacs-mirror/emacs@80cf13a)

load-with-code-conversion: Symbol’s function definition is void: treemacs-define-leaf-node

Alexander-Miller/treemacs#987

we should remove these instances.

You prefer to remove lsp-metals for now?

The worst-case scenario is me removing lsp-metals from my configuration which I don't want to do since I expect these bugs will be fixed one day. 😅 (mjhoy/dotfiles@0ee2c23)

@yyoncho
Copy link
Member

yyoncho commented Oct 25, 2022

We should fix metals.

@jcs090218
Copy link
Member Author

Got it! I think there is no reason for this patch anymore! Closing this now!

@jcs090218 jcs090218 closed this Oct 25, 2022
@jcs090218 jcs090218 deleted the fix/load branch March 17, 2024 21:43
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.

2 participants