-
Notifications
You must be signed in to change notification settings - Fork 200
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
Update textDocument/documentSymbol parsing to LSP 3.10.0 spec #303
Conversation
…tavora#303) * eglot (eglot-imenu): Use helper function to parse DocumentSymbol tree.
c95b6d9
to
096df42
Compare
This seems useful, but I'm not familiar with this part of the code, so I'd rather not comment the code changes. However, there's a pending pull request (#213) also modifying imenu related code. There, João asked for writing accompanying tests. I think it's not a must, but can you do that for this PR? |
Nevermind, I see it's a recursive data structure, so recursiveness makes sense. |
Yeah, it's actually a tree (although I have no clue yet how to best use this for imenu).. :) I will try to get around to writing tests, but preferably by mocking the behavior of the language server, instead of relying on actual servers being available. In any case, that would probably only happen after I figure out how to extract the parsing from the xref part as well. Are there any comments explaining how that works? It seems to cache identifiers from the current buffer or sth like that..? |
@ilohmar do you know of any other server that supports this? I was hoping I could avoid installing |
Sorry, I have only used pyls, gopls and clangd myself. Have you ever worked with Go? I understand trying to avoid dependencies, however, the actual installation itself is easy and fast. |
Hi @joaotavora AFAIK, the Microsoft‘s python-language-server and Apple's swift language server sourcekit-lsp support I'm looking forward to seeing this feature supported by eglot. |
I'm not in a position to install any of those right now. But this is indeed in my queue. I will clean it up and put it in a side repo where you can test with those servers |
…tavora#303) * eglot (eglot-imenu): Use helper function to parse DocumentSymbol tree.
096df42
to
d5a9c7c
Compare
I just resolved some conflicts and now it applies cleanly on master. But I haven't tested in any way. |
@ilohmar Please specify |
Is there way to prioritize the PR? :) |
I get the message. It helps if you can confirm that it applies cleanly to master, addresses @muffinmad 's comment and works correctly for at least one server where it wasn't working before. Post your results here, please. Thanks. |
To be clear: doing any of those things helps, you don't have to do all of them to be useful. |
…tavora#303) * eglot (eglot-imenu): Use helper function to parse DocumentSymbol tree.
* eglot.el (eglot-client-capabilities, defvar): Add DocumentSymbol. (eglot-client-capabilities): Add :hierarchicalDocumentSymbolSupport. (eglot--parse-DocumentSymbol): Remove. (eglot-imenu): Rewrite.
d5a9c7c
to
57867bb
Compare
I've pushed some changes to the implementation, in hopes of simplifying it, but I don't have any server to test with yet. Maybe someone can do that testing for me? |
Sorry fo delay. It works! thank you very much! |
Thanks @joaotavora ! I can't test this feature right now, but I notice that Eglot is now sending |
Ahh, good catch. A simple |
Example python file: def some_function():
def some_inline_function():
pass
pass
class SomeClassName:
foo = 1
def method_name(self):
pass
class SomeOtherClass:
foo = 2
def method_name(self):
pass For that file the server responded to the
The Imenu structure now looks like this:
I used to Imenu structure provided by
I found the later Imenu structure more compact, intuitive and easy to use. No need to think about type of item to navigate to at the first place. Maybe it would be better to not group Imenu items by item type but only by hierarchical structure? What do you think? P.S. The python language server is |
Yes, I agree. |
…lot-imenu A reworking of an original implementation by Ingo Lohmar <ingo.lohmar@github.com> * eglot.el (eglot-client-capabilities, defvar): Add DocumentSymbol. (eglot-client-capabilities): Add :hierarchicalDocumentSymbolSupport. (eglot--parse-DocumentSymbol): Remove. (eglot-imenu): Rewrite. * NEWS.md (1.7): Mention new feature
…lot-imenu A reworking of an original implementation by Ingo Lohmar <ingo.lohmar@github.com> * eglot.el (eglot-client-capabilities, defvar): Add DocumentSymbol. (eglot-client-capabilities): Add :hierarchicalDocumentSymbolSupport. (eglot--parse-DocumentSymbol): Remove. (eglot-imenu): Rewrite. * NEWS.md (1.7): Mention new feature
A reworking of an original implementation by Ingo Lohmar <ingo.lohmar@github.com> * eglot.el (eglot-client-capabilities, defvar): Add DocumentSymbol. (eglot-client-capabilities): Add :hierarchicalDocumentSymbolSupport. (eglot--parse-DocumentSymbol): Remove. (eglot-imenu): Rewrite. * NEWS.md (1.7): Mention new feature #303: joaotavora/eglot#303
A reworking of an original implementation by Ingo Lohmar <ingo.lohmar@github.com> * eglot.el (eglot-client-capabilities, defvar): Add DocumentSymbol. (eglot-client-capabilities): Add :hierarchicalDocumentSymbolSupport. (eglot--parse-DocumentSymbol): Remove. (eglot-imenu): Rewrite. * NEWS.md (1.7): Mention new feature GitHub-reference: close joaotavora/eglot#303
https://microsoft.github.io/language-server-protocol/specification#textDocument_documentSymbol
The code is correct, I hope, but possibly very inelegant, as it is the
first time I have worked with the eglot-* macros.
Also note:
xref-backend-identifier-completion-table
needs the sameparsing, but I have not had a closer look at the xref part yet. I think
we should parse in one place, to a structure keeping all information,
and then cast that into the necessary form for either xref or imenu.