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

Send settings on client initialization #125

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Conversation

anguslees
Copy link
Contributor

Send settings on LSP client initialization. Note jsonnet-language-server is unusual and expects configuration without a prefix. This is probably a bug in jsonnet-language-server, but this PR works with the existing code by passing through the "jsonnet" settings subtree, without the "jsonnet" prefix.

Example:

;; .emacs
(lsp-register-custom-settings
 '(("jsonnet.formatting"
   ;; jsonnetfmt --string-style d --comment-style s
   ((StringStyle . "double")
    (CommentStyle . "slash")))))

Send settings on LSP client initialization.  Note jsonnet-language-server is unusual and expects configuration without a prefix.  This is probably a bug in jsonnet-language-server, but this PR works with the existing code by passing through the "jsonnet" settings subtree, without the "jsonnet" prefix.

Example:
```elisp
;; .emacs
(lsp-register-custom-settings
 '(("jsonnet.formatting"
   ;; jsonnetfmt --string-style d --comment-style s
   ((StringStyle . "double")
    (CommentStyle . "slash")))))
```
@CLAassistant
Copy link

CLAassistant commented Sep 5, 2023

CLA assistant check
All committers have signed the CLA.

@julienduchesne
Copy link
Member

@jdbaldry, leaving this one to you 😄

(with-lsp-workspace workspace
(lsp--set-configuration
;; TODO: jsonnet-language-server settings should use a prefix
(ht-get (lsp-configuration-section "jsonnet") "jsonnet"))))
Copy link
Member

@jdbaldry jdbaldry Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question

Does this introduce a dependency on ht?

Research

It is already a transitive dependency since it is required by lsp-mode (https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L39)

Follow up question

Does this usage mean that this package should also declare the dependency explicitly?

Initial thought

I'm not at all an expert on Emacs packages but I'd assume it's safest to add the dependency explicitly.

Copy link
Contributor Author

@anguslees anguslees Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If lsp-mode stops using ht here, then this code is wrong and won't work. So I don't think an explicit dependency improves anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for checking :)

@jdbaldry
Copy link
Member

jdbaldry commented Sep 6, 2023

Also would you mind signing the CLA?

@jdbaldry
Copy link
Member

jdbaldry commented Sep 8, 2023

Thanks for the contribution!

@jdbaldry jdbaldry merged commit ebc2b49 into grafana:main Sep 8, 2023
4 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.

4 participants