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

Feature/completion item/resolve #25

Merged

Conversation

krassowski
Copy link
Contributor

@krassowski krassowski commented Apr 28, 2021

Supersedes palantir/python-language-server#905. Rebase of #1.

  • implemented new hook for resolve
  • implemented resolve in jedi_completion and rope_completion
  • added eager option to rope_completion and jedi_completion allow users using clients which cannot yet handle completionItem/resolve to restore to eager resolution (inside of textDocument/completions)
  • added test for resolve and documentation in Jedi
  • tested with development version of JupyterLab-LSP client; the client implementation was tested against reference JavaScript/TypeScript server and against the R-languageserver

TODO:

  • document eager for rope

@andfoy andfoy added this to the v1.1.0 milestone Apr 29, 2021
@andfoy
Copy link
Contributor

andfoy commented May 19, 2021

@krassowski, any updates on this one?

@krassowski
Copy link
Contributor Author

No, sorry not yet. Feel free to pick it up if you need it. Otherwise, I will get to finish it eventually, but through May I am having busy time at Uni and cannot afford to do both maintenance and active development.

@andfoy
Copy link
Contributor

andfoy commented May 20, 2021

@krassowski, no worries! I was just checking in to see what was the state of this PR! Have a nice end of semester

@ccordoba12 ccordoba12 removed this from the v1.1.0 milestone Jun 25, 2021
@krassowski krassowski force-pushed the krassowski-feature/completionItem/resolve branch from c88c554 to 24911a8 Compare July 9, 2021 21:10
@krassowski krassowski marked this pull request as draft July 9, 2021 21:55
@krassowski krassowski marked this pull request as ready for review July 9, 2021 22:21
@krassowski
Copy link
Contributor Author

Sorry it took me so long to get back to this. I tried to use the proposed document-specific caching mechanism but I don't think it is possible as explained in #25 (comment).

@andfoy
Copy link
Contributor

andfoy commented Jul 9, 2021

Hi @krassowski, no worries! I've just left you a suggestion on the review discussion.

The document identifier is stored in the completion item data.
@krassowski krassowski force-pushed the krassowski-feature/completionItem/resolve branch from 5b82377 to 79fc79f Compare July 10, 2021 15:39
@krassowski krassowski requested a review from andfoy July 12, 2021 15:56
@@ -49,6 +49,11 @@
"default": false,
"description": "Enable fuzzy when requesting autocomplete."
},
"pylsp.plugins.jedi_completion.eager": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how we should manage these kind of configuration options that apply to both jedi, rope and possibly other completion backends

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think splitting them by plugin is fine because some plugins may be very fast to eagerly resolve completions and some may be slow. It might be beneficial to let user/IDE make this choice (not necessarily today with rope and jedi as only completion providers, but potentially in the future).

This made me realise I missed adding the setting to schema for rope...

@@ -162,8 +162,8 @@ def capabilities(self):
'resolveProvider': False, # We may need to make this configurable
},
'completionProvider': {
'resolveProvider': False, # We know everything ahead of time
'triggerCharacters': ['.']
'resolveProvider': True, # We could know everything ahead of time, but this takes time to transfer
Copy link
Contributor

Choose a reason for hiding this comment

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

If this value is True, then it means that eager is False, right? Then if a client decides to have all the information ready and sets eager to True, then it is their responsibility to not calling completionResolve?

Copy link
Contributor Author

@krassowski krassowski Jul 12, 2021

Choose a reason for hiding this comment

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

If this value is True, then it means that eager is False, right?

IMO no, eager still can be True. resolveProvider just declares that server will handle completionItem/resolve requests; it does not forbid server to return resolved items in the first place.

if a client decides to have all the information ready and sets eager to True, then it is their responsibility to not calling completionResolve?

No, the client is still allowed to call completionItem/resolve, it will be just inefficient for them to do so.

@andfoy
Copy link
Contributor

andfoy commented Jul 12, 2021

Hi @krassowski, thanks for your work on this one, I appreciate a lot that you managed to remove that global variable mechanism! I left you a few questions about the behaviour of this new feature.

@andfoy andfoy added this to the v1.2.0 milestone Jul 13, 2021
@andfoy
Copy link
Contributor

andfoy commented Jul 13, 2021

Thanks for this work @krassowski!

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.

3 participants