-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
treewide: do not overwrite lsp-client-settings changed by user #4421
base: master
Are you sure you want to change the base?
Conversation
42b0e44
to
9c0905c
Compare
upd: added a CI check for this to avoid regressions in the future |
42fdebe
to
3df350a
Compare
3df350a
to
c048a7f
Compare
If a user has set a setting inside `lsp-client-settings`, changing its value will result in a silent bug in user configuration that looks like a regression from `lsp-mode` mode update. Let's avoid that by adding a new function `lsp-register-new-settings` that avoids overwritting whatever was set by user and make use of it treewide. While at it, prohibit `lsp-register-custom-settings` from internal use for all the same reasons. Fixes: emacs-lsp#4420
c048a7f
to
03d89dd
Compare
Hmm, apparently github workflows doesn't work well with exclamation sign. Had to quote per advice over at the link, hopefully it will work as expected. |
Yeah, it didn't fail with a "unknown command", so it is working. |
Thanks for the contribution but I have some concerns.
... in your config. |
Sorry, I'm a bit confused here… Are you talking about the PR changes or about an alternative way a user may configure lsp-server settings? In the first case I'm unclear what you want me to do. In the latter case, the documentation for
Thank you, nice to know there's a workaround. |
The point is that when we migrate that file to lsp-defcustom you will still get the same issue unless you use the |
Ah, so you want me to fix |
No. I mean that lsp-defcustom is supposed to work that way and you cannot fix it because it won't behave as expected. So the only option seems to be to fix your config otherwise we will introduce inconsistencies (lsp-defcustom wont work like defcustom). |
Why? You earlier mentioned that it's supposed to work similarly to |
If you go to a defcustom form and eval only it - it will override the value. |
It doesn't, here's a testcase I just made with a separate file: λ cat test.el
(setq foo nil)
(defcustom foo t "test")
λ emacs --batch -l test.el --eval "(print foo)"
nil |
As a matter of fact, here's the relevant quote from the
IOW, if variable is "bound" aka exists, it does not assign the value to it. |
Any news here? |
From what I read, questions raised seems resolved, so… I imagine it'd be useful to have some solution to the user regressions discussed. I kind of hoped to have this merged faster, before more users are gonna be hit by the Python commit, but another week passed, so the hope becomes lesser. However, we still can make the code future-proof. |
Ping |
Ping 2 |
It overrides it if you eval only the defcustom form interactively. |
What version are you testing with? I just did a Also, I don't see how interactive behavior matters in this context. |
Oh, actually wait, I'm not sure why I had different result but now I am re-testing and indeed after iteractive re-evaluation it changes. But anyway, why does interactive behavior matter here? It is frequent that interactive behavior differ from non-interactive one, and the bug we are facing here is for non-interactive code. |
If a user has set a setting inside
lsp-client-settings
, changing its value will result in a silent bug in user configuration that looks like a regression fromlsp-mode
mode update. Let's avoid that by adding a new functionlsp-register-new-settings
that avoids overwritting whatever was set by user and make use of it treewide.While at it, prohibit
lsp-register-custom-settings
from internal use for all the same reasons.Fixes: #4420