Skip to content

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
Collaborator

@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
Collaborator

@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
Collaborator

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
Collaborator

jdbaldry commented Sep 6, 2023

Also would you mind signing the CLA?

@jdbaldry
Copy link
Collaborator

jdbaldry commented Sep 8, 2023

Thanks for the contribution!

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