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

Parse config from initializeOptions and pass in the old value of config to onConfigurationChange #285

Merged
merged 4 commits into from
Feb 21, 2021

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Feb 20, 2021

onConfigurationChange is now pure, if you want to react to configuration changes you will have to define a handler for the WorkspaceDidChangeConfiguration message.

@lukel97
Copy link
Collaborator

lukel97 commented Feb 21, 2021

I thought initializationOptions and the workspace configuration were two different things, but it just so happened that some clients passed in the workspace configuration into it by coincidence? Which is why vscode doesn't actually pass it on startup.
If we want to preserve compatibility with coc.vim would it be possible to make a workspace/configuration request immediately after initialisation in lsp like you suggested, which would then call HLS's onConfigurationChange handler?

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 21, 2021

I'm pretty sure config was supposed to be passed into initializationOptions at one point, but was later supposedly deprecated. See the discussion on microsoft/language-server-protocol#972 (comment)

@wz1000 wz1000 merged commit 1778cab into haskell:master Feb 21, 2021
@andys8
Copy link
Contributor

andys8 commented Feb 27, 2021

If I'm not mistaken haskell-langauge-server 1.0.0 was released without this and should work with coc/vim. Would be awesome if this would be released and used in hls :) @wz1000

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 27, 2021

@andys8 we still need a lsp-test release, and I'm hoping to the all the other changes in.

/cc @bubba

@andys8
Copy link
Contributor

andys8 commented Feb 27, 2021

Sounds good. Thanks for the info 👍

@lukel97
Copy link
Collaborator

lukel97 commented Feb 27, 2021

@wz1000 doing the lsp-test release now, but for some reason I can no longer build lsp-types, I'm getting some mysterious TH error. I'm not sure whats changed


src/Language/LSP/Types/Lens.hs:66:1: error:
    • Type constructor ‘Language.LSP.Types.Method.Method’ cannot be used here
        (perhaps you intended to use DataKinds)
    • In the kind ‘Language.LSP.Types.Method.Method 'Language.LSP.Types.Method.FromClient t_aaEK0’
      In the first argument of ‘Registration’, namely
        ‘(m_aaEK1 :: Language.LSP.Types.Method.Method 'Language.LSP.Types.Method.FromClient t_aaEK0)’
      In the first argument of ‘HasId’, namely
        ‘(Registration (m_aaEK1 :: Language.LSP.Types.Method.Method 'Language.LSP.Types.Method.FromClient t_aaEK0))’
   |
66 | makeFieldsNoPrefix ''Registration
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@felixonmars
Copy link
Contributor

felixonmars commented Mar 25, 2021

@wz1000 doing the lsp-test release now, but for some reason I can no longer build lsp-types, I'm getting some mysterious TH error. I'm not sure whats changed


src/Language/LSP/Types/Lens.hs:66:1: error:
    • Type constructor ‘Language.LSP.Types.Method.Method’ cannot be used here
        (perhaps you intended to use DataKinds)
    • In the kind ‘Language.LSP.Types.Method.Method 'Language.LSP.Types.Method.FromClient t_aaEK0’
      In the first argument of ‘Registration’, namely
        ‘(m_aaEK1 :: Language.LSP.Types.Method.Method 'Language.LSP.Types.Method.FromClient t_aaEK0)’
      In the first argument of ‘HasId’, namely
        ‘(Registration (m_aaEK1 :: Language.LSP.Types.Method.Method 'Language.LSP.Types.Method.FromClient t_aaEK0))’
   |
66 | makeFieldsNoPrefix ''Registration
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I got the same failure with lens-5. It builds fine with lens-4.19.2 and others unchanged.

The required change {-# LANGUAGE DataKinds #-} is already present in this PR, so perhaps you are building an old unpatched version?

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