elbywan / crystalline

A Language Server Protocol implementation for Crystal. 🔮
MIT License
424 stars 21 forks source link

Crystalline is ignoring "hierarchicalDocumentSymbolSupport" on "documentSymbol" client capabilities. #12

Closed hugopl closed 3 years ago

hugopl commented 3 years ago

Hi,

Crystalline seems far better than first time I checked out, before first release, so I'm trying to use it on my editor and probably let it be the default LS for Crystal. However I found a minor issue that in my understanding seems like a wrong implementation of the LSP spec:

If the client capabilities for documentSymbol is set as:

    "documentSymbol": {
        "dynamicRegistration": false,
        "symbolKind": {
            "valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26]
        },
        "hierarchicalDocumentSymbolSupport": false
    }

A documentSymbol request should create a response with an array of SymbolInformation when hierarchicalDocumentSymbolSupport is set to false on client capabilities, but Crystalline is always sending an array of DocumentSymbol.

How to reproduce

  1. Compile master branch of Tijolo
  2. start it with tijolo --debug in some terminal.
  3. Configure it to use crystallise (:hamburger: menu -> preferences, replace crystal = "scry" by `crystal = "crystalline"
  4. Open a crystal file, hit Ctrp + P then type . (dot space).
  5. See the LSP communication on terminal, log file at /tmp or pressing Alt + 2
elbywan commented 3 years ago

Hey @hugopl,

Crystalline seems far better than first time I checked out, before first release, so I'm trying to use it on my editor and probably let it be the default LS for Crystal.

Cool, thanks for trying it out again 👍!

However I found a minor issue that in my understanding seems like a wrong implementation of the LSP spec:

That's unfortunately right, and actually the client capabilities were not handled at all as I wanted to focus on having a working server with a vscode client first.

I added the hierarchicalDocumentSymbolSupport support in the following PR: https://github.com/elbywan/crystalline/pull/13

Could you check and confirm that it works fine so I can merge? (artifacts can be downloaded here)

hugopl commented 3 years ago

I tested and the patch at https://github.com/elbywan/crystalline/pull/13/files worked nicely, thanks!

elbywan commented 3 years ago

Great! I just released v0.1.9 which contains the change 📦.