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

Add lsp-haskell-plugins config, for coming HLS feature #100

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

alanz
Copy link
Contributor

@alanz alanz commented Dec 21, 2020

This is a first stab at support for haskell/haskell-language-server#691

We will probably have to have a big switch here too, to not send the
new config unless we know hls supports it. How?

Provides context for #99

This is a first stab at support for haskell/haskell-language-server#691

We will probably have to have a big switch here too, to not send the
new config unless we know hls supports it. How?

Provides context for emacs-lsp#99
@alanz alanz requested a review from michaelpj December 21, 2020 16:43
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Seems plausible.

We will probably have to have a big switch here too, to not send the
new config unless we know hls supports it. How?

Ugh, I have no idea how you navigate this, actually. What does HLS do if you send config options that it doesn't know about? Perhaps ignoring it (or sending a warning?) will make this kind of thing easier in futuer?

@@ -238,7 +310,23 @@ and `lsp-haskell-server-args' and `lsp-haskell-server-wrapper-function'."
("haskell.liquidOn" lsp-haskell-liquid-on t)
("haskell.diagnosticsOnChange" lsp-haskell-diagnostics-on-change t)
("haskell.maxNumberOfProblems" lsp-haskell-max-number-of-problems)
("haskell.hlintOn" lsp-haskell-hlint-on t)))
("haskell.hlintOn" lsp-haskell-hlint-on t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to haskell.plugin.hlint.globalOn?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If so let's move the hlint defcustom under lsp-haskell-plugins too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the legacy config items, which will have to be moved/deprecated some time. See comments at haskell/haskell-language-server#691 (review)

@@ -238,7 +310,23 @@ and `lsp-haskell-server-args' and `lsp-haskell-server-wrapper-function'."
("haskell.liquidOn" lsp-haskell-liquid-on t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the liquid haskell support also going to become a plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a whole lot of legacy configuration items we brought over from HIE, some of which do things, some don't.

We have kept them so the parser doesn't blow up on the hls side if they get sent.

And we probably need a better deprecation story. It is important to report errors for malformed config, but it can become annoying.

;; ---------------------------------------------------------------------
;; Plugin-specific configuration
(defgroup lsp-haskell-plugins nil
"Customization group for 'lsp-haskell' plugins."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Customization group for 'lsp-haskell' plugins."
"Customization group for 'haskell-language-server' plugins."

They're not lsp-haskell plugins, after all!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, the elisp package is called lsp-haskell

@alanz
Copy link
Contributor Author

alanz commented Dec 21, 2020

@michaelpj FYI I just ran this version of the emacs client against an older HLS, and nothing bad happened (except the new config did not get recognised). And this is because the FromJSON instance looks for known keys, and so will ignore old ones. And uses defaults for missing keys. So should be safe in the case of dropping config items too.

https://github.com/haskell/haskell-language-server/blob/0063ec73340a3e741985b46da1b3a40e6ac9c03e/hls-plugin-api/src/Ide/Plugin/Config.hs#L77-L85

@alanz alanz marked this pull request as ready for review January 8, 2021 13:32
@alanz alanz merged commit 5d3f481 into emacs-lsp:master Jan 8, 2021
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