Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

Only send completionItem/resolve if supported #2624

Merged
merged 6 commits into from
Oct 12, 2018

Conversation

feltech
Copy link
Contributor

@feltech feltech commented Oct 9, 2018

  • Oni will hang indefinitely if the language server simply chooses to
    ignore unsupported requests.
  • This can be avoided if the server provides correct capabilities and
    we actually check them before making the request.

* Oni will hang indefinitely if the language server simply chooses to
ignore unsupported requests.
* This can be avoided if the server provides correct capabilities and
we actually check them before making the request.
@feltech
Copy link
Contributor Author

feltech commented Oct 9, 2018

This addresses one of the shortcomings I mentioned in #1016 (comment)

@feltech
Copy link
Contributor Author

feltech commented Oct 9, 2018

There are still tests to write, so this is still WIP at the moment.

@feltech
Copy link
Contributor Author

feltech commented Oct 9, 2018

Also note to self (and whoever else cares): update wiki page to remove warning about completion failing in cquery/ccls once this is merged.

@codecov
Copy link

codecov bot commented Oct 9, 2018

Codecov Report

Merging #2624 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2624      +/-   ##
==========================================
+ Coverage    45.3%   45.33%   +0.02%     
==========================================
  Files         361      361              
  Lines       14569    14572       +3     
  Branches     1912     1913       +1     
==========================================
+ Hits         6601     6606       +5     
+ Misses       7744     7741       -3     
- Partials      224      225       +1
Impacted Files Coverage Δ
...er/src/Services/Completion/CompletionsRequestor.ts 32.35% <100%> (+19.44%) ⬆️
browser/src/Services/Notifications/Notification.ts 16.66% <0%> (-8.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4d877b...da3ba9b. Read the comment docs.

@akinsho
Copy link
Member

akinsho commented Oct 10, 2018

@feltech thanks for the PR 👍 not to make this more complex than your original intent I wonder if you've seen anywhere where we could generalise this for other lsp capabilities as this could come up again for another lsp maybe in the handler that calls this function there could be away to check if a capability is available before actually calling the function that handles the capability?

@feltech
Copy link
Contributor Author

feltech commented Oct 10, 2018

Re. generalising. The thought did occur to me.

It would seem sensible to at least have a final sanity check before sending a command to the language server (which could potentially hang if the command is not supported). However, based on the example of getting completion details, there isn't any immediately obvious automatic mapping between command and capability (e.g. completionItem/resolve to completionProvider.resolveProvider - they're sort-of similar, but not quite).

So I suspect the intention is to treat each command individually, unfortunately. I haven't looked in detail into the language server protocol, or at any other implementation to see how this problem is handled there. Perhaps there is a clever way to deal with this problem in the general case buried in other implementations.

@feltech
Copy link
Contributor Author

feltech commented Oct 10, 2018

OK, added tests.

I took the liberty of upgrading sinon (spys, stub, etc library). I wasn't quite sure what to do about yarn.lock, since it seemed to include more than just sinon version changes, so I left that out...

@akinsho
Copy link
Member

akinsho commented Oct 11, 2018

@feltech all for keeping it simple 👍, re then yarn lock we usually keep it updated under version control so other developers can resolve the right dependencies when they install the way it handles resolution of dependencies means it occasionally involves changes to more than one package so I wouldn't worry too much about that

Copy link
Member

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

Thanks for updating the yarn.lock 👍 turns out the changes involve a recent change yarn added, the specifics of which elude me which is why all the extra unexpected changes

@akinsho akinsho merged commit a369b79 into onivim:master Oct 12, 2018
@feltech feltech deleted the feature/improve_cpp_langserver_support branch October 12, 2018 13:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants